Dispatch push event to worker from PushServiceImpl#OnMessage.

Push API: dispatch push event to service worker.
BUG=350377
Depends on https://codereview.chromium.org/404353003/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288023

[mlamouri (slow - plz ping), 2014-06-20 13:57:23]

Would be great to fix that issue so people trying to build your patch with clang
don't need to do manual fixes ;)

https://codereview.chromium.org/340773006/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.h (right):

https://codereview.chromium.org/340773006/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.h:44: virtual int
id() { return provider_id_; }
You should not inline this if you want to make clang chromium-style happy. IOW,
it doesn't build with clang ;)

[Michael van Ouwerkerk, 2014-06-20 15:02:44]

https://codereview.chromium.org/340773006/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.h (right):

https://codereview.chromium.org/340773006/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.h:44: virtual int
id() { return provider_id_; }
On 2014/06/20 13:57:23, Mounir Lamouri wrote:
> You should not inline this if you want to make clang chromium-style happy.
IOW,
> it doesn't build with clang ;)

Done.

[Michael van Ouwerkerk, 2014-07-22 11:28:28]

Hi John, could you review this from a feature perspective?

[Michael van Ouwerkerk, 2014-07-22 13:32:44]

Hi michaeln, could you please review this from a Service Worker perspective?

[johnme, 2014-07-23 14:25:24]

lgtm with nits. It'll be great to have this working end-to-end :)

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

