Update the Google Search geolocation disclosure to make it more obvious.

Update the Google Search geolocation disclosure to make it more obvious.

This Android change (behind a flag) is the first in a series of patches
to make the Google Search geolocation disclosure more noticeable.

When the flag is enabled, this patch will:
- disable the existing disclosure
- show a new infobar disclosure when
  - an omnibox search has just been done
  - Google search is the default search engine
  - Chrome has geolocation permission
  - The current default search origin has not had geolocation allowed
    or blocked by the user.

Note, this patch shows the disclosure on every navigation that meets
these conditions.

Future patches will limit how many times the disclosure is shown, will
update the infobar to have a link, and update the infobar to always
be on top of other infobars.

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

[benwells, 2016-11-04 06:33:16]

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

[commit-bot: I haz the power, 2016-11-04 06:33:33]

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, 2016-11-04 07:33:18]

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

[commit-bot: I haz the power, 2016-11-04 07:33:19]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-11-04 09:25:56]

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

[benwells, 2016-11-04 09:25:56]

Hi Dan,

Please have a look. This is just the first patch getting most of the boilerplate
in, with more interesting stuff to come.

I've probably got more boilerplate than actually needed, let me know if there's
stuff I can remove.

And the names are all a bit long, I think I got carried away doing Java stuff.

Cheers
Ben.

