Request the icon of a Web Notification in the renderer process.

Request the icon of a Web Notification in the renderer process.

This will allow us to move the URL fetching logic out of /c/b/notifications/.
Mind that this code will soon move to //content/child/, so we can't
re-use some of the existing image fetching logic in //content/renderer/.

BUG=

Committed: https://crrev.com/d17f01a45c92d94869750d7cefb23aa73ed9514d
Cr-Commit-Position: refs/heads/master@{#299685}

[Peter Beverloo, 2014-09-16 20:09:43]

peter@chromium.org changed reviewers:
+ dewittj@chromium.org, mukai@chromium.org

[Peter Beverloo, 2014-09-16 20:09:43]

Not ready for review yet (it needs it rough edges smoothed, especially in
notification_provider.cc), but just making sure that you're aware of it.

I wonder if it makes sense to move the notification image / button icon
downloading code to another place. It would remove quite a lot of code from
MessageCenterNotificationManager.

[dewittj, 2014-09-17 16:36:20]

Also may want to think about capping the amount of image data that can be sent
to the browser process; we do this in extensions land.

https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:162:
gfx::Image::CreateFrom1xBitmap(params.icon),
I think that today, this will work for 1x and 2x displays as we've written the
views code but it is still risky; since the image will be marked as having only
a 1x representation.

I realize that this is a limitation of the current API, but possibly good to put
a TODO in here, to fix the image creation someday.

[Peter Beverloo, 2014-09-17 16:39:27]

On 2014/09/17 16:36:20, dewittj wrote:
> Also may want to think about capping the amount of image data that can be sent
> to the browser process; we do this in extensions land.

Yes, I'm considering to resize the image to a reasonable per-platform maximum as
part of this decoding step, but haven't figured out yet how we'd decide on such
a limitation. Where/how do extensions decide on this limit? 256x256?

> 
>
https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
> File chrome/browser/notifications/desktop_notification_service.cc (right):
> 
>
https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
> chrome/browser/notifications/desktop_notification_service.cc:162:
> gfx::Image::CreateFrom1xBitmap(params.icon),
> I think that today, this will work for 1x and 2x displays as we've written the
> views code but it is still risky; since the image will be marked as having
only
> a 1x representation.
> 
> I realize that this is a limitation of the current API, but possibly good to
put
> a TODO in here, to fix the image creation someday.

Agreed. This is something that'll come up on Android and Chrome OS more than on
desktop platforms (except for Mac retina).

[dewittj, 2014-09-17 16:54:32]

On 2014/09/17 16:39:27, Peter Beverloo wrote:

> Yes, I'm considering to resize the image to a reasonable per-platform maximum
as
> part of this decoding step, but haven't figured out yet how we'd decide on
such
> a limitation. Where/how do extensions decide on this limit? 256x256?

We actually detect the highest scale factor possible on the current platform and
then use the constants in message center to determine the max device pixel size.
 Then extensions will do a downscaling step if it's too big. 
src/chrome/renderer/extensions/notifications_native_handler.cc is how we pass
this data to the renderer, and notifications_custom_bindings.js uses that while
it's doing the download in javascript.

This is a hard dependency on message_center, but I imagine we could refactor
NotificationUIManager to report the sizes of stuff.  Alternately we could just
#define some size that's unlikely to be exceeded and then do that.

[Jun Mukai, 2014-09-17 16:57:47]

https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:162:
gfx::Image::CreateFrom1xBitmap(params.icon),
On 2014/09/17 16:36:20, dewittj wrote:
> I think that today, this will work for 1x and 2x displays as we've written the
> views code but it is still risky; since the image will be marked as having
only
> a 1x representation.
> 
> I realize that this is a limitation of the current API, but possibly good to
put
> a TODO in here, to fix the image creation someday.

Using CreateFrom1xBitmap is not bad by itself, it will automatically rescaled in
Views size IIRC.  However, that means the scale could be blurry in some
situation, and specifying larger image for hidpi device may cause weird effect
of rescaling and shrinking (I haven't checked it though).

More thinking deeply, specifying single bitmap itself could be a bad idea by
itself because we have no idea what is the best image in some environment, like
Retina Macbook or Chromebook Pixel with normal external display -- the
notification card appears on both screens. But to address that issue, we need to
change the notification API to support such situation.

Until then, it would be good to put a TODO here.

[Peter Beverloo, 2014-09-18 16:54:36]

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

[Peter Beverloo, 2014-09-18 16:54:37]

This should be ready for review now. I do expect a few iterations.

+mukai/dewittj for notifications
+jamesr for content/renderer/

I'll add a security reviewer for IPC once you're happy.

https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:162:
gfx::Image::CreateFrom1xBitmap(params.icon),
On 2014/09/17 16:57:47, Jun Mukai wrote:
> On 2014/09/17 16:36:20, dewittj wrote:
> > I think that today, this will work for 1x and 2x displays as we've written
the
> > views code but it is still risky; since the image will be marked as having
> only
> > a 1x representation.
> > 
> > I realize that this is a limitation of the current API, but possibly good to
> put
> > a TODO in here, to fix the image creation someday.
> 
> Using CreateFrom1xBitmap is not bad by itself, it will automatically rescaled
in
> Views size IIRC.  However, that means the scale could be blurry in some
> situation, and specifying larger image for hidpi device may cause weird effect
> of rescaling and shrinking (I haven't checked it though).
> 
> More thinking deeply, specifying single bitmap itself could be a bad idea by
> itself because we have no idea what is the best image in some environment,
like
> Retina Macbook or Chromebook Pixel with normal external display -- the
> notification card appears on both screens. But to address that issue, we need
to
> change the notification API to support such situation.
> 
> Until then, it would be good to put a TODO here.

I agree that it would be good for the Notification API to support images of
different resolution (there's Android devices with 3x density out there), and
will pursue this for the HTML5 API.

I've added a TODO here for now.

[dewittj, 2014-09-18 17:46:50]

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:169: // appropriate
resolution, doing so immediately should result in icons of
What do you mean immediately?

Also gfx::Image does provide a way to set multiple scale factors, it's:

gfx::ImageSkia skia(gfx::ImageSkiaRep(icon_bitmap, 1.0f));
skia.AddRepresentation(gfx::ImageSkiaRep(icon_2x_bitmap, 2.0f));
...
gfx::Image result(skia)

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:134: if (completed_)
Is this the correct failure behavior?  In extensions we don't allow
notifications without an icon.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:228:
RemovePendingNotification(notification);
Is this correct?  (I'm unfamiliar with blink code)

but it seems like the notification should continue to be shown even if the
object is destroyed before the icon downloads.  Possibly need to increment ref
count of the object while it's downloading the icon?

[Mike West, 2014-09-18 19:48:41]

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

[Mike West, 2014-09-18 19:48:43]

A few drive-by comments. Hope they're helpful.

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:174: params.body,
This is a huge signature.

It looks like you're passing in a number of `params` attributes. Perhaps a
future refactoring could just pass in a const ref to the params object itself?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:48: // Downloads |image_url| and
invokes |callback| with the decoded SkBitmap when
There's not an 'image_url' parameter anywhere. Perhaps "... the notification's
image"?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:94: WebURLRequest
request(notification_.iconURL());
Please set the request's context. It looks like it ought to be
'WebURLRequest::RequestContextImage'.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:96:
loader_.reset(blink::Platform::current()->createURLLoader());
Should this be an AssociatedURLLoader? I'd imagine that an extension's CSP
should govern whether or not it could request a particular URL as a notification
icon, for instance.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:104: loader_->cancel();
You don't do anything with the contents of |buffer_| here. It seems like you
should clear it?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:138: completed_ = true;
Again, it seems like you should do something with |buffer_| here?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
File content/renderer/notification_provider.h (right):

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.h:53: // A vector tracking notifications
which' icon is still being downloaded.
Nit: Stray '. Perhaps "Vector containing notification whose icons are not yet
downloaded."

[Peter Beverloo, 2014-09-19 14:15:48]

Thanks for the reviews! I uploaded a new iteration.

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:169: // appropriate
resolution, doing so immediately should result in icons of
On 2014/09/18 17:46:49, dewittj wrote:
> What do you mean immediately?
> 
> Also gfx::Image does provide a way to set multiple scale factors, it's:
> 
> gfx::ImageSkia skia(gfx::ImageSkiaRep(icon_bitmap, 1.0f));
> skia.AddRepresentation(gfx::ImageSkiaRep(icon_2x_bitmap, 2.0f));
> ...
> gfx::Image result(skia)

Ah, that's good to know, however, I don't think that's how we should implement
this when we support multiple image resolutions for Web Notifications, as it
would imply requesting all the images. Instead, we should download an SkBitmap
of the right format from the renderer process, and pass along the used scale
factor so that we can use that ImageSkiaRep constructor.

I've updated the comment.

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:174: params.body,
On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> This is a huge signature.
> 
> It looks like you're passing in a number of `params` attributes. Perhaps a
> future refactoring could just pass in a const ref to the params object itself?

We're trying to trim down the number of Notification constructors to make
upcoming refactorings a bit easier. There aren't many call-sites, so the noise
is fine for now.

In the longer run we could consider doing this, but the struct is going to
include other data which does not relate to the Notification class as well.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:48: // Downloads |image_url| and
invokes |callback| with the decoded SkBitmap when
On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> There's not an 'image_url' parameter anywhere. Perhaps "... the notification's
> image"?

Done.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:94: WebURLRequest
request(notification_.iconURL());
On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> Please set the request's context. It looks like it ought to be
> 'WebURLRequest::RequestContextImage'.

Done.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:96:
loader_.reset(blink::Platform::current()->createURLLoader());
On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> Should this be an AssociatedURLLoader? I'd imagine that an extension's CSP
> should govern whether or not it could request a particular URL as a
notification
> icon, for instance.

Notifications won't be associated with a RenderFrame/RenderView anymore (this
class is going to move to //content/child/), and there is no frame to associate
it with.

Does CSP apply to requests done from service workers? If so, how do I get and
apply the constraints?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:104: loader_->cancel();
On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> You don't do anything with the contents of |buffer_| here. It seems like you
> should clear it?

buffer_ is unlikely to contain sensitive data, and since it's a member of
ImageDownloader it gets cleared when the objects itself gets destroyed (which
happens immediately after we invoke the callback, or when Cancel() gets called).

Do you want me to clear it manually anyway?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:134: if (completed_)
On 2014/09/18 17:46:49, dewittj wrote:
> Is this the correct failure behavior?  In extensions we don't allow
> notifications without an icon.

The Web Notification API doesn't imply restrictions on this. It also raises
questions about what to do when the image is specified, but couldn't be decoded
successfully.

I don't think we're changing behavior here. It's worth revisiting when we start
discussions about supporting icons of multiple resolutions, though.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:138: completed_ = true;
On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> Again, it seems like you should do something with |buffer_| here?

Done.

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:228:
RemovePendingNotification(notification);
On 2014/09/18 17:46:49, dewittj wrote:
> Is this correct?  (I'm unfamiliar with blink code)
> 
> but it seems like the notification should continue to be shown even if the
> object is destroyed before the icon downloads.  Possibly need to increment ref
> count of the object while it's downloading the icon?

We'd only cancel showing the notification if the image is still being requested,
which is extremely unlikely for document contexts.

We're going to have to be smarter with this when enabling notifications to be
displayed from workers, given that a scenario like the following isn't unlikely:

service_worker.js
=========================
onpush = function(event) {
  new Notification('You have received a message!', {
    message: event.data.fullName + ' says hello.',
    icon: 'https://my-site.com/icon.png',

    serviceWorker: serviceWorkerRegistration
  });
};
=========================

In which case the Push event would release their reference on the SW immediately
after the event finished invoking, which is likely to kill it.

Doing this cleanly requires us to separate the notification data from the
delegate, which I plan on doing as part of an upcoming Blink refactoring for Web
Worker support (I uploaded a WIP here:
https://codereview.chromium.org/584023002).

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
File content/renderer/notification_provider.h (right):

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.h:53: // A vector tracking notifications
which' icon is still being downloaded.
On 2014/09/18 19:48:43, Mike West (OOO until 24th) wrote:
> Nit: Stray '. Perhaps "Vector containing notification whose icons are not yet
> downloaded."

It's not stray, it indicates ownership and doesn't include the "s" because
pronouncing the word ends with an s-like tone.

I do prefer "whose". Changed :-).

[Mike West, 2014-09-19 15:00:28]

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:96:
loader_.reset(blink::Platform::current()->createURLLoader());
On 2014/09/19 14:15:48, Peter Beverloo wrote:
> On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> > Should this be an AssociatedURLLoader? I'd imagine that an extension's CSP
> > should govern whether or not it could request a particular URL as a
> notification
> > icon, for instance.
> 
> Notifications won't be associated with a RenderFrame/RenderView anymore (this
> class is going to move to //content/child/), and there is no frame to
associate
> it with.
> 
> Does CSP apply to requests done from service workers? If so, how do I get and
> apply the constraints?

Hrm. That's a bit surprising. I thought notifications were tied to an
ExecutionContext, so that they had something to hook onto for message passing. I
assumed that meant that they were tied to a RenderFrameHost on the //content
side. Is that not the case? How is that going to work in the future?

https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
content/renderer/notification_provider.cc:104: loader_->cancel();
On 2014/09/19 14:15:48, Peter Beverloo wrote:
> On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> > You don't do anything with the contents of |buffer_| here. It seems like you
> > should clear it?
> 
> buffer_ is unlikely to contain sensitive data, and since it's a member of
> ImageDownloader it gets cleared when the objects itself gets destroyed (which
> happens immediately after we invoke the callback, or when Cancel() gets
called).
> 
> Do you want me to clear it manually anyway?

My worry was that the load could cancel, get restarted somehow, and end up with
corrupted buffer. Less a security issue, more a correctness issue. It's not a
huge deal if we're always going to tear down the object after calling Cancel.
Could you add a comment to that effect in the header so that it's clear that
that's the intended usage?

[Peter Beverloo, 2014-09-19 15:28:04]

I'll pull out all the constructor changes from the next iteration of this CL,
it's too noisy and should be done separately if we want to do it at all.

On 2014/09/19 15:00:28, Mike West (OOO until 24th) wrote:
>
https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
> File content/renderer/notification_provider.cc (right):
> 
>
https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
> content/renderer/notification_provider.cc:96:
> loader_.reset(blink::Platform::current()->createURLLoader());
> On 2014/09/19 14:15:48, Peter Beverloo wrote:
> > On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> > > Should this be an AssociatedURLLoader? I'd imagine that an extension's CSP
> > > should govern whether or not it could request a particular URL as a
> > notification
> > > icon, for instance.
> > 
> > Notifications won't be associated with a RenderFrame/RenderView anymore
(this
> > class is going to move to //content/child/), and there is no frame to
> associate
> > it with.
> > 
> > Does CSP apply to requests done from service workers? If so, how do I get
and
> > apply the constraints?
> 
> Hrm. That's a bit surprising. I thought notifications were tied to an
> ExecutionContext, so that they had something to hook onto for message passing.
I
> assumed that meant that they were tied to a RenderFrameHost on the //content
> side. Is that not the case? How is that going to work in the future?

They are, but an ExecutionContext is not always associated with a Document.

I see that we associate CSP data with Workers as part of
{Dedicated,Shared,Service}WorkerGlobalScope::create(), so we could do the CSP
check in Blink, and clear out icon_url (or reject) if it isn't allowed per the
policy? If I understand correctly, the policy would be set based on the response
headers for the worker.

>
https://codereview.chromium.org/554213003/diff/40001/content/renderer/notific...
> content/renderer/notification_provider.cc:104: loader_->cancel();
> On 2014/09/19 14:15:48, Peter Beverloo wrote:
> > On 2014/09/18 19:48:42, Mike West (OOO until 24th) wrote:
> > > You don't do anything with the contents of |buffer_| here. It seems like
you
> > > should clear it?
> > 
> > buffer_ is unlikely to contain sensitive data, and since it's a member of
> > ImageDownloader it gets cleared when the objects itself gets destroyed
(which
> > happens immediately after we invoke the callback, or when Cancel() gets
> called).
> > 
> > Do you want me to clear it manually anyway?
> 
> My worry was that the load could cancel, get restarted somehow, and end up
with
> corrupted buffer. Less a security issue, more a correctness issue. It's not a
> huge deal if we're always going to tear down the object after calling Cancel.
> Could you add a comment to that effect in the header so that it's clear that
> that's the intended usage?

Done. There's also DCHECKs guarding against re-use (on loader_).

[Mike West, 2014-09-19 15:41:41]

On 2014/09/19 15:28:04, Peter Beverloo wrote:
> I'll pull out all the constructor changes from the next iteration of this CL,
> it's too noisy and should be done separately if we want to do it at all.

That's up to you, I certainly won't insist on it.

> On 2014/09/19 15:00:28, Mike West (OOO until 24th) wrote:
> > Hrm. That's a bit surprising. I thought notifications were tied to an
> > ExecutionContext, so that they had something to hook onto for message
passing.
> I
> > assumed that meant that they were tied to a RenderFrameHost on the //content
> > side. Is that not the case? How is that going to work in the future?
> 
> They are, but an ExecutionContext is not always associated with a Document.

CSP is associated with a SecurityContext, not a Document. Any ExecutionContext
should have an associated CSP object.

> I see that we associate CSP data with Workers as part of
> {Dedicated,Shared,Service}WorkerGlobalScope::create(), so we could do the CSP
> check in Blink, and clear out icon_url (or reject) if it isn't allowed per the
> policy?

We could certainly do that. Right now, CSP is taken care of in ResourceFetcher.
Ideally, we'll move it up into //content after the repositories merge.

> If I understand correctly, the policy would be set based on the response
> headers for the worker.

Yeah. This isn't completely implemented yet; right now, Workers generally
inherit CSP from their creator Document. That needs to change, but I haven't
done it yet.

> > On 2014/09/19 14:15:48, Peter Beverloo wrote:
> > My worry was that the load could cancel, get restarted somehow, and end up
> with
> > corrupted buffer. Less a security issue, more a correctness issue. It's not
a
> > huge deal if we're always going to tear down the object after calling
Cancel.
> > Could you add a comment to that effect in the header so that it's clear that
> > that's the intended usage?
> 
> Done. There's also DCHECKs guarding against re-use (on loader_).

Yup, I see those now. Thanks!

[jamesr, 2014-09-22 22:09:46]

In a renderer context why is this load not associated with any particular
frame/document?  Should window.stop() stop loading of this image?  Should the
spinner spin while it is loading?  Which of these things are true today?

[Peter Beverloo, 2014-09-23 16:25:18]

PTAL.

On 2014/09/22 22:09:46, jamesr wrote:
> In a renderer context why is this load not associated with any particular
> frame/document?  Should window.stop() stop loading of this image?  Should the
> spinner spin while it is loading?  Which of these things are true today?

This code will soon move to //content/child/ as Notifications will be exposed in
Web Workers as well (most notably Service Workers), and cannot depend on the
existence of either. There's little point in implementing it in
//content/renderer/ context now, and then re-implementing it later this week for
//content/child/.

[jamesr, 2014-09-23 16:32:23]

On 2014/09/23 16:25:18, Peter Beverloo wrote:
> PTAL.
> 
> On 2014/09/22 22:09:46, jamesr wrote:
> > In a renderer context why is this load not associated with any particular
> > frame/document?  Should window.stop() stop loading of this image?  Should
the
> > spinner spin while it is loading?  Which of these things are true today?
> 
> This code will soon move to //content/child/ as Notifications will be exposed
in
> Web Workers as well (most notably Service Workers), and cannot depend on the
> existence of either. There's little point in implementing it in
> //content/renderer/ context now, and then re-implementing it later this week
for
> //content/child/.

What is the renderer behavior without this patch?

[Peter Beverloo, 2014-09-23 16:43:34]

On 2014/09/23 16:32:23, jamesr wrote:
> On 2014/09/23 16:25:18, Peter Beverloo wrote:
> > PTAL.
> > 
> > On 2014/09/22 22:09:46, jamesr wrote:
> > > In a renderer context why is this load not associated with any particular
> > > frame/document?  Should window.stop() stop loading of this image?  Should
> the
> > > spinner spin while it is loading?  Which of these things are true today?
> > 
> > This code will soon move to //content/child/ as Notifications will be
exposed
> in
> > Web Workers as well (most notably Service Workers), and cannot depend on the
> > existence of either. There's little point in implementing it in
> > //content/renderer/ context now, and then re-implementing it later this week
> for
> > //content/child/.
> 
> What is the renderer behavior without this patch?

If the IPC for requesting to show a notification arrives in the browser process,
the icon will be downloaded regardless of the rest of the lifetime of the page.
Concretely, window.stop() has no effect, neither has closing the page.

The only thing a page can do to cancel downloading an icon is cancelling it
through JavaScript, using:

=========================
var n = new Notification('Title', {
  icon: '//example.com/icon.png'
});
n.close();
=========================

This patch changes that a little bit. window.stop() won't affect the load (since
the loader is not associated), but closing the page before the icon has
downloaded will mean we'll drop the notification. This should be very rare, and
well within the flexibility the spec gives us.

[jamesr, 2014-09-24 04:34:48]

The fact that this is going to move to be more generic doesn't relax the
requirements for the content/renderer/ embedding.  If you think being loose with
the loading semantics is the best thing to do for the content/renderer/
embedding then OK, but don't compromise it in order to share more code with
other types of embeddings like workers.

[jamesr, 2014-09-24 04:39:54]

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:61: int encoded_data_length) OVERRIDE;
not lgtm.  You shouldn't use OVERRIDE on cross-repo interfaces or modifying the
interface becomes impossible

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:78: std::string buffer_;
std::string should only be for utf8 string data, not arbitrary bytes.  If you
want to buffer arbitrary bytes, use a vector<uint8_t> or something like that

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:115: DCHECK(data_length > 0);
DCHECK_GT is slightly better

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:159: base::Unretained(this))));
what makes the base::Unretained() here safe?

[dewittj, 2014-09-24 18:28:44]

chrome/browser/notifications lgtm

one other thing: I'm not so familiar with the IPC code; but do we need to do any
sanity checking on |icon| once it comes over to the browser process, since it
could be from a compromised renderer?

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/554213003/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:169: // appropriate
resolution, doing so immediately should result in icons of
On 2014/09/19 14:15:48, Peter Beverloo wrote:
> On 2014/09/18 17:46:49, dewittj wrote:
> > What do you mean immediately?
> > 
> > Also gfx::Image does provide a way to set multiple scale factors, it's:
> > 
> > gfx::ImageSkia skia(gfx::ImageSkiaRep(icon_bitmap, 1.0f));
> > skia.AddRepresentation(gfx::ImageSkiaRep(icon_2x_bitmap, 2.0f));
> > ...
> > gfx::Image result(skia)
> 
> Ah, that's good to know, however, I don't think that's how we should implement
> this when we support multiple image resolutions for Web Notifications, as it
> would imply requesting all the images. Instead, we should download an SkBitmap
> of the right format from the renderer process, and pass along the used scale
> factor so that we can use that ImageSkiaRep constructor.
> 
> I've updated the comment.

I don't think the "right format" is so simple, since it can change over time;
users can change the pixel density on their Chromebooks with a key combo!  But
still fine for now.

Also, I've been looking into this for the extension API, see
https://codereview.chromium.org/354733002/

[Peter Beverloo, 2014-10-01 19:34:08]

PTAL. All tests pass as well now.

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:61: int encoded_data_length) OVERRIDE;
On 2014/09/24 04:39:54, jamesr wrote:
> not lgtm.  You shouldn't use OVERRIDE on cross-repo interfaces or modifying
the
> interface becomes impossible

Erps, that's an oversight indeed. Fixed.

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:78: std::string buffer_;
On 2014/09/24 04:39:54, jamesr wrote:
> std::string should only be for utf8 string data, not arbitrary bytes.  If you
> want to buffer arbitrary bytes, use a vector<uint8_t> or something like that

There's quite some precedence for using an std::string here (e.g.
content::WebURLLoaderClientImpl which powers the ResourceFetcher), hence my
choice. I've changed it to be a vector.

