Implement PushManager#hasPermission

Chrome implementation of PushManager#hasPermission()
Depends on https://codereview.chromium.org/658723003/

BUG=401424
Committed: https://crrev.com/7e9a98dd33a2826f82251a1740b68ae473674a16
Cr-Commit-Position: refs/heads/master@{#302259}

[Miguel Garcia, 2014-10-15 15:27:38]

miguelg@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Miguel Garcia, 2014-10-15 15:27:54]

This is ready for an initial review.

[mlamouri (slow - plz ping), 2014-10-16 09:04:56]

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

[mlamouri (slow - plz ping), 2014-10-16 09:04:57]

drive-by review

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

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:55:
profile_->GetHostContentSettingsMap()
nit: do you really need a line break here?

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:242: return
PushPermissionStatus::PushPermissionDefault;
Why isn't that named PushPermissionAsk?

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:243: default:
What about the other values? Shouldn't SESSION_ONLY be allowed? and DEFAULT?

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:19:
PushPermissionStatus;
Do we allow typedef's to be added in a header?

https://codereview.chromium.org/661463002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/661463002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:85: BrowserThread::UI,
Could you use BrowserMessageFilter::OverrideThreadForMessage() instead of
manually posting this to the UI thread?

https://codereview.chromium.org/661463002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:143: render_frame_id);
nit: indent

https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.h:48: 
nit: no need for that new line.

https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.h:54: virtual void permissionStatus(
nit: uppercase 'P' for PermissionStatus().

https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.h:65:
blink::WebPushPermissionCallback::PushPermissionStatus);
nit: indent

[Miguel Garcia, 2014-10-16 13:50:27]

Thanks for the review Mounir!

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

https://chromiumcodereview.appspot.com/661463002/diff/40001/chrome/browser/co...
chrome/browser/content_settings/permission_context_base.cc:55:
profile_->GetHostContentSettingsMap()
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> nit: do you really need a line break here?

Done.

https://chromiumcodereview.appspot.com/661463002/diff/40001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://chromiumcodereview.appspot.com/661463002/diff/40001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.cc:242: return
PushPermissionStatus::PushPermissionDefault;
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> Why isn't that named PushPermissionAsk?

The API permission is defined as default. Since this enum comes from blink it
seems more reasonable to call it default. I don't feel too strongly about it
though.

https://chromiumcodereview.appspot.com/661463002/diff/40001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.cc:243: default:
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> What about the other values? Shouldn't SESSION_ONLY be allowed? and DEFAULT?

I rather keep it simple.
- Default does not mean anything in this context
- Session only exists in the enum but as far as I can see it is actually never
used for permissions as we understand them, they seem specific to cookies and
indexdb

https://cs.corp.google.com/#search/&sq=package:%5Eclankium$&q=SESSION_ONLY%20...

https://chromiumcodereview.appspot.com/661463002/diff/40001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://chromiumcodereview.appspot.com/661463002/diff/40001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.h:19:
PushPermissionStatus;
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> Do we allow typedef's to be added in a header?

I see this all the time
https://cs.corp.google.com/#search/&sq=package:%5Eclankium$&type=cs&q=typedef...

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/browser/p...
File content/browser/push_messaging_message_filter.cc (right):

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/browser/p...
content/browser/push_messaging_message_filter.cc:85: BrowserThread::UI,
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> Could you use BrowserMessageFilter::OverrideThreadForMessage() instead of
> manually posting this to the UI thread?

That actually sounds pretty cool (and I didn't know it existed). It seems I
would have to do the change for all the messages received in this filter which
is fine but I rather do it in a specific CL that only does that for both
messages. Sounds good?

I added a TODO in OnRegister

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/browser/p...
content/browser/push_messaging_message_filter.cc:143: render_frame_id);
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/renderer/...
File content/renderer/push_messaging_dispatcher.h (right):

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/renderer/...
content/renderer/push_messaging_dispatcher.h:48: 
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> nit: no need for that new line.