[gone, 2016-11-04 23:21:49]

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

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:14:
public class SearchGeolocationDisclosureInfoBar extends InfoBar {
I guess you need a new class because you'll be special-casing things off of the
infobar type?

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:39:
layout.setMessage(mMessageText);
I don't think this is necessary if you pass messageText as the third parameter
to the super constructor.

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3234: return
GeolocationHeader.hasGeolocationPermission(getWindowAndroid().getContext().get());
Confirmed with Ted that you don't actually need the WindowAndroid here; you can
just use ContextUtils.getApplicationContext().  (Hopefully not Tab, because
Tab's pretty bloated already.)  You might just be able to make
hasGeolocationPermission call ContextUtils.getApplicationContext() itself and
make that function @CalledByNative.

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/sear...
File chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h
(right):

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h:33:
static void Create(content::WebContents* web_contents);
newline here

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.cc:649:
MaybeShowDefaultSearchGeolocationDisclosure(gurl);
Is it possible to add this to a WebContentsObserver instead?  I'd like to avoid
adding more stuff to Tab/TabAndroid—especially since the function you're adding
doesn't seem to care too much about the Tab itself (just the Window and the
WebContents, AFAICT).

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.h
(right):

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.h:14:
class SearchGeolocationDisclosureInfoBar : public InfoBarAndroid {
Does it make more sense to override ConfirmInfoBar?  Are you planning on adding
any buttons to this thing?

https://codereview.chromium.org/2475213002/diff/1/components/infobars/core/in...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2475213002/diff/1/components/infobars/core/in...
components/infobars/core/infobar_delegate.h:148:
SEARCH_GEOLOCATION_DISCLOSURE_INFOBAR_DELEGATE = 72,
You need to update histograms.xml.

[benwells, 2016-11-07 07:04:21]

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

[commit-bot: I haz the power, 2016-11-07 07:04:32]

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, 2016-11-07 07:23:55]

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

[commit-bot: I haz the power, 2016-11-07 07:23:55]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[benwells, 2016-11-07 08:36:06]

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

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:14:
public class SearchGeolocationDisclosureInfoBar extends InfoBar {
On 2016/11/04 23:21:48, dfalcantara (check my queue) wrote:
> I guess you need a new class because you'll be special-casing things off of
the
> infobar type?

This infobar will be pretty simple except it will have a link embedded in the
message, a bit like GeneratedPasswordSavedInfoBar.java.

It won't have any buttons.

Will I need a new class for that?

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:39:
layout.setMessage(mMessageText);
On 2016/11/04 23:21:48, dfalcantara (check my queue) wrote:
> I don't think this is necessary if you pass messageText as the third parameter
> to the super constructor.

Done.

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3234: return
GeolocationHeader.hasGeolocationPermission(getWindowAndroid().getContext().get());
On 2016/11/04 23:21:48, dfalcantara (check my queue) wrote:
> Confirmed with Ted that you don't actually need the WindowAndroid here; you
can
> just use ContextUtils.getApplicationContext().  (Hopefully not Tab, because
> Tab's pretty bloated already.)  You might just be able to make
> hasGeolocationPermission call ContextUtils.getApplicationContext() itself and
> make that function @CalledByNative.

Done.

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/sear...
File chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h
(right):

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h:33:
static void Create(content::WebContents* web_contents);
On 2016/11/04 23:21:48, dfalcantara (check my queue) wrote:
> newline here

Done.

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.cc:649:
MaybeShowDefaultSearchGeolocationDisclosure(gurl);
On 2016/11/04 23:21:48, dfalcantara (check my queue) wrote:
> Is it possible to add this to a WebContentsObserver instead?  I'd like to
avoid
> adding more stuff to Tab/TabAndroid—especially since the function you're
adding
> doesn't seem to care too much about the Tab itself (just the Window and the
> WebContents, AFAICT).

Done.

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.h
(right):

https://codereview.chromium.org/2475213002/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.h:14:
class SearchGeolocationDisclosureInfoBar : public InfoBarAndroid {
On 2016/11/04 23:21:49, dfalcantara (check my queue) wrote:
> Does it make more sense to override ConfirmInfoBar?  Are you planning on
adding
> any buttons to this thing?

No buttons, just a message with a link embedded in it.

https://codereview.chromium.org/2475213002/diff/1/components/infobars/core/in...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2475213002/diff/1/components/infobars/core/in...
components/infobars/core/infobar_delegate.h:148:
SEARCH_GEOLOCATION_DISCLOSURE_INFOBAR_DELEGATE = 72,
On 2016/11/04 23:21:49, dfalcantara (check my queue) wrote:
> You need to update histograms.xml.

Done.

https://codereview.chromium.org/2475213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
(right):

https://codereview.chromium.org/2475213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:58:
if (!hasGeolocationPermission()) return;
Maybe I should create two versions of this to be able to reuse the context that
is passed in?

[gone, 2016-11-07 19:01:53]

lgtm

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

https://codereview.chromium.org/2475213002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBar.java:14:
public class SearchGeolocationDisclosureInfoBar extends InfoBar {
On 2016/11/07 08:36:06, benwells (slow) wrote:
> On 2016/11/04 23:21:48, dfalcantara (check my queue) wrote:
> > I guess you need a new class because you'll be special-casing things off of
> the
> > infobar type?
> 
> This infobar will be pretty simple except it will have a link embedded in the
> message, a bit like GeneratedPasswordSavedInfoBar.java.
> 
> It won't have any buttons.
> 
> Will I need a new class for that?

That InfoBar seems to be gone, but if I remember it correctly you'll probably
need to do something similar to it because embedded link text isn't supported in
the base class.  I'm not picturing this class getting too crazy.

https://codereview.chromium.org/2475213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
(right):

https://codereview.chromium.org/2475213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:58:
if (!hasGeolocationPermission()) return;
On 2016/11/07 08:36:06, benwells (slow) wrote:
> Maybe I should create two versions of this to be able to reuse the context
that
> is passed in?

You mean making two hasGeolocationPermission functions, with one just calling
the other with the result of ContextUtils.getApplicationContext()?  I'd be fine
with that.

[benwells, 2016-11-07 22:44:20]

benwells@chromium.org changed reviewers:
+ isherman@chromium.org, pkasting@chromium.org

[benwells, 2016-11-07 22:44:20]

+pkasting for chrome/browser/ui/tab_helpers and components/infobars
+isherman for histograms

https://codereview.chromium.org/2475213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java
(right):

https://codereview.chromium.org/2475213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:58:
if (!hasGeolocationPermission()) return;
On 2016/11/07 19:01:53, dfalcantara (check my queue) wrote:
> On 2016/11/07 08:36:06, benwells (slow) wrote:
> > Maybe I should create two versions of this to be able to reuse the context
> that
> > is passed in?
> 
> You mean making two hasGeolocationPermission functions, with one just calling
> the other with the result of ContextUtils.getApplicationContext()?  I'd be
fine
> with that.

Done.

[Ilya Sherman, 2016-11-08 01:15:53]

histograms.xml lgtm

[Peter Kasting, 2016-11-08 01:17:51]

LGTM with comments

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation_disclosure_infobar_delegate.cc
(right):

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.cc:30:
std::unique_ptr<infobars::InfoBar> infobar(
Nit: Prefer base::MakeUnique() to bare new:

  auto infobar = base::MakeUnique<SearchGeolocationDisclosureInfoBar>(...);

Although I would likely just copy the other infobars and inline this into the
next statement, even though it's ugly... but up to you.

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h
(right):

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h:33:
static void Create(content::WebContents* web_contents);
Nit: I would tend to prefer this immediately after the destructor, since it's
the first thing callers access and it's kind of serving as a constructor. 
Should have a descriptive comment too (as should GetMessageText(), probably).

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h:36:
SearchGeolocationDisclosureInfoBarDelegate();
Why is this protected when there are no subclasses?  I think it should be
private.

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.cc
(right):

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.cc:24:
delegate()->AsSearchGeolocationDisclosureInfoBarDelegate();
Nit: Use GetDelegate()

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.cc:41:
return delegate()->AsSearchGeolocationDisclosureInfoBarDelegate();
Don't add this virtual downcast method; you know this delegate is of the right
type, so just static-cast it.

We try to avoid adding the AsXXXDelegate() functions if possible.

[benwells, 2016-11-08 02:23:02]

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

[commit-bot: I haz the power, 2016-11-08 02:23:37]

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 02:53:40]

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation_disclosure_infobar_delegate.cc
(right):

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.cc:30:
std::unique_ptr<infobars::InfoBar> infobar(
On 2016/11/08 01:17:51, Peter Kasting wrote:
> Nit: Prefer base::MakeUnique() to bare new:
> 
>   auto infobar = base::MakeUnique<SearchGeolocationDisclosureInfoBar>(...);
> 
> Although I would likely just copy the other infobars and inline this into the
> next statement, even though it's ugly... but up to you.

Done.

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h
(right):

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h:33:
static void Create(content::WebContents* web_contents);
On 2016/11/08 01:17:51, Peter Kasting wrote:
> Nit: I would tend to prefer this immediately after the destructor, since it's
> the first thing callers access and it's kind of serving as a constructor. 
> Should have a descriptive comment too (as should GetMessageText(), probably).

Done.

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation_disclosure_infobar_delegate.h:36:
SearchGeolocationDisclosureInfoBarDelegate();
On 2016/11/08 01:17:51, Peter Kasting wrote:
> Why is this protected when there are no subclasses?  I think it should be
> private.

Made private.

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.cc
(right):

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.cc:24:
delegate()->AsSearchGeolocationDisclosureInfoBarDelegate();
On 2016/11/08 01:17:51, Peter Kasting wrote:
> Nit: Use GetDelegate()

Done.

https://codereview.chromium.org/2475213002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/search_geolocation_disclosure_infobar.cc:41:
return delegate()->AsSearchGeolocationDisclosureInfoBarDelegate();
On 2016/11/08 01:17:51, Peter Kasting wrote:
> Don't add this virtual downcast method; you know this delegate is of the right
> type, so just static-cast it.
> 
> We try to avoid adding the AsXXXDelegate() functions if possible.

Cool, glad to hear that's the way its done :)

[commit-bot: I haz the power, 2016-11-08 03:24:21]

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

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

Dry run: This issue passed the CQ dry run.

[benwells, 2016-11-08 03:59:15]

The CQ bit was checked by benwells@chromium.org

[benwells, 2016-11-08 03:59:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, pkasting@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2475213002/#ps60001
(title: "Feedback")

[commit-bot: I haz the power, 2016-11-08 03:59:34]

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 04:06:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-08 04:14:32]

Description was changed from

==========
Update the Google Search geolocation disclosure to make it more obvious.

This Android change (behind a flag) is the first in a series of patches
to make the Google Search geolocation disclosure more noticeable.

When the flag is enabled, this patch will:
- disable the existing disclosure
- show a new infobar disclosure when
  - an omnibox search has just been done
  - Google search is the default search engine
  - Chrome has geolocation permission
  - The current default search origin has not had geolocation allowed
    or blocked by the user.

Note, this patch shows the disclosure on every navigation that meets
these conditions.

Future patches will limit how many times the disclosure is shown, will
update the infobar to have a link, and update the infobar to always
be on top of other infobars.

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

to

==========
Update the Google Search geolocation disclosure to make it more obvious.

This Android change (behind a flag) is the first in a series of patches
to make the Google Search geolocation disclosure more noticeable.

When the flag is enabled, this patch will:
- disable the existing disclosure
- show a new infobar disclosure when
  - an omnibox search has just been done
  - Google search is the default search engine
  - Chrome has geolocation permission
  - The current default search origin has not had geolocation allowed
    or blocked by the user.

Note, this patch shows the disclosure on every navigation that meets
these conditions.

Future patches will limit how many times the disclosure is shown, will
update the infobar to have a link, and update the infobar to always
be on top of other infobars.

BUG=661011

Committed: https://crrev.com/3503a8fc3531a8b903aef6db34bed83a73d2960d
Cr-Commit-Position: refs/heads/master@{#430516}
==========

[commit-bot: I haz the power, 2016-11-08 04:14:33]

Patchset 4 (id:??) landed as
https://crrev.com/3503a8fc3531a8b903aef6db34bed83a73d2960d
Cr-Commit-Position: refs/heads/master@{#430516}