Put a Settings link on the new Google Search geolocation disclosure.

Put a Settings link on the new Google Search geolocation disclosure.

This allows users to block geolocation access for Google Search.

BUG=661011
Committed: https://crrev.com/cfbb5a87d44b0d2a420e167d907fece883daf0ee
Cr-Commit-Position: refs/heads/master@{#430768}

[benwells, 2016-11-08 03:18:52]

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

[commit-bot: I haz the power, 2016-11-08 03:19:23]

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

[benwells, 2016-11-08 03:53:29]

benwells@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[gone, 2016-11-08 04:40:28]

lgtm % comments.

Won't press for the changes to onLinkClicked() because I think the infobar is
supposed to be relatively simple in the end.

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java
(right):

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:59:
public void createContent(InfoBarLayout layout) {
Currently empty, but call super.createContent(layout) anyway.

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:78:
public void onLinkClicked() {
Will leave this up to you, but I'd prefer to avoid straying too far from how
onLinkClicked is expected to normally work:

1) Rename this function something else so that it doesn't override onLinkClicked

2) Make the native side SearchGeolocationDisclosureInfoBar::onLinkClicked():
   a) Hop back into Java to pop open the dialog
   b) Close itself like it currently does

It does cost another JNI hop, but I think the flow is more faithful to the
framework (*InfoBar.java subclasses are supposed to exist for customizing the UI
styling, with the delegates doing the actual work and interfacing with the
backend).  If this infobar expands a lot I'd push for moving this to the
delegate, but I'm fine for now.

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:84:
context.startActivity(settingsIntent);
IntentUtils.safeStartActivity(context, settingsIntent);

https://codereview.chromium.org/2482023002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2482023002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15603: +        <message
name="IDS_SEARCH_GEOLOCATION_DISCLOSURE_INFOBAR_SETTINGS_LINK_TEXT" desc="Text
to put in the link to the Settings page in the search geolocation disclosure
UI.">
It'd be good to just reuse the actual Settings title (IDS_PREFERENCES) in case
they ever get renamed to something else.

[benwells, 2016-11-08 11:26:34]

Hey Dan, how's this? Since the feedback was non-trivial and I'm not sure if I
got it right, I'll wait for your lg before landing.

If you think I should make the native infobar call the infobar_delegate, and
have that call something like GeolocationHeader to show the settings page, let
me know and I'll do that.

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java
(right):

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:59:
public void createContent(InfoBarLayout layout) {
On 2016/11/08 04:40:28, dfalcantara (check my queue) wrote:
> Currently empty, but call super.createContent(layout) anyway.

Done.

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:78:
public void onLinkClicked() {
On 2016/11/08 04:40:27, dfalcantara (check my queue) wrote:
> Will leave this up to you, but I'd prefer to avoid straying too far from how
> onLinkClicked is expected to normally work:
> 
> 1) Rename this function something else so that it doesn't override
onLinkClicked
> 
> 2) Make the native side SearchGeolocationDisclosureInfoBar::onLinkClicked():
>    a) Hop back into Java to pop open the dialog
>    b) Close itself like it currently does
> 
> It does cost another JNI hop, but I think the flow is more faithful to the
> framework (*InfoBar.java subclasses are supposed to exist for customizing the
UI
> styling, with the delegates doing the actual work and interfacing with the
> backend).  If this infobar expands a lot I'd push for moving this to the
> delegate, but I'm fine for now.

Done.

https://codereview.chromium.org/2482023002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:84:
context.startActivity(settingsIntent);
On 2016/11/08 04:40:27, dfalcantara (check my queue) wrote:
> IntentUtils.safeStartActivity(context, settingsIntent);

Done.

https://codereview.chromium.org/2482023002/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2482023002/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:15603: +        <message
name="IDS_SEARCH_GEOLOCATION_DISCLOSURE_INFOBAR_SETTINGS_LINK_TEXT" desc="Text
to put in the link to the Settings page in the search geolocation disclosure
UI.">
On 2016/11/08 04:40:28, dfalcantara (check my queue) wrote:
> It'd be good to just reuse the actual Settings title (IDS_PREFERENCES) in case
> they ever get renamed to something else.

IDS_PREFERENCES is "Preferences", and "IDS_SETTINGS" has an ampersand in it. I
couldn't find something that just mapped to Settings.

[gone, 2016-11-08 18:13:46]

Yeah, that's pretty much what I was asking for :)  lgtm again

There's actually two IDS_SETTINGS strings, with one in
android_chrome_strings.grd.  I'm honestly not sure which one takes precedence
when the resources are compiled together.  Kind of strange that the PAK file
generator doesn't complain about it either :/

[benwells, 2016-11-08 22:42:39]

On 2016/11/08 18:13:46, dfalcantara (check my queue) wrote:
> Yeah, that's pretty much what I was asking for :)  lgtm again
> 
> There's actually two IDS_SETTINGS strings, with one in
> android_chrome_strings.grd.  I'm honestly not sure which one takes precedence
> when the resources are compiled together.  Kind of strange that the PAK file
> generator doesn't complain about it either :/

Oh ... weird. I tried IDS_SETTINGS and in my build it had the ampersand. I was
hoping it would use the android one but it didn't, so I've left it as is.

[benwells, 2016-11-08 22:42:46]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2016-11-08 22:43:32]

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, 2016-11-08 23:47:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-08 23:54:58]

Description was changed from

==========
Put a Settings link on the new Google Search geolocation disclosure.

This allows users to block geolocation access for Google Search.

BUG=661011
==========

to

==========
Put a Settings link on the new Google Search geolocation disclosure.

This allows users to block geolocation access for Google Search.

BUG=661011

Committed: https://crrev.com/cfbb5a87d44b0d2a420e167d907fece883daf0ee
Cr-Commit-Position: refs/heads/master@{#430768}
==========

[commit-bot: I haz the power, 2016-11-08 23:54:59]

Patchset 5 (id:??) landed as
https://crrev.com/cfbb5a87d44b0d2a420e167d907fece883daf0ee
Cr-Commit-Position: refs/heads/master@{#430768}