Hook PushMessagingMessageFilter up to GCMDriver

Hook PushMessagingMessageFilter up to GCMDriver

Add plumbing for registration from push service-agnostic content layer
to GCMDriver in chrome layer.

To achieve this, GCMProfileService owns an implementation of a new
PushMessagingService class, which can be used from the content layer.

Based on mvanouwerkerk's prototype in https://codereview.chromium.org/186023002, but significantly refactored.

BUG=350384
TBR=benm@chromium.org

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

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

[johnme, 2014-06-05 19:15:55]

Jian and/or Filip, can you review the chrome/browser/services/gcm parts, and
also check that the general approach makes sense?

Jochen, since you reviewed mvanouwerkerk's creation of
PushMessagingMessageFilter[1], would you mind giving OWNERS review of the rest?

Thanks!

[1]: https://codereview.chromium.org/219653002

[jochen (gone - plz use gerrit), 2014-06-06 07:05:38]

+mvanouwerkerk

s/getting/getter/

https://codereview.chromium.org/317823007/diff/1/content/public/browser/conte...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/317823007/diff/1/content/public/browser/conte...
content/public/browser/content_browser_client.cc:244: BrowserContext*
browser_context) {
why not add a push messaging service getting to browser context?

[Michael van Ouwerkerk, 2014-06-06 10:43:30]

https://codereview.chromium.org/317823007/diff/1/content/browser/push_messagi...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/317823007/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:54:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Looks like we really do everything on the browser thread. Maybe for a future
patch consider using a WebContentsObserver instead of a BrowserMessageFilter so
all our code just runs on the browser thread.

[johnme, 2014-06-06 13:31:43]

On 2014/06/06 07:05:38, jochen traveling until Jun 23 wrote:
>
https://codereview.chromium.org/317823007/diff/1/content/public/browser/conte...
> File content/public/browser/content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/317823007/diff/1/content/public/browser/conte...
> content/public/browser/content_browser_client.cc:244: BrowserContext*
> browser_context) {
> why not add a push messaging service getter to browser context?

Done (and removed from ContentBrowserClient accordingly).

On 2014/06/06 10:43:30, Michael van Ouwerkerk wrote:
>
https://codereview.chromium.org/317823007/diff/1/content/browser/push_messagi...
> File content/browser/push_messaging_message_filter.cc (right):
> 
>
https://codereview.chromium.org/317823007/diff/1/content/browser/push_messagi...
> content/browser/push_messaging_message_filter.cc:54:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> Looks like we really do everything on the browser thread. Maybe for a future
> patch consider using a WebContentsObserver instead of a BrowserMessageFilter
so
> all our code just runs on the browser thread.

Makes sense; it would probably confuse this patch to do both at the same time,
so I've added a TODO.

[Michael van Ouwerkerk, 2014-06-06 13:42:46]

lgtm

https://codereview.chromium.org/317823007/diff/40001/content/public/browser/b...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/317823007/diff/40001/content/public/browser/b...
content/public/browser/browser_context.h:191: virtual PushMessagingService*
GetPushMessagingService() = 0;
It seems we could provide a default implementation that returns NULL. Is that
against pattern?

[johnme, 2014-06-06 14:17:54]

https://codereview.chromium.org/317823007/diff/40001/content/public/browser/b...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/317823007/diff/40001/content/public/browser/b...
content/public/browser/browser_context.h:191: virtual PushMessagingService*
GetPushMessagingService() = 0;
On 2014/06/06 13:42:46, Michael van Ouwerkerk wrote:
> It seems we could provide a default implementation that returns NULL. Is that
> against pattern?

None of the others do this. Perhaps it's intentional to force the embedder to
notice any new methods and think about them?

[jianli, 2014-06-06 17:32:21]

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.h:58:
content::PushMessagingService* push_messaging_service() {
nit: add const modifier

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:25: if
(!gcm_profile_service_->driver())
Please comment that GCMDriver could be NULL due to that the GCM service might be
shut down.

Or, you could avoid doing such check. You can let GCMProfileService own
PushMessagingServiceImpl via scoped_ptr. In GCMProfileService::Shutdown, you
also reset the scoped ptr.

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:16: class
PushMessagingServiceImpl : public content::PushMessagingService {
Please document what this class is for.

[johnme, 2014-06-06 18:48:25]

Thanks, done.

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.h:58:
content::PushMessagingService* push_messaging_service() {
On 2014/06/06 17:32:20, jianli wrote:
> nit: add const modifier

Done.

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:25: if
(!gcm_profile_service_->driver())
On 2014/06/06 17:32:21, jianli wrote:
> Please comment that GCMDriver could be NULL due to that the GCM service might
be
> shut down.
> 
> Or, you could avoid doing such check. You can let GCMProfileService own
> PushMessagingServiceImpl via scoped_ptr. In GCMProfileService::Shutdown, you
> also reset the scoped ptr.

I added the comment, since in the current approach, PushMessagingMessageFilters
keep a raw pointer to the PushMessagingService they are given, so I need the
lifetime of PushMessagingService to be greater than the lifetime of any
PushMessagingMessageFilters, which is tied to the lifetime of the corresponding
RenderProcessHosts. Open to other ways of handling the lifetimes though.

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:16: class
PushMessagingServiceImpl : public content::PushMessagingService {
On 2014/06/06 17:32:21, jianli wrote:
> Please document what this class is for.

Done.

[fgorski, 2014-06-09 17:43:49]

gcm parts look ok. Please apply comments.

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

https://codereview.chromium.org/317823007/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:33:
base::Unretained(this),
This does not look safe, as you don't have any additional shutdown logic.
I think you should replace that with weak_ptr_factory.

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

https://codereview.chromium.org/317823007/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:32: GCMProfileService*
gcm_profile_service_; // It owns us.
nit: end-of-line comments should be separated from the code by 2 spaces.

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

https://codereview.chromium.org/317823007/diff/80001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:46: this,
weak_factory_.GetWeakPtr()?

[johnme, 2014-06-09 19:04:46]

Addresses Filip's comments - thanks!

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

https://codereview.chromium.org/317823007/diff/40001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.h:58:
content::PushMessagingService* push_messaging_service() {
On 2014/06/06 18:48:24, johnme wrote:
> On 2014/06/06 17:32:20, jianli wrote:
> > nit: add const modifier
> 
> Done.

Actually, I replied too soon - I had to remove the const modifier because when a
const method takes a reference to a non-mutable member variable, the pointer you
get is a const pointer, so compile failed trying to cast "const
PushMessagingServiceImpl*" to "content::PushMessagingService*". I could add a
const_cast or mark push_messaging_service_ as mutable, but actually I disagree
that this method should be const, since the returned PushMessagingService lets
you mutate GCMDriver, which is part of the state of GCMProfileService. In a old
chromium-dev thread[1], there seemed to be rough agreement that in general const
methods should not return non-const pointers. So I think having this non-const
is correct, and for consistency we should make driver() be non-const (which
doesn't break any existing code), though I'll leave that for a separate patch.

[1]:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/zvd-z8U_u1I/dis...

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

https://codereview.chromium.org/317823007/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.cc:33:
base::Unretained(this),
On 2014/06/09 17:43:49, fgorski wrote:
> This does not look safe, as you don't have any additional shutdown logic.
> I think you should replace that with weak_ptr_factory.

I think it probably was safe, as this is a member variable of GCMProfileService
which owns GCMDriver, so it should always outlive GCMDriver (and the callback is
not persisted beyond the lifetime of GCMDriver). But I'm not 100% confident in
this, and a weak_ptr_factory does sound less brittle, so done.

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

https://codereview.chromium.org/317823007/diff/80001/chrome/browser/services/...
chrome/browser/services/gcm/push_messaging_service_impl.h:32: GCMProfileService*
gcm_profile_service_; // It owns us.
On 2014/06/09 17:43:49, fgorski wrote:
> nit: end-of-line comments should be separated from the code by 2 spaces.

Done.

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

https://codereview.chromium.org/317823007/diff/80001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:46: this,
On 2014/06/09 17:43:49, fgorski wrote:
> weak_factory_.GetWeakPtr()?

So if the render process gets killed while this is thread-hopping, it won't
bother registering, rather than initiating a registration (since Bind holds a
ref to this that will keep it alive) that it knows it won't be able to deliver?
Sure, may as well since there's already a weak_factory_. Done.

[fgorski, 2014-06-09 23:11:10]

lgtm

[jianli, 2014-06-09 23:22:10]

lgtm

[jochen (gone - plz use gerrit), 2014-06-12 14:52:31]

lgtm

[johnme, 2014-06-12 16:27:34]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:30:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/317823007/160001

[commit-bot: I haz the power, 2014-06-12 20:43:08]

Change committed as 276792

[Dirk Pranke, 2014-06-12 22:40:40]

On 2014/06/12 20:43:08, I haz the power (commit-bot) wrote:
> Change committed as 276792

Unfortunately, it looks like this patch breaks the single push_messaging
layout_test :(.

See, e.g., 

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/32769
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux/327...

(the test times out).

So I'm reverting it. Sorry!

[Dirk Pranke, 2014-06-12 23:01:28]

On 2014/06/12 22:40:40, Dirk Pranke wrote:
> On 2014/06/12 20:43:08, I haz the power (commit-bot) wrote:
> > Change committed as 276792
> 
> Unfortunately, it looks like this patch breaks the single push_messaging
> layout_test :(.
> 
> See, e.g., 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/32769
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux/327...
> 
> (the test times out).
> 
> So I'm reverting it. Sorry!

Sigh. The second URL is
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux/327...

[johnme, 2014-06-13 11:53:24]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2014-06-13 11:54:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnme@chromium.org/317823007/180001

[commit-bot: I haz the power, 2014-06-13 14:43:43]

Change committed as 277016

[jam, 2014-06-15 03:23:24]

why is any of this code in content? this appears to be a chrome/google specific
feature, i.e. not the web platform. so it should belong completely in chrome.
see http://www.chromium.org/developers/content-module

[Michael van Ouwerkerk, 2014-06-15 19:47:20]

On 2014/06/15 03:23:24, jam wrote:
> why is any of this code in content? this appears to be a chrome/google
specific
> feature, i.e. not the web platform. so it should belong completely in chrome.
> see http://www.chromium.org/developers/content-module

John, the Push API is a W3C draft standard: https://w3c.github.io/push-api/

The implementation uses GCM, but the GCM bits are not in the content layer.
There are some more details in the design document:
https://docs.google.com/a/chromium.org/document/d/1-YTIsnKfLfqELO54vo-lz3WaW2...

[jam, 2014-06-16 06:41:33]

On 2014/06/15 19:47:20, Michael van Ouwerkerk wrote:
> On 2014/06/15 03:23:24, jam wrote:
> > why is any of this code in content? this appears to be a chrome/google
> specific
> > feature, i.e. not the web platform. so it should belong completely in
chrome.
> > see http://www.chromium.org/developers/content-module
> 
> John, the Push API is a W3C draft standard: https://w3c.github.io/push-api/
> 
> The implementation uses GCM, but the GCM bits are not in the content layer.
> There are some more details in the design document:
>
https://docs.google.com/a/chromium.org/document/d/1-YTIsnKfLfqELO54vo-lz3WaW2...

oh i see, thanks for the links. I hadn't seen any of these in the linked bug,
and only mention of chrome.

questions: why is there push_messaging_service with just one method? i.e. why
isn't this on BrowserContext?
Also on the renderer side, I see virtual blink::WebPushClient* webPushClient();
on RenderViewImpl. This should be on RenderFrameImpl, as that's where we're
moving existing stuff off as part of site isolation. Since
WebView/WebViewClient/RenderView are eventually going away, new interfaces like
this should go on the new place initially instead of the old place and then
having to convert them later.

[Michael van Ouwerkerk, 2014-06-17 13:29:57]

On 2014/06/16 06:41:33, jam wrote:
> On 2014/06/15 19:47:20, Michael van Ouwerkerk wrote:
> > On 2014/06/15 03:23:24, jam wrote:
> > > why is any of this code in content? this appears to be a chrome/google
> > specific
> > > feature, i.e. not the web platform. so it should belong completely in
> chrome.
> > > see http://www.chromium.org/developers/content-module
> > 
> > John, the Push API is a W3C draft standard: https://w3c.github.io/push-api/
> > 
> > The implementation uses GCM, but the GCM bits are not in the content layer.
> > There are some more details in the design document:
> >
>
https://docs.google.com/a/chromium.org/document/d/1-YTIsnKfLfqELO54vo-lz3WaW2...
> 
> oh i see, thanks for the links. I hadn't seen any of these in the linked bug,
> and only mention of chrome.
> 
> questions: why is there push_messaging_service with just one method? i.e. why
> isn't this on BrowserContext?
> Also on the renderer side, I see virtual blink::WebPushClient*
webPushClient();
> on RenderViewImpl. This should be on RenderFrameImpl, as that's where we're
> moving existing stuff off as part of site isolation. Since
> WebView/WebViewClient/RenderView are eventually going away, new interfaces
like
> this should go on the new place initially instead of the old place and then
> having to convert them later.

John: there will be more methods for the other JavaScript calls: unregister(),
isRegistered(), and hasPermission().

You are right, we have to port this to RenderFrameImpl. This is a leftover from
the earliest commits and prototypes, which were done when the pattern for using
RenderFrameImpl was not clear yet.