Move remaining @CalledByNative methods out of ChromeApplication.

Move remaining @CalledByNative methods out of ChromeApplication.

This CL moves showPasswordSettings, areParentalControlsEnabled, and
openClearBrowsingData into more fitting locations.

BUG=698009
Review-Url: https://codereview.chromium.org/2768023002
Cr-Commit-Position: refs/heads/master@{#460132}
Committed: https://chromium.googlesource.com/chromium/src/+/cb80fc5be2e9a107a6c1f47d3a6ccc0e4391a91a

[estevenson, 2017-03-22 19:58:59]

The CQ bit was checked by estevenson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 19:59:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estevenson, 2017-03-22 20:06:55]

estevenson@chromium.org changed reviewers:
+ tedchoc@chromium.org

[estevenson, 2017-03-22 20:06:56]

ptal Ted!

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/pref...
File chrome/browser/android/preferences/preferences_launcher.h (right):

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/pref...
chrome/browser/android/preferences/preferences_launcher.h:18: class
PreferencesLauncher {
Before I submit https://codereview.chromium.org/2763133002/ I wanted to ask:

This doesn't need to be a class. Do we have a preference for classes with static
methods vs. functions in a namespace? I've seen both used fairly widely in
Chrome, the class method has the advantage that it can be used like
chrome::android::PreferencesLauncher, but it's also unnecessary, wdty?

[commit-bot: I haz the power, 2017-03-22 21:13:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 21:13:40]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-03-23 03:41:16]