Done.

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/renderer/...
content/renderer/push_messaging_dispatcher.h:54: virtual void permissionStatus(
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> nit: uppercase 'P' for PermissionStatus().

It seems we are using lower case for the virtual methods in this case (see
registerPushMessaging above).

https://chromiumcodereview.appspot.com/661463002/diff/40001/content/renderer/...
content/renderer/push_messaging_dispatcher.h:65:
blink::WebPushPermissionCallback::PushPermissionStatus);
On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> nit: indent

Done.

[Miguel Garcia, 2014-10-16 13:50:27]

Thanks for the review Mounir!

[Miguel Garcia, 2014-10-17 08:53:43]

On 2014/10/16 13:50:27, Miguel Garcia wrote:
> Thanks for the review Mounir!

Friendly ping on this one too.

[mlamouri (slow - plz ping), 2014-10-17 09:30:56]

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:243: default:
On 2014/10/16 13:50:26, Miguel Garcia wrote:
> On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> > What about the other values? Shouldn't SESSION_ONLY be allowed? and DEFAULT?
> 
> I rather keep it simple.
> - Default does not mean anything in this context
> - Session only exists in the enum but as far as I can see it is actually never
> used for permissions as we understand them, they seem specific to cookies and
> indexdb
> 
>
https://cs.corp.google.com/#search/&sq=package:%5Eclankium$&q=SESSION_ONLY%20...

I would prefer to have SESSION_ONLY be respected and expose the permission as
granted in that case but if this is being mostly ignored by other users, I will
not insist ;)

This said, what about CONTENT_SETTING_DEFAULT? Shouldn't that be handled? Aren't
we going to reach NOTREACHED() if the value is DEFAULT?

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:19:
PushPermissionStatus;
On 2014/10/16 13:50:26, Miguel Garcia wrote:
> On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> > Do we allow typedef's to be added in a header?
> 
> I see this all the time
>
https://cs.corp.google.com/#search/&sq=package:%5Eclankium$&type=cs&q=typedef...

I see it being used inside classes or when a class define a complex type and
want consumers of the API it is exposing to have an easy access. It sounds
different in that case.

https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.h:54: virtual void permissionStatus(
On 2014/10/16 13:50:26, Miguel Garcia wrote:
> On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> > nit: uppercase 'P' for PermissionStatus().
> 
> It seems we are using lower case for the virtual methods in this case (see
> registerPushMessaging above).

registerPushMessaging() is an override from WebPushClient. Blink is using a
different style than Chromium.

[Miguel Garcia, 2014-10-20 09:40:42]

> > On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> > > What about the other values? Shouldn't SESSION_ONLY be allowed? and
DEFAULT?
> > 
> > I rather keep it simple.
> > - Default does not mean anything in this context
> > - Session only exists in the enum but as far as I can see it is actually
never
> > used for permissions as we understand them, they seem specific to cookies
and
> > indexdb
> > 
> >
>
https://cs.corp.google.com/#search/&sq=package:%5Eclankium$&q=SESSION_ONLY%20...
> 
> I would prefer to have SESSION_ONLY be respected and expose the permission as
> granted in that case but if this is being mostly ignored by other users, I
will
> not insist ;)
> 
> This said, what about CONTENT_SETTING_DEFAULT? Shouldn't that be handled?
Aren't
> we going to reach NOTREACHED() if the value is DEFAULT?

Every permission that matters in this case is initialized to ASK on startup
https://cs.corp.google.com/#clankium/src/chrome/browser/content_settings/cont...

What's more we don't prompt if what we see is DEFAULT so I am not really sure
what to return here. If we ever end up in this case I rather hit the NOTREACHED
to see that we have a bug.


> 
>
https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
> File chrome/browser/services/gcm/push_messaging_service_impl.h (right):
> 
>
https://codereview.chromium.org/661463002/diff/40001/chrome/browser/services/...
> chrome/browser/services/gcm/push_messaging_service_impl.h:19:
> PushPermissionStatus;
> On 2014/10/16 13:50:26, Miguel Garcia wrote:
> > On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> > > Do we allow typedef's to be added in a header?
> > 
> > I see this all the time
> >
>
https://cs.corp.google.com/#search/&sq=package:%5Eclankium$&type=cs&q=typedef...
> 
> I see it being used inside classes or when a class define a complex type and
> want consumers of the API it is exposing to have an easy access. It sounds
> different in that case.

