Move push client to frame.

Move push client to frame.

Depends on https://codereview.chromium.org/720443002/

BUG=304341, 350378
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185228

[Michael van Ouwerkerk, 2014-10-29 10:57:22]

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

[Michael van Ouwerkerk, 2014-10-29 10:57:23]

Peter, could you take a look please?

[Peter Beverloo, 2014-10-29 11:25:07]

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (right):

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:18: class PushController final :
public NoBaseWillBeGarbageCollected<PushController>, public
WillBeHeapSupplement<LocalFrame> {
This will only be used for requesting permission, right? You should probably
call it PushPermissionController to clarify its responsibilities. (Which I
should do for Notifications as well.)

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:28: WebPushClient* client() const
{ return m_client; }
nit: this should be private.

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:36: if (!document->domWindow() ||
!document->page() || !document->frame())
Remove the "!document->page()" check.

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:44: ASSERT(client);
We're going to be hitting this assert when using layout tests, aren't we?

https://codereview.chromium.org/687933002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/687933002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1588: providePushControllerTo(*m_frame,
m_client->pushClient());
Hmm. The WebPushClient should probably be moved to public/Web/ now that it's
being retrieved (exclusively) through the WebFrameClient, using a proxy in
Source/web/ like all the other features here are doing.

[Michael van Ouwerkerk, 2014-11-07 11:02:58]

mvanouwerkerk@chromium.org changed reviewers:
- peter@chromium.org

[Michael van Ouwerkerk, 2014-11-11 17:20:06]

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (right):

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:18: class PushController final :
public NoBaseWillBeGarbageCollected<PushController>, public
WillBeHeapSupplement<LocalFrame> {
On 2014/10/29 11:25:07, Peter Beverloo wrote:
> This will only be used for requesting permission, right? You should probably
> call it PushPermissionController to clarify its responsibilities. (Which I
> should do for Notifications as well.)

This will also plumb through a sender id from the manifest.

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:28: WebPushClient* client() const
{ return m_client; }
On 2014/10/29 11:25:07, Peter Beverloo wrote:
> nit: this should be private.

Actually, I think this method is not needed at all.

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:36: if (!document->domWindow() ||
!document->page() || !document->frame())
On 2014/10/29 11:25:07, Peter Beverloo wrote:
> Remove the "!document->page()" check.

Done.

https://codereview.chromium.org/687933002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:44: ASSERT(client);
On 2014/10/29 11:25:07, Peter Beverloo wrote:
> We're going to be hitting this assert when using layout tests, aren't we?

No, there is already an implementation of pushClient() in RenderFrameImpl

https://codereview.chromium.org/687933002/diff/20001/Source/web/WebLocalFrame...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/687933002/diff/20001/Source/web/WebLocalFrame...
Source/web/WebLocalFrameImpl.cpp:1588: providePushControllerTo(*m_frame,
m_client->pushClient());
On 2014/10/29 11:25:07, Peter Beverloo wrote:
> Hmm. The WebPushClient should probably be moved to public/Web/ now that it's
> being retrieved (exclusively) through the WebFrameClient, using a proxy in
> Source/web/ like all the other features here are doing.

I don't think that's necessary. It's more complex plumbing, for no strong
reason. I think if anything, the other features should be updated to the new
pattern.

[Michael van Ouwerkerk, 2014-11-11 17:20:44]

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

[Michael van Ouwerkerk, 2014-11-11 17:20:44]

Adding Peter back as reviewer, as I'm resurrecting this patch. Peter, could you
please take a look?

[Peter Beverloo, 2014-11-11 18:23:28]

https://codereview.chromium.org/687933002/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.cpp (right):

https://codereview.chromium.org/687933002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.cpp:26: return
controller->m_client;
nit: Please just keep the client() method, make it private if you want to.

https://codereview.chromium.org/687933002/diff/60001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/687933002/diff/60001/public/web/WebFrameClien...
public/web/WebFrameClient.h:82: class WebPushClient;
The PushClient currently has functionality for checking permission and
registering for push. Exactly which parts of this are going to be duplicated for
Frame vs. Workers?

https://codereview.chromium.org/687933002/diff/60001/public/web/WebFrameClien...
public/web/WebFrameClient.h:594: // Push API
------------------------------------------------------------
You already have a Push API block around line 333.

[Michael van Ouwerkerk, 2014-11-12 12:05:25]

Thanks Peter! Please take another look.

https://codereview.chromium.org/687933002/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.cpp (right):

https://codereview.chromium.org/687933002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.cpp:26: return
controller->m_client;
On 2014/11/11 18:23:28, Peter Beverloo wrote:
> nit: Please just keep the client() method, make it private if you want to.

Done.

https://codereview.chromium.org/687933002/diff/60001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/687933002/diff/60001/public/web/WebFrameClien...
public/web/WebFrameClient.h:82: class WebPushClient;
On 2014/11/11 18:23:28, Peter Beverloo wrote:
> The PushClient currently has functionality for checking permission and
> registering for push. Exactly which parts of this are going to be duplicated
for
> Frame vs. Workers?

There will be no duplication. Requesting permission will hit this client, and
hop to the browser process to do that. Fetching the sender id from the manifest
will also hit this client, but will stay in the renderer to do that.

After that: register, unregister, getRegistration, and hasPermission will flow
through platform. The register call will use the sender id as input. It will be
rejected in the browser process if it turns out no permission was granted. This
way, all these calls can be run from both document and worker contexts.

https://codereview.chromium.org/687933002/diff/60001/public/web/WebFrameClien...
public/web/WebFrameClient.h:594: // Push API
------------------------------------------------------------
On 2014/11/11 18:23:28, Peter Beverloo wrote:
> You already have a Push API block around line 333.

Done.

[Michael van Ouwerkerk, 2014-11-12 13:40:24]

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

[Michael van Ouwerkerk, 2014-11-12 13:40:25]

Hi Mike, could you take a look as owner please? Thanks!

[Mike West, 2014-11-12 14:03:41]

mkwst@chromium.org changed required reviewers:
+ peter@chromium.org

[Mike West, 2014-11-12 14:03:42]

Looks reasonable, but please wait for Peter to be happy before landing it.

LGTM.

[Peter Beverloo, 2014-11-12 14:24:03]

lgtm

https://codereview.chromium.org/687933002/diff/80001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/687933002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:49: // delete WebPushClient and
usage such as this one.
Is this FIXME still relevant?

[Michael van Ouwerkerk, 2014-11-12 14:33:01]

https://codereview.chromium.org/687933002/diff/80001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/687933002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:49: // delete WebPushClient and
usage such as this one.
On 2014/11/12 14:24:03, Peter Beverloo wrote:
> Is this FIXME still relevant?

No, but that is not really related to the current change. I think you know what
the plan is, we talked about it in some detail.

[Michael van Ouwerkerk, 2014-11-12 17:13:02]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-11-12 17:13:09]

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

[commit-bot: I haz the power, 2014-11-12 17:47:09]

Committed patchset #5 (id:80001) as 185228