Plumb Push API permission request through Blink using Frame.

Plumb Push API permission request through Blink using Frame.

This follows the pattern of NotificationPermissionClient.

This is part of a series of changes in both Blink and Chromium.

BUG=389194
TEST=PushMessagingBrowserTest*
TEST=LayoutTests/push_messaging/*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184923

[Michael van Ouwerkerk, 2014-11-06 14:07:13]

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

[Michael van Ouwerkerk, 2014-11-06 14:07:14]

Hi Mike, I think you reviewed a bunch of similar Notifications changes for Peter
recently. Could you please take a look at this one?

[Mike West, 2014-11-06 14:21:20]

Looks pretty reasonable.

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

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:58: if (permissionClient)
Do you expect there to be execution contexts without PushPermissionClients?

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

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:21: if
(!context->isDocument())
Hrm. Isn't this sort of thing supposed to work inside workers?

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:27: if
(!document->frame()->isLocalFrame())
Can you add a FIXME to support RemoteFrames? (Or potentially assert that this
isn't a remote frame; it's not clear that we'd actually want to support that
case).

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushPermissionRequestCallback.h (right):

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionRequestCallback.h:25:
PushPermissionRequestCallback(PushManager*, WebPushClient*,
PassRefPtr<ScriptPromiseResolver>, WebServiceWorkerProvider*);
Can you document the ownership properties here?

[Michael van Ouwerkerk, 2014-11-06 14:59:46]

Thanks Mike! Please take another look.

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

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.cpp:58: if (permissionClient)
On 2014/11/06 14:21:20, Mike West wrote:
> Do you expect there to be execution contexts without PushPermissionClients?

Yes, it will not be available in service workers. A frame is required as
PushPermissionClient is a supplement of LocalFrame.

Until I make the corresponding Chromium side changes and finish wiring this all
up, even for document contexts this will be null.

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

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:21: if
(!context->isDocument())
On 2014/11/06 14:21:20, Mike West wrote:
> Hrm. Isn't this sort of thing supposed to work inside workers?

No, this is exactly the part that cannot work inside a service worker, because
there is no direct association with a frame. PushPermissionClient is a
supplement of LocalFrame. A frame is required to display the permission request
ui (an infobar) to the user.

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:27: if
(!document->frame()->isLocalFrame())
On 2014/11/06 14:21:20, Mike West wrote:
> Can you add a FIXME to support RemoteFrames? (Or potentially assert that this
> isn't a remote frame; it's not clear that we'd actually want to support that
> case).

Isn't returning null sufficient? I'm not sure we'd want to support this with
remote frames.

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushPermissionRequestCallback.h (right):

https://codereview.chromium.org/707833002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionRequestCallback.h:25:
PushPermissionRequestCallback(PushManager*, WebPushClient*,
PassRefPtr<ScriptPromiseResolver>, WebServiceWorkerProvider*);
On 2014/11/06 14:21:20, Mike West wrote:
> Can you document the ownership properties here?

Done.

[Mike West, 2014-11-06 15:37:15]

Alright. Thanks for taking another pass. LGTM % nits.

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

https://codereview.chromium.org/707833002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:22: return 0;
s/0/nullptr/g

https://codereview.chromium.org/707833002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:28: return 0;
Ditto.

[Michael van Ouwerkerk, 2014-11-06 15:41:13]

Thanks Mike!

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

https://codereview.chromium.org/707833002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:22: return 0;
On 2014/11/06 15:37:14, Mike West wrote:
> s/0/nullptr/g

Done.

https://codereview.chromium.org/707833002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushPermissionClient.cpp:28: return 0;
On 2014/11/06 15:37:14, Mike West wrote:
> Ditto.

Done.

[Michael van Ouwerkerk, 2014-11-06 15:41:37]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-11-06 15:42:18]

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

[commit-bot: I haz the power, 2014-11-06 16:44:18]

Committed patchset #6 (id:100001) as 184923