Migrate geolocation permissions to the new common permission class.

Migrate geolocation permissions to the new common permission class.

BUG=392145
TBR=benm

Committed: https://crrev.com/fa052070ded331c02f881cbc8332090986bc8f53
Cr-Commit-Position: refs/heads/master@{#297180}

[Miguel Garcia, 2014-08-11 18:30:35]

Initial review.

[Michael van Ouwerkerk, 2014-08-13 10:01:11]

https://codereview.chromium.org/459953002/diff/40001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/459953002/diff/40001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:152: void
CancelGeolocationPermissionRequests(
Looks like this is dead code now. No callers in file. Anonymous namespace. Does
the delegate have other callers of  CancelGeolocationPermissionRequests?

https://codereview.chromium.org/459953002/diff/40001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/459953002/diff/40001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.h:126: const
base::Callback<void(bool)> result_callback) OVERRIDE;
Why const? It's not a reference.

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/apps/web_...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/apps/web_...
chrome/browser/apps/web_view_browsertest.cc:1900:
"web_view/geolocation/embedder_has_permission",
No need for this edit.

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/apps/web_...
chrome/browser/apps/web_view_browsertest.cc:1922:
IN_PROC_BROWSER_TEST_F(WebViewTest, GeolocationAPICancelGeolocation) {
Is this still working as intended?

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1954:
requesting_frame.GetOrigin(), user_gesture,
Can we set a DCHECK_EQ instead:
DCHECK_EQ(requesting_frame, requesting_frame.GetOrigin())

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:86:
->GetContentSettingAndMaybeUpdateLastUsage(
Should probably let dhnish@ know that he gets more data for the Permissions
Manager now :-) Definitely get him to review this.

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_uma_util.cc:31:
PERMISSION_GEOLOCATION = 3,
4?

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (left):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:51: void
RecordUmaEvent(GeolocationInfoBarDelegateEvent event) {
Did you also deprecate the associated histograms?

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:44:
requesting_frame_(requesting_frame),
If we're not checking for origin here, we have to do it elsewhere.

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

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context.cc:45: if
(!requesting_frame_origin.is_valid() || !embedder_origin.is_valid()) {
These checks would be nice for other APIs as well but might break backward
compatibility. Could you file a bug please for further investigation?

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:47: struct
PermissionRequestInfo {
This struct looks generic. Can it be used for other APIs as well? Or should we
just get rid of it?

https://codereview.chromium.org/459953002/diff/40001/chrome/browser/geolocati...
chrome/browser/geolocation/geolocation_permission_context_android.h:72: // Will
perform a final check on the system location settigns before
s/settigns/settings/

https://codereview.chromium.org/459953002/diff/40001/content/public/browser/c...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/459953002/diff/40001/content/public/browser/c...
content/public/browser/content_browser_client.cc:220: const
base::Callback<void(bool)> result_callback) {
Why does this need to be const?

[Miguel Garcia, 2014-08-13 13:18:13]

Thanks! adding owners for the different sections now

benm for webview
bauerb for content_settings.
jam for content

https://chromiumcodereview.appspot.com/459953002/diff/40001/android_webview/b...
File android_webview/browser/aw_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/android_webview/b...
android_webview/browser/aw_content_browser_client.cc:152: void
CancelGeolocationPermissionRequests(
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> Looks like this is dead code now. No callers in file. Anonymous namespace.
Does
> the delegate have other callers of  CancelGeolocationPermissionRequests?

Done.

https://chromiumcodereview.appspot.com/459953002/diff/40001/android_webview/b...
File android_webview/browser/aw_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/android_webview/b...
android_webview/browser/aw_content_browser_client.h:126: const
base::Callback<void(bool)> result_callback) OVERRIDE;
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> Why const? It's not a reference.

This can in fact be a const ref. Done

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ap...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ap...
chrome/browser/apps/web_view_browsertest.cc:1900:
"web_view/geolocation/embedder_has_permission",
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> No need for this edit.

Acknowledged.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ap...
chrome/browser/apps/web_view_browsertest.cc:1922:
IN_PROC_BROWSER_TEST_F(WebViewTest, GeolocationAPICancelGeolocation) {
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> Is this still working as intended?

It is yes, you should be able to cancel a geolocation requst.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ch...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ch...
chrome/browser/chrome_content_browser_client.cc:1954:
requesting_frame.GetOrigin(), user_gesture,
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> Can we set a DCHECK_EQ instead:
> DCHECK_EQ(requesting_frame, requesting_frame.GetOrigin())

Why? The content_browser_client API does not require you to pass an origin.
Other embedders might decide to handle permissions based on the url instead of
the orging. The chrome embedder does expect an origin which is why there is a
DCHECK already inside the PermissionContextBase#RequestPermission method.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/co...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/co...
chrome/browser/content_settings/permission_context_base.cc:86:
->GetContentSettingAndMaybeUpdateLastUsage(
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> Should probably let dhnish@ know that he gets more data for the Permissions
> Manager now :-) Definitely get him to review this.

Acknowledged.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/co...
File chrome/browser/content_settings/permission_context_uma_util.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/co...
chrome/browser/content_settings/permission_context_uma_util.cc:31:
PERMISSION_GEOLOCATION = 3,
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> 4?

Yep, thanks for spotting it.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (left):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:51: void
RecordUmaEvent(GeolocationInfoBarDelegateEvent event) {
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> Did you also deprecate the associated histograms?

Acknowledged.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
File chrome/browser/geolocation/geolocation_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
chrome/browser/geolocation/geolocation_infobar_delegate.cc:44:
requesting_frame_(requesting_frame),
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> If we're not checking for origin here, we have to do it elsewhere.

The check is done in permission_context_base.cc already, at the very beginning
of the flow.

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
chrome/browser/geolocation/geolocation_permission_context.cc:45: if
(!requesting_frame_origin.is_valid() || !embedder_origin.is_valid()) {
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> These checks would be nice for other APIs as well but might break backward
> compatibility. Could you file a bug please for further investigation?

https://code.google.com/p/chromium/issues/detail?id=403291

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
File chrome/browser/geolocation/geolocation_permission_context_android.h
(right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
chrome/browser/geolocation/geolocation_permission_context_android.h:47: struct
PermissionRequestInfo {
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> This struct looks generic. Can it be used for other APIs as well? Or should we
> just get rid of it?
This is only used to pass information during the thread dance that android
geoloc requires. I think it's fine to have it here unless we end up needing the
same dance for other permissions (but right now there is no other global android
setting like geolocation).

https://chromiumcodereview.appspot.com/459953002/diff/40001/chrome/browser/ge...
chrome/browser/geolocation/geolocation_permission_context_android.h:72: // Will
perform a final check on the system location settigns before
On 2014/08/13 10:01:10, Michael van Ouwerkerk wrote:
> s/settigns/settings/

Done.

https://chromiumcodereview.appspot.com/459953002/diff/40001/content/public/br...
File content/public/browser/content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/40001/content/public/br...
content/public/browser/content_browser_client.cc:220: const
base::Callback<void(bool)> result_callback) {
On 2014/08/13 10:01:11, Michael van Ouwerkerk wrote:
> Why does this need to be const?

Const ref. It does not need to but it's just nicer I think and the underlying
permission check infrastructure uses a const ref already.

[Miguel Garcia, 2014-08-13 13:19:29]

Adding Daniel as well since this changes moves the usage auditing to the common
permission class which means that all other permissions (midi, notifications,
push messaging etc.) will be audited as well.

[Bernhard Bauer, 2014-08-13 13:34:32]

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/permission_bubble_request_impl.cc:68: case
CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
Nit: these case statements aren't correctly aligned.

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:57:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Nit: I always forget this myself, but there is DCHECK_CURRENTLY_ON, which will
provide more meaningful error messages on failure.

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:58: if
(shutting_down_)
When is this called during shutdown?

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:121:
base::WeakPtrFactory<PermissionContextBase> weak_factory_;
The WeakPtrFactory should be the last member, to ensure that it will be
destroyed first, which will invalidate weak pointers.

[Miguel Garcia, 2014-08-13 14:25:51]

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
File chrome/browser/content_settings/permission_bubble_request_impl.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
chrome/browser/content_settings/permission_bubble_request_impl.cc:68: case
CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
On 2014/08/13 13:34:32, Bernhard Bauer wrote:
> Nit: these case statements aren't correctly aligned.

Done.

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
chrome/browser/content_settings/permission_context_base.cc:57:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/08/13 13:34:32, Bernhard Bauer wrote:
> Nit: I always forget this myself, but there is DCHECK_CURRENTLY_ON, which will
> provide more meaningful error messages on failure.

Nice!

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
chrome/browser/content_settings/permission_context_base.cc:58: if
(shutting_down_)
On 2014/08/13 13:34:32, Bernhard Bauer wrote:
> When is this called during shutdown?

This can be called by webkit at any time essentially.

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
File chrome/browser/content_settings/permission_context_base.h (right):

https://chromiumcodereview.appspot.com/459953002/diff/80001/chrome/browser/co...
chrome/browser/content_settings/permission_context_base.h:121:
base::WeakPtrFactory<PermissionContextBase> weak_factory_;
On 2014/08/13 13:34:32, Bernhard Bauer wrote:
> The WeakPtrFactory should be the last member, to ensure that it will be
> destroyed first, which will invalidate weak pointers.

Done.

[Miguel Garcia, 2014-08-13 14:27:41]

+mpearson for the histogram change

[Mark P, 2014-08-13 17:29:27]

https://codereview.chromium.org/459953002/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/459953002/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9006: +    Deprecated 8/2014, and
replaced by PermissionActions.
replaced by the histogram XXX.PermissionActions?
replace by the user action PermissionActionsXYZ?
please be clearer.

ditto with other histogram below

[Bernhard Bauer, 2014-08-13 21:05:08]

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/459953002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:58: if
(shutting_down_)
On 2014/08/13 14:25:50, Miguel Garcia wrote:
> On 2014/08/13 13:34:32, Bernhard Bauer wrote:
> > When is this called during shutdown?
> 
> This can be called by webkit at any time essentially.

Well, not really at *any* time... The reason why I'm asking is because this is a
KeyedService, so the profile must exist when this method is called, but the
interval between KeyedService::Shutdown() and profile destruction is actually
very short! Basically, all keyed services are shut down in topological order
specified by their dependencies, then destroyed. During Shutdown(), a
KeyedService should drop all of its references to other keyed services, because
they will go away soon.

That means that if this is called between Shutdown() and destruction, it's
during shutdown of another KeyedService. But a) we know that this call is coming
from Blink, and b) if it was coming from another KeyedService, it would mean
that the other service was not specifying its dependencies correctly (or
otherwise it would be shut down after us).

A shutdown flag is applicable to a RefCountedKeyedService that is used on
another thread (because it might actually outlive the profile), but not in this
case.

https://codereview.chromium.org/459953002/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/459953002/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.cc:57:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Nit: DCHECK_CURRENTLY_ON?

[jam, 2014-08-15 17:32:44]

why are you changing content?

[jam, 2014-08-15 17:33:22]

On 2014/08/15 17:32:44, jam wrote:
> why are you changing content?

and to expand, i see you added a comment to create an interface. the reason
there's a cancel callback there is precisely to avoid adding one or two-method
interfaces in the public API

[Miguel Garcia, 2014-09-17 14:59:10]

On 2014/08/15 17:33:22, jam wrote:
> On 2014/08/15 17:32:44, jam wrote:
> > why are you changing content?
> 
> and to expand, i see you added a comment to create an interface. the reason
> there's a cancel callback there is precisely to avoid adding one or two-method
> interfaces in the public API

Resurrecting this patch now, I apologize for the delay, now that my vacation and
perf iteration 1 are mostly out of the way I will be much more proactive.

Bernhard and I discussed offline the need of adding the extra logic to shut down
services and agreed we don't need it so I have removed it.

jam@ I really would like to avoid the callback for cancelling a request if
possible, it makes the code harder to understand and makes the author of every
permission repeat the same pattern of collecting and maintaining an array of
cancel callbacks just in case they need to be cancelled.

I understand you want to keep the content interface as small as possible so I
propose removing the CreateXXPermissionRequest methods instead and keep a single
RequestPermission(PermissionEnum type) and CancelPermission(PermissionEnum
type). I have prototyped this in https://codereview.chromium.org/571483002 where
as you can see the net amount of methods is -1 even when I add the
CancelPermission one. WDYT?

[Bernhard Bauer, 2014-09-19 09:04:24]

content_settings/ LGTM

https://codereview.chromium.org/459953002/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_permission_helper.cc
(right):

https://codereview.chromium.org/459953002/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_permission_helper.cc:183:
web_view_permission_helper_delegate_-> RequestMediaAccessPermission(
No space after "->"

[jam, 2014-09-22 15:07:08]

On 2014/09/17 14:59:10, Miguel Garcia wrote:
> On 2014/08/15 17:33:22, jam wrote:
> > On 2014/08/15 17:32:44, jam wrote:
> > > why are you changing content?
> > 
> > and to expand, i see you added a comment to create an interface. the reason
> > there's a cancel callback there is precisely to avoid adding one or
two-method
> > interfaces in the public API
> 
> Resurrecting this patch now, I apologize for the delay, now that my vacation
and
> perf iteration 1 are mostly out of the way I will be much more proactive.
> 
> Bernhard and I discussed offline the need of adding the extra logic to shut
down
> services and agreed we don't need it so I have removed it.
> 
> jam@ I really would like to avoid the callback for cancelling a request if
> possible, it makes the code harder to understand and makes the author of every
> permission repeat the same pattern of collecting and maintaining an array of
> cancel callbacks just in case they need to be cancelled.
> 
> I understand you want to keep the content interface as small as possible so I
> propose removing the CreateXXPermissionRequest methods instead and keep a
single
> RequestPermission(PermissionEnum type) and CancelPermission(PermissionEnum
> type). I have prototyped this in https://codereview.chromium.org/571483002
where
> as you can see the net amount of methods is -1 even when I add the
> CancelPermission one. WDYT?

ok, if you'll merge all the methods into one in a followup patch, lgtm

[Michael van Ouwerkerk, 2014-09-22 17:02:56]

lgtm with nits

https://chromiumcodereview.appspot.com/459953002/diff/180001/chrome/browser/g...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/180001/chrome/browser/g...
chrome/browser/geolocation/geolocation_permission_context.cc:30:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
DCHECK_CURRENTLY_ON

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:43358: <enum
name="GeolocationInfoBarDelegateEvent" type="int">
Please also deprecate the GeolocationInfoBarDelegateAndroidEvent enum

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:9251: +    Deprecated 8/2014, and
replaced by PermissionActions.
9/2014

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:43366: +    Deprecated 8/2014, and
replaced by PermissionAction.
9/2014

[Miguel Garcia, 2014-09-24 11:04:44]

Histograms and nits fixed. PTAL

https://chromiumcodereview.appspot.com/459953002/diff/100001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/459953002/diff/100001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:9006: +    Deprecated 8/2014, and
replaced by PermissionActions.
On 2014/08/13 17:29:27, Mark P wrote:
> replaced by the histogram XXX.PermissionActions?
> replace by the user action PermissionActionsXYZ?
> please be clearer.
> 
> ditto with other histogram below

Done.

https://chromiumcodereview.appspot.com/459953002/diff/180001/chrome/browser/g...
File chrome/browser/geolocation/geolocation_permission_context.cc (right):

https://chromiumcodereview.appspot.com/459953002/diff/180001/chrome/browser/g...
chrome/browser/geolocation/geolocation_permission_context.cc:30:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2014/09/22 17:02:56, Michael van Ouwerkerk wrote:
> DCHECK_CURRENTLY_ON

Done.

https://chromiumcodereview.appspot.com/459953002/diff/180001/extensions/brows...
File extensions/browser/guest_view/web_view/web_view_permission_helper.cc
(right):

https://chromiumcodereview.appspot.com/459953002/diff/180001/extensions/brows...
extensions/browser/guest_view/web_view/web_view_permission_helper.cc:183:
web_view_permission_helper_delegate_-> RequestMediaAccessPermission(
On 2014/09/19 09:04:24, Bernhard Bauer wrote:
> No space after "->"

I reverted this file since it did not belong to this change.

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:43358: <enum
name="GeolocationInfoBarDelegateEvent" type="int">
On 2014/09/22 17:02:56, Michael van Ouwerkerk wrote:
> Please also deprecate the GeolocationInfoBarDelegateAndroidEvent enum

Done.

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:9251: +    Deprecated 8/2014, and
replaced by PermissionActions.
On 2014/09/22 17:02:56, Michael van Ouwerkerk wrote:
> 9/2014

Done.

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:9251: +    Deprecated 8/2014, and
replaced by PermissionActions.
On 2014/09/22 17:02:56, Michael van Ouwerkerk wrote:
> 9/2014

Done.

https://chromiumcodereview.appspot.com/459953002/diff/180001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:43366: +    Deprecated 8/2014, and
replaced by PermissionAction.
On 2014/09/22 17:02:56, Michael van Ouwerkerk wrote:
> 9/2014

Done.

[Mark P, 2014-09-24 17:59:38]

histograms.xml will lgtm once you fix the issue (in two places) below

--mark

https://chromiumcodereview.appspot.com/459953002/diff/220001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/459953002/diff/220001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:9277: +   
ContentSettingsPermissionActions_Geolocation.
missing "." between Settings and Permission

ditto below

[Miguel Garcia, 2014-09-29 14:24:58]

Thanks! landing this now

TBR=benm since the change there is a straight change for the new API signature.

https://chromiumcodereview.appspot.com/459953002/diff/220001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/459953002/diff/220001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:9277: +   
ContentSettingsPermissionActions_Geolocation.
On 2014/09/24 17:59:38, Mark P wrote:
> missing "." between Settings and Permission
> 
> ditto below

Done.

[Miguel Garcia, 2014-09-29 14:25:38]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-09-29 14:26:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/459953002/240001

[commit-bot: I haz the power, 2014-09-29 15:31:46]

Committed patchset #13 (id:240001) as 87d634675a409ee085a3833cef338fa1fee7b903

[commit-bot: I haz the power, 2014-09-29 15:32:34]

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

[lgarron, 2015-04-03 23:07:23]

lgarron@chromium.org changed reviewers:
+ lgarron@chromium.org

[lgarron, 2015-04-03 23:07:25]

https://codereview.chromium.org/459953002/diff/240001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/459953002/diff/240001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9346: +   
ContentSettings.PermissionActions_Geolocation.
Should I be able to find ContentSettings.PermissionActions_Geolocation in
histograms.xml?

[Miguel Garcia, 2015-04-06 09:05:43]

https://codereview.chromium.org/459953002/diff/240001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/459953002/diff/240001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9346: +   
ContentSettings.PermissionActions_Geolocation.
On 2015/04/03 23:07:24, lgarron wrote:
> Should I be able to find ContentSettings.PermissionActions_Geolocation in
> histograms.xml?

Yes, look for <histogram_suffixes name="PermissionActions"> around
https://cs.corp.google.com/#clankium/src/tools/metrics/histograms/histograms....
(takes a while to load). The histogram is defined as a stem and a set of
suffixes (one per permission).