https://codereview.chromium.org/340773006/diff/130001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:77:
push_messaging_router_ = content::PushMessagingRouter::Create(profile);
Nit: I guess this could be in the initializer list? But I don't really mind.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
File content/browser/push_messaging_router_impl.cc (right):

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:25:
PushMessagingRouterImpl::PushMessagingRouterImpl(
Nit: Would it make sense for the constructor to
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ?

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:82: const
PushMessagingApplicationId& application_id,
application_id appears to be unused.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:102: void
PushMessagingRouterImpl::DoSendMessage(
Nit: let's add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); for
clarity.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:109: // this method returns.
Nit: s/returns/returns (before the event is delivered to the Service Worker)/ ?

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:128:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Nit: move to top of method?

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
File content/browser/push_messaging_router_impl.h (right):

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.h:28: explicit
PushMessagingRouterImpl(BrowserContext* browser_context);
I'd suggest to either make this constructor private or remove the static Create
method; it's confusing having two ways to instantiate this.

https://codereview.chromium.org/340773006/diff/130001/content/public/browser/...
File content/public/browser/push_messaging_router.h (right):

https://codereview.chromium.org/340773006/diff/130001/content/public/browser/...
content/public/browser/push_messaging_router.h:31: virtual void SendMessage(
s/Send/Deliver/ to avoid confusion with upstream message sending. Ditto
everywhere else.

https://codereview.chromium.org/340773006/diff/130001/content/public/common/p...
File content/public/common/push_messaging_status.cc (right):

https://codereview.chromium.org/340773006/diff/130001/content/public/common/p...
content/public/common/push_messaging_status.cc:32: return "Message delivery
failed because no Service Worker was found";
Nit: "Message delivery failed - no matching Service Worker was found" for
consistency? Ditto below.

[Michael van Ouwerkerk, 2014-07-23 16:57:52]

Thanks John!

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

https://codereview.chromium.org/340773006/diff/130001/chrome/browser/services...
chrome/browser/services/gcm/push_messaging_service_impl.cc:77:
push_messaging_router_ = content::PushMessagingRouter::Create(profile);
On 2014/07/23 14:25:23, johnme wrote:
> Nit: I guess this could be in the initializer list? But I don't really mind.

Done.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
File content/browser/push_messaging_router_impl.cc (right):

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:25:
PushMessagingRouterImpl::PushMessagingRouterImpl(
On 2014/07/23 14:25:23, johnme wrote:
> Nit: Would it make sense for the constructor to
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ?

I think it's not necessary for this class, as it's a simple constructor.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:82: const
PushMessagingApplicationId& application_id,
On 2014/07/23 14:25:23, johnme wrote:
> application_id appears to be unused.

Done.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:102: void
PushMessagingRouterImpl::DoSendMessage(
On 2014/07/23 14:25:23, johnme wrote:
> Nit: let's add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); for
> clarity.

Done.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:109: // this method returns.
On 2014/07/23 14:25:23, johnme wrote:
> Nit: s/returns/returns (before the event is delivered to the Service Worker)/
?

Done.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:128:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/07/23 14:25:23, johnme wrote:
> Nit: move to top of method?

Done.

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
File content/browser/push_messaging_router_impl.h (right):

https://codereview.chromium.org/340773006/diff/130001/content/browser/push_me...
content/browser/push_messaging_router_impl.h:28: explicit
PushMessagingRouterImpl(BrowserContext* browser_context);
On 2014/07/23 14:25:23, johnme wrote:
> I'd suggest to either make this constructor private or remove the static
Create
> method; it's confusing having two ways to instantiate this.

The PushMessagingRouterImpl constructor does need to be public so that
PushMessagingRouter::Create can call it. PushMessagingRouter is in the public
content API, PushMessagingRouterImpl is not. So this constructor is invisible
outside of the content layer. I think this is how the pattern is supposed to be.

https://codereview.chromium.org/340773006/diff/130001/content/public/browser/...
File content/public/browser/push_messaging_router.h (right):

https://codereview.chromium.org/340773006/diff/130001/content/public/browser/...
content/public/browser/push_messaging_router.h:31: virtual void SendMessage(
On 2014/07/23 14:25:23, johnme wrote:
> s/Send/Deliver/ to avoid confusion with upstream message sending. Ditto
> everywhere else.

Done.

https://codereview.chromium.org/340773006/diff/130001/content/public/common/p...
File content/public/common/push_messaging_status.cc (right):

https://codereview.chromium.org/340773006/diff/130001/content/public/common/p...
content/public/common/push_messaging_status.cc:32: return "Message delivery
failed because no Service Worker was found";
On 2014/07/23 14:25:23, johnme wrote:
> Nit: "Message delivery failed - no matching Service Worker was found" for
> consistency? Ditto below.

Done.

[michaeln, 2014-07-24 02:08:17]

looks pretty good

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
File content/browser/push_messaging_router_impl.cc (right):

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:13: #include
"content/public/browser/push_messaging_application_id.h"
missing file? push_messaging_application_id.h

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:61:
service_worker_context->context()->GetLiveRegistration(
this code path to lookup the 'live' object may not be needed, the Find method
below will do that lookup and if found return it to you callback immediately

having one code patch could simplify this method a little

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:108: // Hold on to the service
worker registration in the callback to keep it alive
nice comment

https://codereview.chromium.org/340773006/diff/150001/content/public/browser/...
File content/public/browser/push_messaging_router.h (right):

https://codereview.chromium.org/340773006/diff/150001/content/public/browser/...
content/public/browser/push_messaging_router.h:18: struct
PushMessagingApplicationId;
where is this defined?

[Michael van Ouwerkerk, 2014-07-24 14:18:55]

Thanks Michael! Please take another look.

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
File content/browser/push_messaging_router_impl.cc (right):

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:13: #include
"content/public/browser/push_messaging_application_id.h"
On 2014/07/24 02:08:16, michaeln wrote:
> missing file? push_messaging_application_id.h

It's defined in the patch this depends on:
https://codereview.chromium.org/404353003/

I had noted the dependency in the description but I guess I should have
clarified that better, sorry.

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:61:
service_worker_context->context()->GetLiveRegistration(
On 2014/07/24 02:08:16, michaeln wrote:
> this code path to lookup the 'live' object may not be needed, the Find method
> below will do that lookup and if found return it to you callback immediately
> 
> having one code patch could simplify this method a little

Done. Nice.

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:108: // Hold on to the service
worker registration in the callback to keep it alive
On 2014/07/24 02:08:16, michaeln wrote:
> nice comment

Yeah that was actually a bit of a mean bug. Maybe in future we can find a way to
hide this pattern inside the SW code? I think the sync people had the same
issue.

https://codereview.chromium.org/340773006/diff/150001/content/public/browser/...
File content/public/browser/push_messaging_router.h (right):

https://codereview.chromium.org/340773006/diff/150001/content/public/browser/...
content/public/browser/push_messaging_router.h:18: struct
PushMessagingApplicationId;
On 2014/07/24 02:08:16, michaeln wrote:
> where is this defined?

(In another pending patch)

[michaeln, 2014-07-25 00:08:27]

lgtm (sorry i missed the other cl)

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
File content/browser/push_messaging_router_impl.cc (right):

https://codereview.chromium.org/340773006/diff/150001/content/browser/push_me...
content/browser/push_messaging_router_impl.cc:108: // Hold on to the service
worker registration in the callback to keep it alive
On 2014/07/24 14:18:55, Michael van Ouwerkerk wrote:
> On 2014/07/24 02:08:16, michaeln wrote:
> > nice comment
> 
> Yeah that was actually a bit of a mean bug. Maybe in future we can find a way
to
> hide this pattern inside the SW code? I think the sync people had the same
> issue.

Right, maybe the SW side of this should keep things alive long enough to
DispatchPushEvent and give back the results.

[Michael van Ouwerkerk, 2014-07-25 13:08:39]

jam: could you please review this patch as content owner?

[jam, 2014-07-25 21:52:45]

first, apologies for the delay (I blame the CY :) )

why do we need this PushMessagingRouter context. How about a static method on
BrowserContext?

[Michael van Ouwerkerk, 2014-07-28 12:55:26]

On 2014/07/25 21:52:45, jam wrote:
> first, apologies for the delay (I blame the CY :) )
> 
> why do we need this PushMessagingRouter context. How about a static method on
> BrowserContext?

No worries, CY has to be done. I'm not quite sure why you made this suggestion:
1. There seems little to no precedent for using BrowserContext to plumb through
a web exposed feature.
2. The implementation needs to live in the content layer to access the service
worker system. It seems that the primary implementation for BrowserContext lives
in chrome/browser/profiles/profile_impl.cc
3. BrowserContext is UI thread only, PushMessagingRouterImpl also uses the IO
thread.
4. We'll need at least one more method to plumb from
PushMessagingServiceImpl::OnMessagesDeleted and a somewhat object oriented
design seems to fit. 5. If I keep the push messaging implementation separate
from major classes and interfaces such as BrowserContext I might later move the
code to a directory and set up an OWNERS file for feature maintainers.

So I guess I'm asking... why would a static method on BrowserContext be better?

[jam, 2014-07-29 22:38:46]

On 2014/07/28 12:55:26, Michael van Ouwerkerk wrote:
> On 2014/07/25 21:52:45, jam wrote:
> > first, apologies for the delay (I blame the CY :) )
> > 
> > why do we need this PushMessagingRouter context. How about a static method
on
> > BrowserContext?
> 
> No worries, CY has to be done. I'm not quite sure why you made this
suggestion:
> 1. There seems little to no precedent for using BrowserContext to plumb
through
> a web exposed feature.
> 2. The implementation needs to live in the content layer to access the service
> worker system. It seems that the primary implementation for BrowserContext
lives
> in chrome/browser/profiles/profile_impl.cc

The static methods on BrowserContext are methods that are implemented inside the
content layer. There's no distinction between web-exposed or non web-exposed
there.

> 3. BrowserContext is UI thread only, PushMessagingRouterImpl also uses the IO
> thread.

That's an internal implementation detail, but the api doesn't have to know that.

> 4. We'll need at least one more method to plumb from
> PushMessagingServiceImpl::OnMessagesDeleted and a somewhat object oriented
> design seems to fit.

you say "at least", do you know for sure there'll be a few more/

> 5. If I keep the push messaging implementation separate
> from major classes and interfaces such as BrowserContext I might later move
the
> code to a directory and set up an OWNERS file for feature maintainers.

that is still possible, i.e. the static method could use a class that is
implemented in a separate directory

> 
> So I guess I'm asking... why would a static method on BrowserContext be
better?

just because we try to avoid interfaces with single-methods

[Michael van Ouwerkerk, 2014-08-06 17:34:51]

I've moved it to BrowserContext. Please take another look :-)

[jam, 2014-08-06 21:22:09]

lgtm

[Michael van Ouwerkerk, 2014-08-07 09:13:04]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-08-07 09:14:15]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/340773006/2...

[commit-bot: I haz the power, 2014-08-07 10:04:46]

Change committed as 288023