Update page info for new search geolocation system.

Update page info for new search geolocation system.

Geolocation access via the geo API and via the X-Geo header has been
made consistent. As part of this project the UI is being updated to show
the geolocation status for the default search engine in more places.

This change updates the page info popup to show allow / block for the
default search engine.

BUG=674398
Review-Url: https://codereview.chromium.org/2633433002
Cr-Commit-Position: refs/heads/master@{#443454}
Committed: https://chromium.googlesource.com/chromium/src/+/7b28e26454b336fce8ad4eb8a5dd2597c0750eaa

[benwells, 2017-01-12 07:16:45]

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

[benwells, 2017-01-12 07:16:45]

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

[benwells, 2017-01-12 07:16:46]

Tim for local review

[benwells, 2017-01-12 07:16:46]

Tim for local review

[tsergeant, 2017-01-12 07:23:51]

lgtm

https://codereview.chromium.org/2633433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/2633433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:632:
**/
Nit: Extra * here

[benwells, 2017-01-12 07:24:05]

+cc raymes

[benwells, 2017-01-12 07:26:05]

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

[commit-bot: I haz the power, 2017-01-12 07:26:16]

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

[benwells, 2017-01-12 07:26:50]

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

[commit-bot: I haz the power, 2017-01-12 07:27:05]

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-01-12 08:00:18]

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

[commit-bot: I haz the power, 2017-01-12 08:00:19]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-01-12 10:16:11]

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

[benwells, 2017-01-12 10:16:12]

+tedchoc for owners review

[Ted C, 2017-01-12 20:11:18]

lgtm

Slightly worried that we might miss cases by not handling this much lower where
we query content settings, but this seems like an ok thing for now.

[benwells, 2017-01-12 22:31:17]

On 2017/01/12 20:11:18, Ted C wrote:
> lgtm
> 
> Slightly worried that we might miss cases by not handling this much lower
where
> we query content settings, but this seems like an ok thing for now.

Are there specific kinds of things you're worried about?

