Location chooser

The user can specify the granularity/precision of the location shared with websites.

Currently the user can either allow/deny any location sharing request. This patch aim to empower users by giving them the ability to share different precision levels of their location in four different levels:
* Street address.
* City only.
* State only.
* Country only.

This is only designed to be part of the user flow when geolocation is requested by a website. Also, since we are moving from infoBars to bubbles, the implementation is based on top of the permission bubbles UI.

BUG=401700
R=meacer, jww

[mhm, 2014-08-09 01:36:40]

Mustafa ..

This is the UI part of the location chooser. It is a little hacky-esh but it
does the job for a PoC.

[meacer, 2014-08-11 18:15:37]

On 2014/08/09 01:36:40, mhm wrote:
> Mustafa ..
> 
> This is the UI part of the location chooser. It is a little hacky-esh but it
> does the job for a PoC.

Looks great! I realize it's not ready for review yet, but added some quick
comments.

I think you aren't handling the case where there are more than one permission
request, but that's perfectly fine for the POC.

[meacer, 2014-08-11 18:15:50]

(forgot to send the comments)

https://codereview.chromium.org/454253002/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/454253002/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:175:
INDEX_ALLOW = 0,
I think you'll want to rename this to EXACT or something similar, rather than
ALLOW given that the geolocation permission request already has a separate allow
boolean. EXACT would give the exact location returned by the API, but it'd still
be gated by the bool.

https://codereview.chromium.org/454253002/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:194: if
(index == INDEX_ALLOW)
Might want to use switch here.

[mhm, 2014-08-13 20:48:22]

Added the part that communicate the user choice all the way to the location
provider. Still trying to fix the issue with the infoBar case.

https://codereview.chromium.org/454253002/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/454253002/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:175:
INDEX_ALLOW = 0,
On 2014/08/11 18:15:50, Mustafa Emre Acer wrote:
> I think you'll want to rename this to EXACT or something similar, rather than
> ALLOW given that the geolocation permission request already has a separate
allow
> boolean. EXACT would give the exact location returned by the API, but it'd
still
> be gated by the bool.

Done.

https://codereview.chromium.org/454253002/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:194: if
(index == INDEX_ALLOW)
On 2014/08/11 18:15:50, Mustafa Emre Acer wrote:
> Might want to use switch here.

Done.

[mhm, 2014-08-15 21:45:35]

meacer@, can you please have a quick look at this last patch for me.

[meacer, 2014-08-15 22:20:25]

Looks good. I'm not sure if you are planning to land this, if so you'll
definitely want to have some tests.

https://codereview.chromium.org/454253002/diff/50001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/454253002/diff/50001/chrome/app/generated_res...
chrome/app/generated_resources.grd:432: +      <message
name="IDS_PERMISSION_CITY" desc="Label on text button to allow the user to share
his city geolocation.">
nit: his -> their

https://codereview.chromium.org/454253002/diff/50001/chrome/app/generated_res...
chrome/app/generated_resources.grd:435: +      <message
name="IDS_PERMISSION_STATE" desc="Label on text button to allow the user to
share his state/province geolocation.">
Same here and below

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:50: void
PermissionGranted(int choice);
nit: Maybe you might want to rename this to something that implies precision.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:118: // (mhm) Fix
content setting to account for different choices.
nit: TODO(mohammed) since that's your Chromium username.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:236: // (mhm) fix.
nit: TODO(mohammed): ...

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:265:
InfoBarRequestCallback));
Does passing base::Bind(callback, -1) directly not work here? If not, you should
rename InfoBarRequestCallback to infobar_request_callback

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:319:
NotifyPermissionSet(id, requesting_frame, callback, -1, allowed);
nit: You can add a constant such as kUnknownPrecision=-1 and pass the constant
by name here.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:336: allowed);
This is fine, but do you need a TODO here, as choice doesn't seem to be passed
to content_settings?

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:191: //
(mhm) test choice.
Nit: TODO(mohammed)

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:174: enum
Item {
nit: This could use a better name, such as GeolocationPrecision.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:190: return
4;
Add an INDEX_COUNT to the enum and return it from here.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:370: if
(requests[0]->GetType() == PermissionBubbleRequest::Type::kGeolocation) {
nit: You might want to add a comment as to why geolocation gets special
treatment here.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:467: } else
if (combobox == geolocation_combobox_)
Braces around if block since the previous block has it.

https://codereview.chromium.org/454253002/diff/50001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/454253002/diff/50001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:144:
geoposition.ApplyPrecision(geolocationPrecision_)));
Since this is creating a new geoposition object, you should consider making it
renaming it and making it static. E.g. static GeoPosition
GetFilteredGeoPosition(const GeoPosition& position, int precision)