Accessing the array on line 129 now uses "&buffer_[0]", as the .data() member of
a vector is a C++11 library function.

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:115: DCHECK(data_length > 0);
On 2014/09/24 04:39:54, jamesr wrote:
> DCHECK_GT is slightly better

Done.

https://codereview.chromium.org/554213003/diff/140001/content/renderer/notifi...
content/renderer/notification_provider.cc:159: base::Unretained(this))));
On 2014/09/24 04:39:54, jamesr wrote:
> what makes the base::Unretained() here safe?

The icon downloaders are owned in the |pending_notifications_| scoped vector, so
we're sure that all instances of IconDownloader will be destroyed before |this|
goes away. They're neither shared nor used outside of this class.

[Peter Beverloo, 2014-10-06 22:46:57]

James: Ping.

[jamesr, 2014-10-06 22:56:24]

content/renderer/ lgtm

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
content/renderer/notification_provider.cc:41: class
NotificationProvider::IconDownloader : public blink::WebURLLoaderClient {
put this in its own .h/.cc file.  It's too much code to slam into another .cc
and hiding it makes it impossible to unit test (which would probably be a good
idea, if not now then further down the line)

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
content/renderer/notification_provider.cc:177: CHECK(found);
what does failing this CHECK mean? is there a security property being tested
here that we need to fail in production over?

if this is just documenting an invariant that we expect to be true here, DCHECK
it. if this is needed for security it deserves a comment explaining what's going
on

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
content/renderer/notification_provider.cc:196: for (; iter !=
pending_notifications_.end(); ++iter) {
should probably be a const_iterator since you aren't going to modify it. or just
use auto.

[Peter Beverloo, 2014-10-13 19:09:29]

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

[Peter Beverloo, 2014-10-13 19:09:30]

All done.

+mkwst@ for general review.
+nasko@ for show_desktop_notification_params.h

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
File content/renderer/notification_provider.cc (right):

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
content/renderer/notification_provider.cc:41: class
NotificationProvider::IconDownloader : public blink::WebURLLoaderClient {
On 2014/10/06 22:56:24, jamesr wrote:
> put this in its own .h/.cc file.  It's too much code to slam into another .cc
> and hiding it makes it impossible to unit test (which would probably be a good
> idea, if not now then further down the line)

Moved to its own file. I've filed http://crbug.com/423039 to keep track of this.

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
content/renderer/notification_provider.cc:177: CHECK(found);
On 2014/10/06 22:56:24, jamesr wrote:
> what does failing this CHECK mean? is there a security property being tested
> here that we need to fail in production over?
> 
> if this is just documenting an invariant that we expect to be true here,
DCHECK
> it. if this is needed for security it deserves a comment explaining what's
going
> on

It means that display is being requested for a non-existing notification. The
Notification data provided by this getter will be send as part of an IPC, but
it's not possible to get here through any of the normal code-paths. Changed to
if+NOTREACHED.

https://codereview.chromium.org/554213003/diff/180001/content/renderer/notifi...
content/renderer/notification_provider.cc:196: for (; iter !=
pending_notifications_.end(); ++iter) {
On 2014/10/06 22:56:24, jamesr wrote:
> should probably be a const_iterator since you aren't going to modify it. or
just
> use auto.

pending_notifications_ is a ScopedVector, so erase() will actually modify the
values (delete them). erase() also only accepts an iterator. Using auto would
give us a NotificationIconLoader* instance, not an iterator.

[nasko, 2014-10-14 22:52:06]

IPC LGTM

[Peter Beverloo, 2014-10-15 13:41:53]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-10-15 13:42:18]

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

[commit-bot: I haz the power, 2014-10-15 15:14:31]

Committed patchset #13 (id:410001)

[commit-bot: I haz the power, 2014-10-15 15:15:42]

Patchset 13 (id:??) landed as
https://crrev.com/d17f01a45c92d94869750d7cefb23aa73ed9514d
Cr-Commit-Position: refs/heads/master@{#299685}