Group the different permission related methods in the content api.

Group the different permission related methods in the content api.

BUG=392145
Committed: https://crrev.com/b7beb08c377e86a8b9b224a5064a2cf0b737a679
Cr-Commit-Position: refs/heads/master@{#301338}

[Miguel Garcia, 2014-10-07 15:16:10]

Patchset #1 (id:1) has been deleted

[Miguel Garcia, 2014-10-07 17:24:26]

Patchset #1 (id:20001) has been deleted

[Miguel Garcia, 2014-10-20 14:33:19]

miguelg@chromium.org changed reviewers:
+ bauerb@chromium.org, jam@chromium.org, peter@chromium.org

[Miguel Garcia, 2014-10-20 14:34:34]

jam@ this is a follow up to https://codereview.chromium.org/459953002 where I
promised I would merge the permission related methods. Could you have a look for
the content interfaces?

peter@ to review the notification related changes

bauerb@ to review the changes in content_settings

[Bernhard Bauer, 2014-10-20 14:57:24]

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

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:409: break;
Do you want to fall through here?

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:412: << permission;
Nit: Align << signs.

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:415: if (delegate == NULL)
{
Nit: Just !delegate

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:432: LOG(WARNING) <<
"Permission not available " << permission;
Is there a difference between "Permission not available" and "Permission request
not implemented"?

Also, if you explicitly list the remaining enum values, the compiler will
complain if someone should add another value and forget updating this place.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1913:
->RequestNotificationPermission(
At the very least, indent this four more spaces, as it is a continuation of the
previous line. Personally, I'm not a fan of seeing a lone -> though; Maybe you
could extract the notification service into a local variable?

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1920: NOTIMPLEMENTED();
Fix alignment.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/notifica...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/notifica...
chrome/browser/notifications/desktop_notification_service.cc:177:
RequestPermission(
You could now probably fit more of the parameters onto this line.

https://codereview.chromium.org/622793002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1004: // TODO(peter) plumb
user_gesture and bridge_id.
Nit: Standard syntax for TODOs has a colon after the parentheses (to make sure
it's caught by searches).

https://codereview.chromium.org/622793002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:450: int request_id, bool
granted);
Does this fit on one line? If not, at least put the first parameter on the
previous line and align the second one.

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:262: true,
You could extract this to a local variable and put the comment before it.

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:112: // Enum for UMA purposes,
make sure you update histograms.xml if you add new
This enum is now for more than just UMA.

[Peter Beverloo, 2014-10-20 17:32:16]

First round of comments. Please run try-bots (including Blink layout ones!)
since this will break things.

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

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:413: break;
Can we just handle these cases in the |default| clause of this switch, rather
than doing the fall-through trampoline? I also think we can be more strict,
since we shouldn't trigger a permission request at all if we know a feature
isn't going to be available.

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:432: LOG(WARNING) <<
"Permission not available " << permission;
nit: break; after this line for consistency.

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:432: LOG(WARNING) <<
"Permission not available " << permission;
On 2014/10/20 14:57:23, Bernhard Bauer wrote:
> Is there a difference between "Permission not available" and "Permission
request
> not implemented"?

+1 to merging them.

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:446: if (!delegate)
This check should be done in AwContentBrowserClient::RequestPermission as well.

Assuming we have a WebContents*, there should be an AwContents instance as well
(which is what AwBrowserPermissionRequestDelegate::FromID returns). Can we
DCHECK instead of doing an if-clause here?

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

https://codereview.chromium.org/622793002/diff/100001/android_webview/browser...
android_webview/browser/aw_content_browser_client.h:128: virtual void
CancelPermissionRequest(
Do you also plan to have a common function for checking whether permission has
been granted for a certain origin or origin+embedder pair?

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1897:
web_contents->GetBrowserContext());
  Profile* profile =
      Profile::FromBrowserContext(web_contents->GetBrowserContext());

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1929: #if defined(OS_ANDROID)
Either place the #if inside of the case-statement, or move the one for
Notifications on 1911 outside of it as well.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1938: LOG(WARNING) <<
"Permission not available " << permission;
nit: break for consistency.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1959:
MidiPermissionContextFactory::GetForProfile(profile)
Fix indentation.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1993: case
content::PERMISSION_MIDI_SYSEX:
Fix indentation.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:203: const
base::Callback<void(bool)>& result_callback) override;
5-space indent??

Also, please remove the blank lines on lines 204, 210 and 216, since this is a
list of overrides.

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/notifica...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/622793002/diff/100001/chrome/browser/notifica...
chrome/browser/notifications/desktop_notification_service.h:59:
NotificationPermissionCallback;
Remove this |typedef| if we're not going to use it anymore.

https://codereview.chromium.org/622793002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1004: // TODO(peter) plumb
user_gesture and bridge_id.
What is a |bridge_id|? The renderer part of notifications does not have one.

https://codereview.chromium.org/622793002/diff/100001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_content_browser_client.cc
(left):

https://codereview.chromium.org/622793002/diff/100001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_content_browser_client.cc:22:
manager->RequestPermission(source_origin, callback);
Removing this is not OK and will break tests.

In order to properly test Web Exposed behavior, layout tests need a way to
control whether permission is granted or not. This code is part of that (there
is more, but c/s/b/ is being refactored quite heavily right now). It defers the
RequestPermission call to a "fake" LayoutTestNotificationManager instead.

[jam, 2014-10-20 20:13:57]

lgtm

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/m...
File content/browser/media/midi_dispatcher_host.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/m...
content/browser/media/midi_dispatcher_host.cc:101:
content::PERMISSION_MIDI_SYSEX,
nit: here and above, and elsewhere, no "content::"

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:115: enum PermissionType {
this should go into a separate file in content/public/browser per
http://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:116: PERMISSION_UNKNOWN = 0,
why is unknown needed?

[Miguel Garcia, 2014-10-21 17:17:13]

Thanks for the reviews

I have ran git cl format now, so all the indentation stuff should have been
magically fixed (I had a look at all comments to double check though)

blink tests have also been fixed now, I will need a three sided patch for one of
them (tracked in crbug/425537) and will run the bot manually before submitting
after blink rolls

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:409: break;
On 2014/10/20 14:57:23, Bernhard Bauer wrote:
> Do you want to fall through here?

Acknowledged.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:412: << permission;
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> Nit: Align << signs.

Acknowledged.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:413: break;
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Can we just handle these cases in the |default| clause of this switch, rather
> than doing the fall-through trampoline? I also think we can be more strict,
> since we shouldn't trigger a permission request at all if we know a feature
> isn't going to be available.

Acknowledged.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:415: if (delegate == NULL)
{
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> Nit: Just !delegate

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:432: LOG(WARNING) <<
"Permission not available " << permission;
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> nit: break; after this line for consistency.

Acknowledged.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:432: LOG(WARNING) <<
"Permission not available " << permission;
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> On 2014/10/20 14:57:23, Bernhard Bauer wrote:
> > Is there a difference between "Permission not available" and "Permission
> request
> > not implemented"?
> 
> +1 to merging them.

Acknowledged.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:432: LOG(WARNING) <<
"Permission not available " << permission;
I merged them and explicitly listed all values as requested.

On 2014/10/20 14:57:23, Bernhard Bauer wrote:
> Is there a difference between "Permission not available" and "Permission
request
> not implemented"?
> 
> Also, if you explicitly list the remaining enum values, the compiler will
> complain if someone should add another value and forget updating this place.

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.cc:446: if (!delegate)
Actually in RequestPermission there are already checks for the delegate and the
behaviour is different since the callback is run and a Logging statement is
printed. It seems a bit out of scope to try to change this in the same cl.



On 2014/10/20 17:32:16, Peter Beverloo wrote:
> This check should be done in AwContentBrowserClient::RequestPermission as
well.
> 
> Assuming we have a WebContents*, there should be an AwContents instance as
well
> (which is what AwBrowserPermissionRequestDelegate::FromID returns). Can we
> DCHECK instead of doing an if-clause here?

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
File android_webview/browser/aw_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/android_webview/...
android_webview/browser/aw_content_browser_client.h:128: virtual void
CancelPermissionRequest(
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Do you also plan to have a common function for checking whether permission has
> been granted for a certain origin or origin+embedder pair?

Not in this patch certainly. But if it's considered useful I can add it.
Otherwise we can wait and see how the infrastructure for the permission API
evolves for that. If we get the mojo service going there is not much sense to
keep updating the content API.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1897:
web_contents->GetBrowserContext());
On 2014/10/20 17:32:16, Peter Beverloo wrote:
>   Profile* profile =
>       Profile::FromBrowserContext(web_contents->GetBrowserContext());

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1913:
->RequestNotificationPermission(
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> At the very least, indent this four more spaces, as it is a continuation of
the
> previous line. Personally, I'm not a fan of seeing a lone -> though; Maybe you
> could extract the notification service into a local variable?

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1920: NOTIMPLEMENTED();
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> Fix alignment.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1929: #if defined(OS_ANDROID)
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Either place the #if inside of the case-statement, or move the one for
> Notifications on 1911 outside of it as well.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1938: LOG(WARNING) <<
"Permission not available " << permission;
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> nit: break for consistency.

Not sure where do you want to see the break

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1959:
MidiPermissionContextFactory::GetForProfile(profile)
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Fix indentation.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.cc:1993: case
content::PERMISSION_MIDI_SYSEX:
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Fix indentation.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
File chrome/browser/chrome_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/c...
chrome/browser/chrome_content_browser_client.h:203: const
base::Callback<void(bool)>& result_callback) override;
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> 5-space indent??
> 
> Also, please remove the blank lines on lines 204, 210 and 216, since this is a
> list of overrides.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/n...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/n...
chrome/browser/notifications/desktop_notification_service.cc:177:
RequestPermission(
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> You could now probably fit more of the parameters onto this line.

Done. But git cl format does not like it and I rather not fight with it.

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/n...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/chrome/browser/n...
chrome/browser/notifications/desktop_notification_service.h:59:
NotificationPermissionCallback;
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Remove this |typedef| if we're not going to use it anymore.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1004: // TODO(peter) plumb
user_gesture and bridge_id.
On 2014/10/20 17:32:16, Peter Beverloo wrote:
> What is a |bridge_id|? The renderer part of notifications does not have one.

I asked Takashi about it a while ago as well. Quoting his reply here

"The bridge_id is an unique ID in the renderer. This is needed because a page
may have multiple clients that may have different permissions, and the id is
used to identify each client."


In practice all other permissions use the routing ID of the associated
RenderFrame.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:450: int request_id, bool
granted);
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> Does this fit on one line? If not, at least put the first parameter on the
> previous line and align the second one.

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/browser/...
File content/browser/media/midi_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/browser/...
content/browser/media/midi_dispatcher_host.cc:101:
content::PERMISSION_MIDI_SYSEX,
On 2014/10/20 20:13:57, jam wrote:
> nit: here and above, and elsewhere, no "content::"

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/public/b...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/public/b...
content/public/browser/content_browser_client.h:112: // Enum for UMA purposes,
make sure you update histograms.xml if you add new
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> This enum is now for more than just UMA.

Acknowledged.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/public/b...
content/public/browser/content_browser_client.h:115: enum PermissionType {
On 2014/10/20 20:13:57, jam wrote:
> this should go into a separate file in content/public/browser per
> http://www.chromium.org/developers/content-module/content-api

Done.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/public/b...
content/public/browser/content_browser_client.h:116: PERMISSION_UNKNOWN = 0,
On 2014/10/20 20:13:57, jam wrote:
> why is unknown needed?

It is not really needed but UMA expects it so I'd rather leave it there.

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/shell/br...
File content/shell/browser/layout_test/layout_test_content_browser_client.cc
(left):

https://chromiumcodereview.appspot.com/622793002/diff/100001/content/shell/br...
content/shell/browser/layout_test/layout_test_content_browser_client.cc:22:
manager->RequestPermission(source_origin, callback);
Fixed now

On 2014/10/20 17:32:16, Peter Beverloo wrote:
> Removing this is not OK and will break tests.
> 
> In order to properly test Web Exposed behavior, layout tests need a way to
> control whether permission is granted or not. This code is part of that (there
> is more, but c/s/b/ is being refactored quite heavily right now). It defers
the
> RequestPermission call to a "fake" LayoutTestNotificationManager instead.

[Bernhard Bauer, 2014-10-23 08:38:01]

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:262: true,
On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> You could extract this to a local variable and put the comment before it.

^^^

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:116: PERMISSION_UNKNOWN = 0,
On 2014/10/21 17:17:12, Miguel Garcia wrote:
> On 2014/10/20 20:13:57, jam wrote:
> > why is unknown needed?
> 
> It is not really needed but UMA expects it so I'd rather leave it there.

Why does UMA expect it? The only reference I can find is in
chrome/browser/content_settings/permission_context_uma_util.cc as a fallback
value for an unknown content setting, and we could simply not record anything in
that case (which should not be reached anyway, so there's no reason to handle it
in production code).

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

https://codereview.chromium.org/622793002/diff/160001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:428: case
content::PERMISSION_PUSH_MESSAGING:
Nit: these three permissions are valid, but not implemented. The two values
below aren't actually valid, so it would be a bit more appropriate to put a
NOTREACHED() there.

https://codereview.chromium.org/622793002/diff/160001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:460: default:
Can you also write out the other values here, and in other files below?

https://codereview.chromium.org/622793002/diff/160001/content/public/browser/...
File content/public/browser/permission_type.h (right):

https://codereview.chromium.org/622793002/diff/160001/content/public/browser/...
content/public/browser/permission_type.h:10: // the UMAa guidelines.
Nit: UMA

[Miguel Garcia, 2014-10-23 12:48:12]

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:262: true,
On 2014/10/23 08:38:01, Bernhard Bauer wrote:
> On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> > You could extract this to a local variable and put the comment before it.
> 
> ^^^

I thought it was just a suggestion and decided not to go with it :)

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:116: PERMISSION_UNKNOWN = 0,
Bernhard explained that the one exception to the rule of "do not remove values"
is entries at the beginning of the range so this can safely be removed.

On 2014/10/23 08:38:01, Bernhard Bauer wrote:
> On 2014/10/21 17:17:12, Miguel Garcia wrote:
> > On 2014/10/20 20:13:57, jam wrote:
> > > why is unknown needed?
> > 
> > It is not really needed but UMA expects it so I'd rather leave it there.
> 
> Why does UMA expect it? The only reference I can find is in
> chrome/browser/content_settings/permission_context_uma_util.cc as a fallback
> value for an unknown content setting, and we could simply not record anything
in
> that case (which should not be reached anyway, so there's no reason to handle
it
> in production code).

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

https://codereview.chromium.org/622793002/diff/160001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:428: case
content::PERMISSION_PUSH_MESSAGING:
On 2014/10/23 08:38:01, Bernhard Bauer wrote:
> Nit: these three permissions are valid, but not implemented. The two values
> below aren't actually valid, so it would be a bit more appropriate to put a
> NOTREACHED() there.

Done.

https://codereview.chromium.org/622793002/diff/160001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:460: default:
On 2014/10/23 08:38:01, Bernhard Bauer wrote:
> Can you also write out the other values here, and in other files below?

Done.

https://codereview.chromium.org/622793002/diff/160001/content/public/browser/...
File content/public/browser/permission_type.h (right):

https://codereview.chromium.org/622793002/diff/160001/content/public/browser/...
content/public/browser/permission_type.h:10: // the UMAa guidelines.
On 2014/10/23 08:38:01, Bernhard Bauer wrote:
> Nit: UMA

Done.

[Bernhard Bauer, 2014-10-23 12:54:53]

LGTM!

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/622793002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:262: true,
On 2014/10/23 12:48:11, Miguel Garcia wrote:
> On 2014/10/23 08:38:01, Bernhard Bauer wrote:
> > On 2014/10/20 14:57:24, Bernhard Bauer wrote:
> > > You could extract this to a local variable and put the comment before it.
> > 
> > ^^^
> 
> I thought it was just a suggestion and decided not to go with it :)

Fair enough. I wasn't sure whether you had seen it or not.

[Peter Beverloo, 2014-10-24 14:37:28]

lgtm % the following comments

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1945: // so there is no reason
to implement it here.
This comment would be more useful if it would briefly explain *why* Push
Messaging doesn't require this flow.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1947: << permission;
nit: "break;" as the last statement in this case.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1949: NOTREACHED() << "Invalid
RequestPermission for " << permission;
nit: "break;" as the last statement in this case.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:172: virtual void
SelectClientCertificate(
s/virtual//.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/notifica...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/notifica...
chrome/browser/notifications/desktop_notification_service.cc:181:
result_callback);
Please remove the WeakPtrFactory from the DesktopNotificationService now that
we're no longer using it.

https://codereview.chromium.org/622793002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/622793002/diff/180001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1009: -1 /* bridge id */,
If all consumers indeed use the routing Id, change this -1 to |routing_id_| and
remove that bit of the TODO. I see no reason to derive. I'll address the
user_gesture plumbing after this lands.

https://codereview.chromium.org/622793002/diff/180001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1011: true /* user_gesture
*/,
Please use C++-style comments:

true,  // user_gesture

https://codereview.chromium.org/622793002/diff/180001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/622793002/diff/180001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:258: 0 /* bridge id */,
Please use c++ style comments:

0,  // bridge_id

https://codereview.chromium.org/622793002/diff/180001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:262: true,
true,  // Only implemented for Android infobars which do not support
       // user gestures.

(also fixes capitalization and spelling)

https://codereview.chromium.org/622793002/diff/180001/content/public/browser/...
File content/public/browser/permission_type.h (right):

https://codereview.chromium.org/622793002/diff/180001/content/public/browser/...
content/public/browser/permission_type.h:3: // found in the LICENSE file.
nit: empty line under this one.

https://codereview.chromium.org/622793002/diff/180001/content/public/browser/...
content/public/browser/permission_type.h:25: }  // namespace content
nit: empty line under this one.

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_content_browser_client.cc
(right):

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_content_browser_client.cc:82:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Lift this DCHECK up one level higher please. It'd be bad if we were to invoke
RequestPermission on different threads depending on |permission|.

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:27:
(CheckPermission(origin) == blink::WebNotificationPermissionAllowed));
Why the extra parenthesis here? Can we use a local instead?

bool allowed =
    CheckPermission(origin) == blink::WebNotificationPermissionAllowed;
callback.Run(allowed);

[Miguel Garcia, 2014-10-24 15:06:13]

miguelg@chromium.org changed reviewers:
+ benm@chromium.org, mukai@chromium.org

[Miguel Garcia, 2014-10-24 15:06:40]

+mukai and benm for remaining owners rubberstamp

Thanks!

[Miguel Garcia, 2014-10-24 16:05:33]

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1945: // so there is no reason
to implement it here.
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> This comment would be more useful if it would briefly explain *why* Push
> Messaging doesn't require this flow.

Done.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1947: << permission;
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> nit: "break;" as the last statement in this case.

Done.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1949: NOTREACHED() << "Invalid
RequestPermission for " << permission;
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> nit: "break;" as the last statement in this case.

Done.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:172: virtual void
SelectClientCertificate(
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> s/virtual//.

Acknowledged.

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/notifica...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/622793002/diff/180001/chrome/browser/notifica...
chrome/browser/notifications/desktop_notification_service.cc:181:
result_callback);
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> Please remove the WeakPtrFactory from the DesktopNotificationService now that
> we're no longer using it.

Done.

https://codereview.chromium.org/622793002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/622793002/diff/180001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1009: -1 /* bridge id */,
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> If all consumers indeed use the routing Id, change this -1 to |routing_id_|
and
> remove that bit of the TODO. I see no reason to derive. I'll address the
> user_gesture plumbing after this lands.

Done.

https://codereview.chromium.org/622793002/diff/180001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1011: true /* user_gesture
*/,
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> Please use C++-style comments:
> 
> true,  // user_gesture

Done.

https://codereview.chromium.org/622793002/diff/180001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/622793002/diff/180001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:258: 0 /* bridge id */,
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> Please use c++ style comments:
> 
> 0,  // bridge_id

Done.

https://codereview.chromium.org/622793002/diff/180001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:262: true,
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> true,  // Only implemented for Android infobars which do not support
>        // user gestures.
> 
> (also fixes capitalization and spelling)

Done.

https://codereview.chromium.org/622793002/diff/180001/content/public/browser/...
File content/public/browser/permission_type.h (right):

https://codereview.chromium.org/622793002/diff/180001/content/public/browser/...
content/public/browser/permission_type.h:3: // found in the LICENSE file.
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> nit: empty line under this one.

Done.

https://codereview.chromium.org/622793002/diff/180001/content/public/browser/...
content/public/browser/permission_type.h:25: }  // namespace content
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> nit: empty line under this one.

Done.

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_content_browser_client.cc
(right):

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_content_browser_client.cc:82:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> Lift this DCHECK up one level higher please. It'd be bad if we were to invoke
> RequestPermission on different threads depending on |permission|.

Done.

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_notification_manager.cc
(right):

https://codereview.chromium.org/622793002/diff/180001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_notification_manager.cc:27:
(CheckPermission(origin) == blink::WebNotificationPermissionAllowed));
On 2014/10/24 14:37:28, Peter Beverloo wrote:
> Why the extra parenthesis here? Can we use a local instead?
> 
> bool allowed =
>     CheckPermission(origin) == blink::WebNotificationPermissionAllowed;
> callback.Run(allowed);