lgtm

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_application.cc (right):

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_application.cc:99: bool
ChromeApplication::RegisterBindings(JNIEnv* env) {
can we delete this file now?

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/pref...
File chrome/browser/android/preferences/preferences_launcher.h (right):

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/pref...
chrome/browser/android/preferences/preferences_launcher.h:18: class
PreferencesLauncher {
On 2017/03/22 20:06:56, estevenson wrote:
> Before I submit https://codereview.chromium.org/2763133002/ I wanted to ask:
> 
> This doesn't need to be a class. Do we have a preference for classes with
static
> methods vs. functions in a namespace? I've seen both used fairly widely in
> Chrome, the class method has the advantage that it can be used like
> chrome::android::PreferencesLauncher, but it's also unnecessary, wdty?

I have no strong preference.  I'm inclined to simply leave this as is.

[estevenson, 2017-03-24 18:15:59]

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_application.cc (right):

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_application.cc:99: bool
ChromeApplication::RegisterBindings(JNIEnv* env) {
On 2017/03/23 03:41:15, Ted C wrote:
> can we delete this file now?

Not quite yet, need to remove the native methods first
(http://crbug.com/698015).

[estevenson, 2017-03-24 18:21:42]

estevenson@chromium.org changed reviewers:
+ bauerb@chromium.org, tsergeant@chromium.org, vabr@chromium.org

[estevenson, 2017-03-24 18:21:44]

+OWNERS reviewers

tsergeant@chromium.org: Please review changes in
chrome/browser/ui/webui/browsing_history_handler.cc

bauerb@chromium.org: Please review changes in
chrome/browser/prefs/incognito_mode_prefs.cc

vabr@chromium.org: Please review changes in 
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc and
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc

[vabr (Chromium), 2017-03-24 19:11:45]

> mailto:vabr@chromium.org: Please review changes in 
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc and
>
chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc

Those two files LGTM.

Cheers,
Vaclav

[tsergeant, 2017-03-26 22:17:21]

browsing_history_handler lgtm

[Bernhard Bauer, 2017-03-27 10:09:40]

https://codereview.chromium.org/2768023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java
(right):

https://codereview.chromium.org/2768023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java:61:
public static boolean isIncognitoDisabled() {
I would be tempted to expose this method to native code instead.

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs.cc:36: #include
"jni/PartnerBrowserCustomizations_jni.h"
Including JNI headers in arbitrary .cc files is a bit tricky, because they can
also include glue methods, so you might end up violating the ODR if you include
the same header in multiple files. It would be nicer to make a dedicated pair of
partner_browser_customizations.{h,cc} files where only the .cc file includes the
JNI header, and use that from here.

[estevenson, 2017-03-27 20:03:53]

https://codereview.chromium.org/2768023002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java
(right):

https://codereview.chromium.org/2768023002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java:61:
public static boolean isIncognitoDisabled() {
On 2017/03/27 10:09:40, Bernhard (OOO until Mar 27) wrote:
> I would be tempted to expose this method to native code instead.

Done.

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/prefs/incogn...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/2768023002/diff/1/chrome/browser/prefs/incogn...
chrome/browser/prefs/incognito_mode_prefs.cc:36: #include
"jni/PartnerBrowserCustomizations_jni.h"
On 2017/03/27 10:09:40, Bernhard (OOO until Mar 27) wrote:
> Including JNI headers in arbitrary .cc files is a bit tricky, because they can
> also include glue methods, so you might end up violating the ODR if you
include
> the same header in multiple files. It would be nicer to make a dedicated pair
of
> partner_browser_customizations.{h,cc} files where only the .cc file includes
the
> JNI header, and use that from here.

Done.

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/preferences/preferences_launcher.cc (right):

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/preferences_launcher.cc:7: #include
"base/android/jni_android.h"
I missed this include in https://codereview.chromium.org/2763133002 and also
mistakenly included <jni.h> in preferences_launcher.h.

We have no tool to automatically verify the correctness of #includes right?

[estevenson, 2017-03-27 20:04:11]

The CQ bit was checked by estevenson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 20:04:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 22:16:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 22:16:35]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-03-28 09:57:54]

Thanks! LGTM.

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/partner_browser_customizations.h (right):

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
chrome/browser/android/partner_browser_customizations.h:19:
PartnerBrowserCustomizations() {}
Nit: You can use DISALLOW_IMPLICIT_CONSTRUCTORS instead of
DISALLOW_COPY_AND_ASSIGN to take care of all of the constructors in one, and you
can just remove the destructor.

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/preferences/preferences_launcher.cc (right):

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/preferences_launcher.cc:7: #include
"base/android/jni_android.h"
On 2017/03/27 20:03:53, estevenson wrote:
> I missed this include in https://codereview.chromium.org/2763133002 and also
> mistakenly included <jni.h> in preferences_launcher.h.
> 
> We have no tool to automatically verify the correctness of #includes right?

Not that I know of, although there is a Clang-based tool, see
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/41f1k52xifg/dis....

[estevenson, 2017-03-28 14:59:39]

Thanks for the reviews!

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/partner_browser_customizations.h (right):

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
chrome/browser/android/partner_browser_customizations.h:19:
PartnerBrowserCustomizations() {}
On 2017/03/28 09:57:53, Bernhard Bauer wrote:
> Nit: You can use DISALLOW_IMPLICIT_CONSTRUCTORS instead of
> DISALLOW_COPY_AND_ASSIGN to take care of all of the constructors in one, and
you
> can just remove the destructor.

Done.

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/preferences/preferences_launcher.cc (right):

https://codereview.chromium.org/2768023002/diff/20001/chrome/browser/android/...
chrome/browser/android/preferences/preferences_launcher.cc:7: #include
"base/android/jni_android.h"
On 2017/03/28 09:57:53, Bernhard Bauer wrote:
> On 2017/03/27 20:03:53, estevenson wrote:
> > I missed this include in https://codereview.chromium.org/2763133002 and also
> > mistakenly included <jni.h> in preferences_launcher.h.
> > 
> > We have no tool to automatically verify the correctness of #includes right?
> 
> Not that I know of, although there is a Clang-based tool, see
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/41f1k52xifg/dis....

Cool, thanks!

[estevenson, 2017-03-28 15:00:42]

The CQ bit was checked by estevenson@chromium.org

[estevenson, 2017-03-28 15:00:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, tsergeant@chromium.org, vabr@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2768023002/#ps40001
(title: "Use DISALLOW_IMPLICIT_CONSTRUCTORS instead of
DISALLOW_COPY_AND_ASSIGN")

[commit-bot: I haz the power, 2017-03-28 15:01:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 16:51:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490713242735500,
"parent_rev": "e1d5761c09510d11548e372e199dfdd75aa96c09", "commit_rev":
"cb80fc5be2e9a107a6c1f47d3a6ccc0e4391a91a"}

[commit-bot: I haz the power, 2017-03-28 16:51:42]

Description was changed from

==========
Move remaining @CalledByNative methods out of ChromeApplication.

This CL moves showPasswordSettings, areParentalControlsEnabled, and
openClearBrowsingData into more fitting locations.

BUG=698009
==========

to

==========
Move remaining @CalledByNative methods out of ChromeApplication.

This CL moves showPasswordSettings, areParentalControlsEnabled, and
openClearBrowsingData into more fitting locations.

BUG=698009

Review-Url: https://codereview.chromium.org/2768023002
Cr-Commit-Position: refs/heads/master@{#460132}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb80fc5be2e9a107a6c1f47d3a6c...
==========

[commit-bot: I haz the power, 2017-03-28 16:51:43]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cb80fc5be2e9a107a6c1f47d3a6c...