If you look in search_geolocation_service.cc [1] and
GeolocationPermissionContextAndroid::GetPermissionStatusInternal [2] we cover
lots of weird edge cases (e.g. enterprise policy, search engines changing,
etc.), which we mapped out pretty thoroughly in the encyclopedic design doc [3].
At this layer (just updating the UI so people know what's going on) hopefully it
should be straightforward, but this project has impressed the complexity of
permissions on me, so I wouldn't be surprised if there are cases we've missed.

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...
[2]:
https://cs.chromium.org/chromium/src/chrome/browser/geolocation/geolocation_p...
[3]:
https://docs.google.com/a/google.com/document/d/1iNVcrm5xxQf1FdCPwm2VWPoQ2F52...

[Ted C, 2017-01-12 22:54:19]

On 2017/01/12 22:31:17, benwells wrote:
> On 2017/01/12 20:11:18, Ted C wrote:
> > lgtm
> > 
> > Slightly worried that we might miss cases by not handling this much lower
> where
> > we query content settings, but this seems like an ok thing for now.
> 
> Are there specific kinds of things you're worried about?
> 
> If you look in search_geolocation_service.cc [1] and
> GeolocationPermissionContextAndroid::GetPermissionStatusInternal [2] we cover
> lots of weird edge cases (e.g. enterprise policy, search engines changing,
> etc.), which we mapped out pretty thoroughly in the encyclopedic design doc
[3].
> At this layer (just updating the UI so people know what's going on) hopefully
it
> should be straightforward, but this project has impressed the complexity of
> permissions on me, so I wouldn't be surprised if there are cases we've missed.
> 
> [1]:
>
https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...
> [2]:
>
https://cs.chromium.org/chromium/src/chrome/browser/geolocation/geolocation_p...
> [3]:
>
https://docs.google.com/a/google.com/document/d/1iNVcrm5xxQf1FdCPwm2VWPoQ2F52...

Nothing specific.  It just seems that as we build these layers on top of content
settings we should potentially prevent accessing the raw content setting if
we want to make them more complicated.

Unfounded fear for sure.

[benwells, 2017-01-12 23:17:28]

On 2017/01/12 22:54:19, Ted C wrote:
> On 2017/01/12 22:31:17, benwells wrote:
> > On 2017/01/12 20:11:18, Ted C wrote:
> > > lgtm
> > > 
> > > Slightly worried that we might miss cases by not handling this much lower
> > where
> > > we query content settings, but this seems like an ok thing for now.
> > 
> > Are there specific kinds of things you're worried about?
> > 
> > If you look in search_geolocation_service.cc [1] and
> > GeolocationPermissionContextAndroid::GetPermissionStatusInternal [2] we
cover
> > lots of weird edge cases (e.g. enterprise policy, search engines changing,
> > etc.), which we mapped out pretty thoroughly in the encyclopedic design doc
> [3].
> > At this layer (just updating the UI so people know what's going on)
hopefully
> it
> > should be straightforward, but this project has impressed the complexity of
> > permissions on me, so I wouldn't be surprised if there are cases we've
missed.
> > 
> > [1]:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...
> > [2]:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/geolocation/geolocation_p...
> > [3]:
> >
>
https://docs.google.com/a/google.com/document/d/1iNVcrm5xxQf1FdCPwm2VWPoQ2F52...
> 
> Nothing specific.  It just seems that as we build these layers on top of
content
> settings we should potentially prevent accessing the raw content setting if
> we want to make them more complicated.
> 
> Unfounded fear for sure.

[benwells, 2017-01-12 23:25:34]

On 2017/01/12 23:17:28, benwells wrote:
> On 2017/01/12 22:54:19, Ted C wrote:
> > On 2017/01/12 22:31:17, benwells wrote:
> > > On 2017/01/12 20:11:18, Ted C wrote:
> > > > lgtm
> > > > 
> > > > Slightly worried that we might miss cases by not handling this much
lower
> > > where
> > > > we query content settings, but this seems like an ok thing for now.
> > > 
> > > Are there specific kinds of things you're worried about?
> > > 
> > > If you look in search_geolocation_service.cc [1] and
> > > GeolocationPermissionContextAndroid::GetPermissionStatusInternal [2] we
> cover
> > > lots of weird edge cases (e.g. enterprise policy, search engines changing,
> > > etc.), which we mapped out pretty thoroughly in the encyclopedic design
doc
> > [3].
> > > At this layer (just updating the UI so people know what's going on)
> hopefully
> > it
> > > should be straightforward, but this project has impressed the complexity
of
> > > permissions on me, so I wouldn't be surprised if there are cases we've
> missed.
> > > 
> > > [1]:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/android/search_geolocatio...
> > > [2]:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/geolocation/geolocation_p...
> > > [3]:
> > >
> >
>
https://docs.google.com/a/google.com/document/d/1iNVcrm5xxQf1FdCPwm2VWPoQ2F52...
> > 
> > Nothing specific.  It just seems that as we build these layers on top of
> content
> > settings we should potentially prevent accessing the raw content setting if
> > we want to make them more complicated.
> > 
> > Unfounded fear for sure.

Ah, interesting, not unfounded at all. This is something the permissions team
talks about a lot (e.g. [1]). Ideally nothing would access content settings but
would go via the permissions layer.

Currently most things do go through the permissions layer, except the UI. The UI
goes direct as it needs information not available now through the permissions
layer, like whether the content setting was from policy or not.

[1]
https://docs.google.com/a/google.com/document/d/1JRvBC5_-CDUZi_l-9DdKLvdpN0wT...

[benwells, 2017-01-12 23:26:05]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-01-12 23:26:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
tsergeant@chromium.org
Link to the patchset: https://codereview.chromium.org/2633433002/#ps40001
(title: "Comment")

[commit-bot: I haz the power, 2017-01-12 23:27:17]

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-01-13 01:53:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484263565214370,
"parent_rev": "69c1208ee296bcdf6e789fae22ad29e0daa4ccfd", "commit_rev":
"7b28e26454b336fce8ad4eb8a5dd2597c0750eaa"}

[commit-bot: I haz the power, 2017-01-13 01:54:32]

Description was changed from

==========
Update page info for new search geolocation system.

Geolocation access via the geo API and via the X-Geo header has been
made consistent. As part of this project the UI is being updated to show
the geolocation status for the default search engine in more places.

This change updates the page info popup to show allow / block for the
default search engine.

BUG=674398
==========

to

==========
Update page info for new search geolocation system.

Geolocation access via the geo API and via the X-Geo header has been
made consistent. As part of this project the UI is being updated to show
the geolocation status for the default search engine in more places.

This change updates the page info popup to show allow / block for the
default search engine.

BUG=674398

Review-Url: https://codereview.chromium.org/2633433002
Cr-Commit-Position: refs/heads/master@{#443454}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b28e26454b336fce8ad4eb8a5dd...
==========

[commit-bot: I haz the power, 2017-01-13 01:54:33]

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