https://codereview.chromium.org/454253002/diff/50001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:256:
geolocationPrecision_ = choice;
Ah, I think I see why you don't use precision instead of choice, as other
permission bubbles may use it with a different meaning.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
File content/public/common/geoposition.cc (right):

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.cc:39: Geoposition
Geoposition::ApplyPrecision(const int choice) const {
In fact, it looks like  Since this method returns a new GeoPosition object,
you'll want to reflect that.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.cc:51: newGeoposition.latitude =
(int)newGeoposition.latitude;
May be better: static_cast<int>(std::floor(newGeoposition.latitute)) (with
#include <cmath>)

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.cc:57: newGeoposition.longitude =
(int)(newGeoposition.longitude / 10.0);
You might want to add that this is a POC, and the calculations aren't accurate.
Also, you'll want to put this whole feature behind a flag if you want to land
it.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
File content/public/common/geoposition.h (right):

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.h:39: Geoposition ApplyPrecision(const int
choice) const;
As I said earlier, this would be better as a static method that takes a
Geoposition and choice, and returns a new Geoposition object.

[Michael van Ouwerkerk, 2014-08-15 23:05:43]

If you intend to submit this patch, I could take a look at it. Also, I'm
currently refactoring geolocation, but you don't seem to be changing the content
layer much so no worries there.

[mhm, 2014-08-15 23:18:57]

https://codereview.chromium.org/454253002/diff/50001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/454253002/diff/50001/chrome/app/generated_res...
chrome/app/generated_resources.grd:432: +      <message
name="IDS_PERMISSION_CITY" desc="Label on text button to allow the user to share
his city geolocation.">
On 2014/08/15 22:20:24, Mustafa Emre Acer wrote:
> nit: his -> their

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/app/generated_res...
chrome/app/generated_resources.grd:435: +      <message
name="IDS_PERMISSION_STATE" desc="Label on text button to allow the user to
share his state/province geolocation.">
On 2014/08/15 22:20:24, Mustafa Emre Acer wrote:
> Same here and below

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:50: void
PermissionGranted(int choice);
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> nit: Maybe you might want to rename this to something that implies precision.

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:118: // (mhm) Fix
content setting to account for different choices.
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> nit: TODO(mohammed) since that's your Chromium username.

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:236: // (mhm) fix.
On 2014/08/15 22:20:24, Mustafa Emre Acer wrote:
> nit: TODO(mohammed): ...

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:265:
InfoBarRequestCallback));
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> Does passing base::Bind(callback, -1) directly not work here? If not, you
should
> rename InfoBarRequestCallback to infobar_request_callback

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:319:
NotifyPermissionSet(id, requesting_frame, callback, -1, allowed);
On 2014/08/15 22:20:24, Mustafa Emre Acer wrote:
> nit: You can add a constant such as kUnknownPrecision=-1 and pass the constant
> by name here.

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:336: allowed);
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> This is fine, but do you need a TODO here, as choice doesn't seem to be passed
> to content_settings?

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:191: //
(mhm) test choice.
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> Nit: TODO(mohammed)

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:174: enum
Item {
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> nit: This could use a better name, such as GeolocationPrecision.

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:190: return
4;
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> Add an INDEX_COUNT to the enum and return it from here.

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:370: if
(requests[0]->GetType() == PermissionBubbleRequest::Type::kGeolocation) {
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> nit: You might want to add a comment as to why geolocation gets special
> treatment here.

Done.

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:467: } else
if (combobox == geolocation_combobox_)
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> Braces around if block since the previous block has it.

Done.

https://codereview.chromium.org/454253002/diff/50001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/454253002/diff/50001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:144:
geoposition.ApplyPrecision(geolocationPrecision_)));
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> Since this is creating a new geoposition object, you should consider making it
> renaming it and making it static. E.g. static GeoPosition
> GetFilteredGeoPosition(const GeoPosition& position, int precision)

Done.

https://codereview.chromium.org/454253002/diff/50001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:256:
geolocationPrecision_ = choice;
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> Ah, I think I see why you don't use precision instead of choice, as other
> permission bubbles may use it with a different meaning.

Done.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
File content/public/common/geoposition.cc (right):

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.cc:39: Geoposition
Geoposition::ApplyPrecision(const int choice) const {
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> In fact, it looks like  Since this method returns a new GeoPosition object,
> you'll want to reflect that. 

Done.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.cc:51: newGeoposition.latitude =
(int)newGeoposition.latitude;
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> May be better: static_cast<int>(std::floor(newGeoposition.latitute)) (with
> #include <cmath>)

Done.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.cc:57: newGeoposition.longitude =
(int)(newGeoposition.longitude / 10.0);
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> You might want to add that this is a POC, and the calculations aren't
accurate.
> Also, you'll want to put this whole feature behind a flag if you want to land
> it.

Done.

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
File content/public/common/geoposition.h (right):

https://codereview.chromium.org/454253002/diff/50001/content/public/common/ge...
content/public/common/geoposition.h:39: Geoposition ApplyPrecision(const int
choice) const;
On 2014/08/15 22:20:25, Mustafa Emre Acer wrote:
> As I said earlier, this would be better as a static method that takes a
> Geoposition and choice, and returns a new Geoposition object.

Done.

[felt, 2014-08-15 23:20:16]

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/454253002/diff/50001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:236: // (mhm) fix.
On 2014/08/15 23:18:56, mhm wrote:
> On 2014/08/15 22:20:24, Mustafa Emre Acer wrote:
> > nit: TODO(mohammed): ...
> 
> Done.

No TODOs for interns who are about to leave. :) This should be assigned to an
engineer, with an accompanying crbug number for the TODO.

[meacer, 2014-08-15 23:34:46]

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:333: //
TODO(mohamed) the geolocation precision need to be passed to content
mohamed -> mohammed :) (or the engineer who'll take ownership as felt said)

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:339:
callback.Run(allowed, choice);
This doesn't look right. The callback takes (int, bool) but you are passing
(allowed, choice)?

It would be better to make all callbacks Callback<void(bool,int)> and then pass
allowed, choice in that order, everywhere.

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_unittest.cc
(right):

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_unittest.cc:191: //
(mohammed) test choice.
nit: TODO(mohammed):

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/454253002/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:373: // the
ability to specifiy the precision of the shared location.
specifiy -> specify

https://codereview.chromium.org/454253002/diff/70001/content/public/common/ge...
File content/public/common/geoposition.h (right):

https://codereview.chromium.org/454253002/diff/70001/content/public/common/ge...
content/public/common/geoposition.h:40: const int precisions);
precisions -> precision