I think the latter argument applies here as well.

> 
>
https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
> File content/renderer/push_messaging_dispatcher.h (right):
> 
>
https://codereview.chromium.org/661463002/diff/40001/content/renderer/push_me...
> content/renderer/push_messaging_dispatcher.h:54: virtual void
permissionStatus(
> On 2014/10/16 13:50:26, Miguel Garcia wrote:
> > On 2014/10/16 09:04:57, Mounir Lamouri wrote:
> > > nit: uppercase 'P' for PermissionStatus().
> > 
> > It seems we are using lower case for the virtual methods in this case (see
> > registerPushMessaging above).
> 
> registerPushMessaging() is an override from WebPushClient. Blink is using a
> different style than Chromium.

so is permissionStatus but you are right that it was not clear. I moved them
together and added /* override */ comments to both methods.

[Michael van Ouwerkerk, 2014-10-20 16:30:08]

Thanks Miguel! What's your test plan for this feature?

https://codereview.chromium.org/661463002/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/661463002/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:234: static
PushPermissionStatus ToPushPermission(
This should be in the anonymous namespace at the top.

https://codereview.chromium.org/661463002/diff/80001/chrome/browser/services/...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://codereview.chromium.org/661463002/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:18: typedef
blink::WebPushPermissionCallback::PushPermissionStatus
That's a weird way of pulling in an enum. It should be in its own file, without
the callback.

https://codereview.chromium.org/661463002/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:64: virtual
PushPermissionStatus PermissionStatus(
This method should be be named as a verb e.g. GetPermissionStatus.

https://codereview.chromium.org/661463002/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/661463002/diff/80001/content/browser/DEPS#new...
content/browser/DEPS:46:
"+third_party/WebKit/public/platform/WebPushPermissionCallback.h",
I think this stretches the limits of a enum/POD.

https://codereview.chromium.org/661463002/diff/80001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/661463002/diff/80001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:45: // TODO(miguelg) No need to
post to the UI thread manually
This is wrong, please delete it. ServiceWorkerContextCore is IO only.

https://codereview.chromium.org/661463002/diff/80001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:82: ServiceWorkerProviderHost*
service_worker_host =
Add this as the first line in this method:
DCHECK_CURRENTLY_ON(BrowserThread::IO);

https://codereview.chromium.org/661463002/diff/80001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:144:
blink::WebPushPermissionCallback::PushPermissionStatus permission_value =
DCHECK_CURRENTLY_ON(BrowserThread::UI);

https://codereview.chromium.org/661463002/diff/80001/content/renderer/push_me...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/661463002/diff/80001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.h:23: class
WebPushPermissionCallback;
No need as you already included it.

https://codereview.chromium.org/661463002/diff/80001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.h:46:
blink::WebServiceWorkerProvider* service_worker_provider) /* override */;
Not sure what the common form is for indicating override in a comment, but this
form shows 0 hits in code search. Isn't there any other precedent?

[Miguel Garcia, 2014-10-22 16:35:28]

Thanks for the review Michael! It seems we are converging so I am adding some
OWNERS now

buerb: for content_settings
fgorski: for gcm
jochen: for content/
palmer: for IPC security review

https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.cc:234: static
PushPermissionStatus ToPushPermission(
On 2014/10/20 16:30:07, Michael van Ouwerkerk wrote:
> This should be in the anonymous namespace at the top.

Done.

https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
File chrome/browser/services/gcm/push_messaging_service_impl.h (right):

https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
chrome/browser/services/gcm/push_messaging_service_impl.h:18: typedef
blink::WebPushPermissionCallback::PushPermissionStatus
On 2014/10/20 16:30:07, Michael van Ouwerkerk wrote:
> That's a weird way of pulling in an enum. It should be in its own file,
without
> the callback.

Done.

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/D...
content/browser/DEPS:46:
"+third_party/WebKit/public/platform/WebPushPermissionCallback.h",
On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
> I think this stretches the limits of a enum/POD.

Done.

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
File content/browser/push_messaging_message_filter.cc (right):

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
content/browser/push_messaging_message_filter.cc:45: // TODO(miguelg) No need to
post to the UI thread manually
On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
> This is wrong, please delete it. ServiceWorkerContextCore is IO only.

Done. Thanks for the offline explanation on why.

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
content/browser/push_messaging_message_filter.cc:82: ServiceWorkerProviderHost*
service_worker_host =
On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
> Add this as the first line in this method:
> DCHECK_CURRENTLY_ON(BrowserThread::IO);

Done.

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
content/browser/push_messaging_message_filter.cc:144:
blink::WebPushPermissionCallback::PushPermissionStatus permission_value =
On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
> DCHECK_CURRENTLY_ON(BrowserThread::UI);

Done.

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/renderer/...
File content/renderer/push_messaging_dispatcher.h (right):

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/renderer/...
content/renderer/push_messaging_dispatcher.h:23: class
WebPushPermissionCallback;
On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
> No need as you already included it.

Done (as in I don't include it anymore)

https://chromiumcodereview.appspot.com/661463002/diff/80001/content/renderer/...
content/renderer/push_messaging_dispatcher.h:46:
blink::WebServiceWorkerProvider* service_worker_provider) /* override */;
On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
> Not sure what the common form is for indicating override in a comment, but
this
> form shows 0 hits in code search. Isn't there any other precedent?

So I would prefer to leave it uncommnented but that will make the multi repo
changes hard while developing the spec. I rather leave it as is (instead of
removing it completetly) while the api stabilizes so we know they are overrides,
then at some point remove the comment.

[Miguel Garcia, 2014-10-22 16:37:38]

And my apologies to everybody for the "Not yet ready for review"
subject. The code review system is sending them like this (this was
the very first original title of the issue which has since changed).

On Wed, Oct 22, 2014 at 5:35 PM,  <miguelg@chromium.org> wrote:
> Thanks for the review Michael! It seems we are converging so I am adding
> some
> OWNERS now
>
> buerb: for content_settings
> fgorski: for gcm
> jochen: for content/
> palmer: for IPC security review
>
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
> File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
> chrome/browser/services/gcm/push_messaging_service_impl.cc:234: static
> PushPermissionStatus ToPushPermission(
> On 2014/10/20 16:30:07, Michael van Ouwerkerk wrote:
>>
>> This should be in the anonymous namespace at the top.
>
>
> Done.
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
> File chrome/browser/services/gcm/push_messaging_service_impl.h (right):
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/chrome/browser/se...
> chrome/browser/services/gcm/push_messaging_service_impl.h:18: typedef
> blink::WebPushPermissionCallback::PushPermissionStatus
> On 2014/10/20 16:30:07, Michael van Ouwerkerk wrote:
>>
>> That's a weird way of pulling in an enum. It should be in its own
>
> file, without
>>
>> the callback.
>
>
> Done.
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/DEPS
> File content/browser/DEPS (right):
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/D...
> content/browser/DEPS:46:
> "+third_party/WebKit/public/platform/WebPushPermissionCallback.h",
> On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
>>
>> I think this stretches the limits of a enum/POD.
>
>
> Done.
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
> File content/browser/push_messaging_message_filter.cc (right):
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
> content/browser/push_messaging_message_filter.cc:45: // TODO(miguelg) No
> need to post to the UI thread manually
> On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
>>
>> This is wrong, please delete it. ServiceWorkerContextCore is IO only.
>
>
> Done. Thanks for the offline explanation on why.
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
> content/browser/push_messaging_message_filter.cc:82:
> ServiceWorkerProviderHost* service_worker_host =
> On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
>>
>> Add this as the first line in this method:
>> DCHECK_CURRENTLY_ON(BrowserThread::IO);
>
>
> Done.
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/browser/p...
> content/browser/push_messaging_message_filter.cc:144:
> blink::WebPushPermissionCallback::PushPermissionStatus permission_value
> =
> On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
>>
>> DCHECK_CURRENTLY_ON(BrowserThread::UI);
>
>
> Done.
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/renderer/...
> File content/renderer/push_messaging_dispatcher.h (right):
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/renderer/...
> content/renderer/push_messaging_dispatcher.h:23: class
> WebPushPermissionCallback;
> On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
>>
>> No need as you already included it.
>
>
> Done (as in I don't include it anymore)
>
>
https://chromiumcodereview.appspot.com/661463002/diff/80001/content/renderer/...
> content/renderer/push_messaging_dispatcher.h:46:
> blink::WebServiceWorkerProvider* service_worker_provider) /* override
> */;
> On 2014/10/20 16:30:08, Michael van Ouwerkerk wrote:
>>
>> Not sure what the common form is for indicating override in a comment,
>
> but this
>>
>> form shows 0 hits in code search. Isn't there any other precedent?
>
>
> So I would prefer to leave it uncommnented but that will make the multi
> repo changes hard while developing the spec. I rather leave it as is
> (instead of removing it completetly) while the api stabilizes so we know
> they are overrides, then at some point remove the comment.
>
> https://chromiumcodereview.appspot.com/661463002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Michael van Ouwerkerk, 2014-10-24 12:36:08]

lgtm with nits

https://codereview.chromium.org/661463002/diff/100001/content/browser/push_me...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/661463002/diff/100001/content/browser/push_me...
content/browser/push_messaging_message_filter.cc:97: if (service_worker_host &&
service_worker_host->active_version()) {
What if this is false? You'd leave the promise hanging forever. The web page may
be spinning, the layout test is timing out. Please reject it instead. Really
this is an edge case we should not be hitting, but might as well handle it the
best we can.

https://codereview.chromium.org/661463002/diff/100001/content/browser/push_me...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/661463002/diff/100001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:39: void OnPermissionStatus(int
render_frame_id, int service_worker_provider_id,
nit: add a newline before this to remain consistent

https://codereview.chromium.org/661463002/diff/100001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/661463002/diff/100001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.cc:118:
blink::WebPushPermissionStatus type) {
tiny nit: I'd prefer status over type

[Miguel Garcia, 2014-10-29 18:32:03]

https://chromiumcodereview.appspot.com/661463002/diff/100001/content/browser/...
File content/browser/push_messaging_message_filter.cc (right):

https://chromiumcodereview.appspot.com/661463002/diff/100001/content/browser/...
content/browser/push_messaging_message_filter.cc:97: if (service_worker_host &&
service_worker_host->active_version()) {
On 2014/10/24 12:36:08, Michael van Ouwerkerk wrote:
> What if this is false? You'd leave the promise hanging forever. The web page
may
> be spinning, the layout test is timing out. Please reject it instead. Really
> this is an edge case we should not be hitting, but might as well handle it the
> best we can.

Done.

https://chromiumcodereview.appspot.com/661463002/diff/100001/content/browser/...
File content/browser/push_messaging_message_filter.h (right):

https://chromiumcodereview.appspot.com/661463002/diff/100001/content/browser/...
content/browser/push_messaging_message_filter.h:39: void OnPermissionStatus(int
render_frame_id, int service_worker_provider_id,
On 2014/10/24 12:36:08, Michael van Ouwerkerk wrote:
> nit: add a newline before this to remain consistent

Done.

https://chromiumcodereview.appspot.com/661463002/diff/100001/content/renderer...
File content/renderer/push_messaging_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/661463002/diff/100001/content/renderer...
content/renderer/push_messaging_dispatcher.cc:118:
blink::WebPushPermissionStatus type) {
On 2014/10/24 12:36:08, Michael van Ouwerkerk wrote:
> tiny nit: I'd prefer status over type

Done.

[Miguel Garcia, 2014-10-29 18:36:12]

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

[Miguel Garcia, 2014-10-29 18:36:13]

Adding Owners for the different directories

jam@ for content and the DEPS changes
bauerb@ for content_settings
fgorski@ for gcm
palmer@ for IPC security review

Thanks!

[jam, 2014-10-29 20:08:58]

lgtm

https://codereview.chromium.org/661463002/diff/120001/content/public/browser/...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/661463002/diff/120001/content/public/browser/...
content/public/browser/push_messaging_service.h:35: virtual
blink::WebPushPermissionStatus GetPermissionStatus(
nit: document everything in this interface

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.cc:117: DCHECK(callback);
here and below, this doesn't do much. in release, it's compiled out. in debug,
the crash on the line below is just as useful

[mlamouri (slow - plz ping), 2014-10-29 20:13:33]

nit: you are using |renderer_id| in some places where you actually mean
|render_process_id|.

lgtm, otherwise.

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:296: LOG(WARNING) <<
"GCM RETURNED " << result;
nit: leftover debugging code?

https://codereview.chromium.org/661463002/diff/120001/content/browser/push_me...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/661463002/diff/120001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:40: void OnPermissionStatus(int
render_frame_id,
nit: this name sounds quite confusing to me. Could it be named
|OnPermissionStatusRequest|?

https://codereview.chromium.org/661463002/diff/120001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:57: void
DoPermissionStatus(const GURL& requesting_origin,
nit: I think |DidGetPermissionStatus| would be a clearer name. The current name
is quite confusing.

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:47: void
DoRegister(blink::WebPushRegistrationCallbacks* callbacks,
nit: leave an empty line between |getPermissionStatus()| and |DoRegister()| to
make it clearer that the latter isn't a WebPushClient implementation.

[palmer, 2014-10-29 20:45:26]

IPC security LGTM.

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.h:68: virtual
ContentSetting PermissionStatus(const GURL& requesting_origin,
Gloriously minor nit: choose 1 of -ing, -[oe]r. I'd probably go with
|requesting_origin| and |embedding_origin|, but whatever.

[fgorski, 2014-10-29 20:52:44]

Is there a way to add unit test for this functionality? Or do you plan to test
it with browser tests?

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:34: static
blink::WebPushPermissionStatus ToPushPermission(ContentSetting setting) {
why static?

[Miguel Garcia, 2014-10-30 12:27:15]

Thanks a lot for the quick review!

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.h:68: virtual
ContentSetting PermissionStatus(const GURL& requesting_origin,
On 2014/10/29 20:45:25, Chromium Palmer wrote:
> Gloriously minor nit: choose 1 of -ing, -[oe]r. I'd probably go with
> |requesting_origin| and |embedding_origin|, but whatever.

Done. I took the chance and made the method const as well.

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:34: static
blink::WebPushPermissionStatus ToPushPermission(ContentSetting setting) {
On 2014/10/29 20:52:44, fgorski wrote:
> why static?

Ok, I removed it. I actually don't know if there is an advantage or disadvantage
either way (since it is not an object method but a simple function).

https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:296: LOG(WARNING) <<
"GCM RETURNED " << result;
On 2014/10/29 20:13:33, Mounir Lamouri wrote:
> nit: leftover debugging code?

Indeed, sorry about that.

https://codereview.chromium.org/661463002/diff/120001/content/browser/push_me...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/661463002/diff/120001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:40: void OnPermissionStatus(int
render_frame_id,
On 2014/10/29 20:13:33, Mounir Lamouri wrote:
> nit: this name sounds quite confusing to me. Could it be named
> |OnPermissionStatusRequest|?

Sure, it did not strike me as confusing but here it is.

https://codereview.chromium.org/661463002/diff/120001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:57: void
DoPermissionStatus(const GURL& requesting_origin,
On 2014/10/29 20:13:33, Mounir Lamouri wrote:
> nit: I think |DidGetPermissionStatus| would be a clearer name. The current
name
> is quite confusing.

It is not really DidGet since by the time we call it we don't really know. I
think I am going with DoPermissionStatusRequest since that is the pattern
followed here (and usually) the IPC ones are Called OnX, then the callbacks are
called DoX

https://codereview.chromium.org/661463002/diff/120001/content/public/browser/...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/661463002/diff/120001/content/public/browser/...
content/public/browser/push_messaging_service.h:35: virtual
blink::WebPushPermissionStatus GetPermissionStatus(
On 2014/10/29 20:08:58, jam wrote:
> nit: document everything in this interface

Done.

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.cc:117: DCHECK(callback);
On 2014/10/29 20:08:58, jam wrote:
> here and below, this doesn't do much. in release, it's compiled out. in debug,
> the crash on the line below is just as useful

It's kind of a nice way of saying. Hey I am callback is always there. But ok.

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/661463002/diff/120001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:47: void
DoRegister(blink::WebPushRegistrationCallbacks* callbacks,
On 2014/10/29 20:13:33, Mounir Lamouri wrote:
> nit: leave an empty line between |getPermissionStatus()| and |DoRegister()| to
> make it clearer that the latter isn't a WebPushClient implementation.

Done.

[Miguel Garcia, 2014-10-30 12:27:28]

On 2014/10/29 20:52:44, fgorski wrote:
> Is there a way to add unit test for this functionality? Or do you plan to test
> it with browser tests?

Now that https://codereview.chromium.org/648623003 has landed I was planning to
build on that with browser tests. I want to first land the layout tests
(https://codereview.chromium.org/676003002/) which depend on this CL but browser
tests are coming.


> 
>
https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
> File chrome/browser/services/gcm/push_messaging_service_impl.cc (right):
> 
>
https://codereview.chromium.org/661463002/diff/120001/chrome/browser/services...
> chrome/browser/services/gcm/push_messaging_service_impl.cc:34: static
> blink::WebPushPermissionStatus ToPushPermission(ContentSetting setting) {
> why static?

[Bernhard Bauer, 2014-10-30 14:19:41]

LGTM with nits:

https://codereview.chromium.org/661463002/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/661463002/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.h:68: virtual
ContentSetting PermissionStatus(const GURL& requesting_origin,
Nit: GetPermissionStatus, to have a verb in the method name?

https://codereview.chromium.org/661463002/diff/140001/content/browser/push_me...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/661463002/diff/140001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:16: class GURL;
Nit: empty line afterwards

https://codereview.chromium.org/661463002/diff/140001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:57: void
DoPermissionStatusRequest(const GURL& requesting_origin,
Nit: What's the order of methods here? I would either group them by topic (i.e.
.*Register together, then .*PermissionStatusRequest), or by prefix (On.*
together, then Do.*, then Did.*).

[fgorski, 2014-10-30 16:04:19]

lgtm

[Miguel Garcia, 2014-10-31 13:02:30]

https://codereview.chromium.org/661463002/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/661463002/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/permission_context_base.h:68: virtual
ContentSetting PermissionStatus(const GURL& requesting_origin,
On 2014/10/30 14:19:41, Bernhard Bauer wrote:
> Nit: GetPermissionStatus, to have a verb in the method name?

Done.

https://codereview.chromium.org/661463002/diff/140001/content/browser/push_me...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/661463002/diff/140001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:16: class GURL;
On 2014/10/30 14:19:41, Bernhard Bauer wrote:
> Nit: empty line afterwards

Done.

https://codereview.chromium.org/661463002/diff/140001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:57: void
DoPermissionStatusRequest(const GURL& requesting_origin,
On 2014/10/30 14:19:41, Bernhard Bauer wrote:
> Nit: What's the order of methods here? I would either group them by topic
(i.e.
> .*Register together, then .*PermissionStatusRequest), or by prefix (On.*
> together, then Do.*, then Did.*).

Done.

[Miguel Garcia, 2014-10-31 13:02:40]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-10-31 13:03:23]

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

[commit-bot: I haz the power, 2014-10-31 14:21:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-10-31 14:22:28]

Patchset 9 (id:??) landed as
https://crrev.com/7e9a98dd33a2826f82251a1740b68ae473674a16
Cr-Commit-Position: refs/heads/master@{#302259}