Done.

[Jun Mukai, 2014-10-24 18:12:50]

lgtm: chrome/browser/notifications

[Miguel Garcia, 2014-10-27 10:07:45]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-10-27 10:08:05]

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

[benm (inactive), 2014-10-27 10:44:19]

benm@chromium.org changed reviewers:
+ michaelbai@chromium.org

[benm (inactive), 2014-10-27 10:44:20]

Michael should take a look at this from the WebView side.

[commit-bot: I haz the power, 2014-10-27 10:58:13]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2014-10-27 10:58:58]

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

[Peter Beverloo, 2014-10-27 11:16:54]

On 2014/10/27 10:44:20, benm wrote:
> Michael should take a look at this from the WebView side.

To clarify, this is missing an android_webview/ review. Please carefully watch
out for the difference between reviewers and OWNERS.

[Miguel Garcia, 2014-10-27 11:28:58]

Michael can you have look?, I apologize for having sent it with your review. I
will fix any comments you might have in there.

[michaelbai, 2014-10-27 16:46:05]

android_webview LGTM

[xhwang, 2015-01-21 05:43:19]

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

[xhwang, 2015-01-21 05:43:20]

https://codereview.chromium.org/622793002/diff/220001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (left):

https://codereview.chromium.org/622793002/diff/220001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:268:
cdm_cancel_permission_map_[GetId(render_frame_id, cdm_id)] = cancel_callback;
We used to have this code here to cancel the info bar when the CDM is destroyed.
This code now is removed and we are not calling CancelPermissionRequest(). I
suspect the info bar will persist even after we destroy the CDM that requested
the info bar.

I'll double check this tomorrow and see what's the best way to fix it.