Refactor Web Notification permission requests.

Refactor Web Notification permission requests.

As part of supporting Web Notifications in workers, only the ability to
request permission will be done through a WebFrame, all other operations
will be routed through Platform.

This patch introduces the NotificationPermissionDispatcher, (lazy) member
of RenderFrameImpl, handling that sole responsibility. The NPD will
replace the existing NotificationProvider in its entirety. However, while
moving functionality around, we need to maintain the existing code paths
and thus use this opportunity to rename DesktopNotifications to a more
generic PlatformNotifications, since they'll also be usable on Android.

When a website currently requests permission to display Web Notifications,
we show an infobar to the user, then send a message (without result!) to
the renderer process, where a synchronous IPC to the browser process will
fire in order to find out whether permission was actually granted. This
patch includes whether permission was granted in the first message.

BUG=392187, 398045
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286321

[Michael van Ouwerkerk, 2014-07-10 10:53:11]

lgtm with nit

https://codereview.chromium.org/381633005/diff/1/content/renderer/notificatio...
File content/renderer/notification_permission_dispatcher.cc (right):

https://codereview.chromium.org/381633005/diff/1/content/renderer/notificatio...
content/renderer/notification_permission_dispatcher.cc:27: routing_id(),
GURL(origin.toString()), request_id));
Where is the check for origin.toString() == "null"?

[Peter Beverloo, 2014-07-15 16:33:30]

+stevenjb for s/DesktopNotification/PlatformNotification/, and general
Notification ownership.
+jochen for content/

I'll add an IPC reviewer once you're happy.

A design document is available here:
 
https://docs.google.com/a/chromium.org/document/d/1LfVhMW7k4QuVy-fT2xgqrtiQxd...

Adding of testing infrastructure is done here:
  https://codereview.chromium.org/395883002/

Enabling this path is done here:
  https://codereview.chromium.org/393863002/

Obviously any old, unused code will be removed afterwards.

https://codereview.chromium.org/381633005/diff/1/content/renderer/notificatio...
File content/renderer/notification_permission_dispatcher.cc (right):

https://codereview.chromium.org/381633005/diff/1/content/renderer/notificatio...
content/renderer/notification_permission_dispatcher.cc:27: routing_id(),
GURL(origin.toString()), request_id));
On 2014/07/10 10:53:11, Michael van Ouwerkerk wrote:
> Where is the check for origin.toString() == "null"?

When does that occur? I wonder why we haven't had a need for such a check so
far, and will investigate.

[stevenjb, 2014-07-15 19:15:55]

-> mukai@ who has been working on notifications more recently than I.

[Jun Mukai, 2014-07-15 21:49:32]

I think stevenjb is still more familiar with the permission code and ipc, but
anyways.

https://codereview.chromium.org/381633005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/381633005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:952:
blink::WebNotificationPermissionAllowed;
Should this be Denied?  Allowing notifications if compiled with
ENABLE_NOTIFICATIONS=false doesn't sound correct.

[Peter Beverloo, 2014-07-15 21:53:48]

https://codereview.chromium.org/381633005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/381633005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:952:
blink::WebNotificationPermissionAllowed;
On 2014/07/15 21:49:32, Jun Mukai wrote:
> Should this be Denied?  Allowing notifications if compiled with
> ENABLE_NOTIFICATIONS=false doesn't sound correct.

I'm merely copying the logic in
RenderMessageFilter::OnCheckNotificationPermission. It's not uncommon to default
features to allowed in content, but I agree that Denied would make more sense
here. I'll change that in the morning.

[stevenjb, 2014-07-15 21:59:50]

Hmm, I really don't know anything about notification permissions, so we may need
to add someone else to review this.

+miket@ - any idea who might be a good reviewer for this?

[Peter Beverloo, 2014-07-15 22:09:01]

+Andrew who might also have opinions.

I'd argue that this patch comes closer to content/ architecture, which has to
change in order to support Notifications in non-Document contexts, most notably
Service Workers.

The majority of this patch duplicates the current permission code, as we'll need
to support both paths until Blink has switched (patch for which is pending),
while cleaning up IPC and the old code.

[jochen (gone - plz use gerrit), 2014-07-17 09:32:47]

fyi, https://codereview.chromium.org/398163008/

[Jun Mukai, 2014-07-22 00:05:48]

hmm, no one so far replied, which may indicate no one remembers the strong
reason why.
I'm okay if it's just a copy of the old code, although I'm a bit curious what
will happen the default value is denied.

[jochen (gone - plz use gerrit), 2014-07-22 11:35:24]

lgtm

https://codereview.chromium.org/381633005/diff/1/content/renderer/notificatio...
File content/renderer/notification_permission_dispatcher.cc (right):

https://codereview.chromium.org/381633005/diff/1/content/renderer/notificatio...
content/renderer/notification_permission_dispatcher.cc:27: routing_id(),
GURL(origin.toString()), request_id));
On 2014/07/15 16:33:30, Peter Beverloo wrote:
> On 2014/07/10 10:53:11, Michael van Ouwerkerk wrote:
> > Where is the check for origin.toString() == "null"?
> 
> When does that occur? I wonder why we haven't had a need for such a check so
> far, and will investigate.

when you run from a file URL (or any other "unique" origin)

[Peter Beverloo, 2014-07-28 16:40:46]

On 2014/07/22 00:05:48, Jun Mukai wrote:
> hmm, no one so far replied, which may indicate no one remembers the strong
> reason why.
> I'm okay if it's just a copy of the old code, although I'm a bit curious what
> will happen the default value is denied.

I captured that question (with an explanation and CL) in
http://crbug.com/398070.

On 2014/07/22 11:35:24, jochen wrote:
> On 2014/07/15 16:33:30, Peter Beverloo wrote:
> > On 2014/07/10 10:53:11, Michael van Ouwerkerk wrote:
> > > Where is the check for origin.toString() == "null"?
> > 
> > When does that occur? I wonder why we haven't had a need for such a check so
> > far, and will investigate.
> 
> when you run from a file URL (or any other "unique" origin)

Ack, thanks! I've tracked this in http://crbug.com/398077 as the existing
code-path doesn't check for this either, and will make sure that we'll match
what other, similar APIs are doing here.

[Peter Beverloo, 2014-07-28 16:45:13]

+palmer for IPC.

The duplication will be gone promply. To clarify:

- This CL depends on CL 405633002 (reviewed).
- -- CL 395883002 is needed for layout test support (reviewed).
- CL 393863002 will switch Blink over (under review).
- Duplicated code will be removed.

[palmer, 2014-07-28 18:55:51]

lgtm

[Peter Beverloo, 2014-07-29 19:00:11]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-07-29 19:02:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peter@chromium.org/381633005/80001

[commit-bot: I haz the power, 2014-07-29 22:40:13]

Change committed as 286321