Use NotificationResources instead of a bare SkBitmap.

Use NotificationResources instead of a bare SkBitmap.

This prepares the notification feature plumbing for using multiple
resources when buttons can have icons. IPC messages cannot take more
than 5 arguments, so the idea is that all icon resources will be wrapped
in this struct.

BUG=581336
Committed: https://crrev.com/9a2f11e52fc01897767d686f3829cb4ced01bf47
Cr-Commit-Position: refs/heads/master@{#371765}

[Michael van Ouwerkerk, 2016-01-26 15:26:48]

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

[Michael van Ouwerkerk, 2016-01-26 15:26:48]

Peter, could you take a look please?

[Peter Beverloo, 2016-01-26 15:43:23]

lgtm

Please update the layout test plumbing as well.

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:469: //
TODO(peter): Icons for Web Notifications are currently always requested for
This is going to be an interesting TODO for us to think about as well.

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_notification_manager.cc
(right):

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:340:
content::NotificationResources());
s/content::// (you have a using declaration above)

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
File content/child/notifications/pending_notifications_tracker.cc (right):

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
content/child/notifications/pending_notifications_tracker.cc:11: #include
"content/public/common/notification_resources.h"
Something to think about: so far we've named _most_ things in the //content API
paltform_notification_* to distinguish between the signalling notification
system and this.

Going forth, with the former becoming a more prominent feature, and the latter
largely being deprecated, this isn't such a large problem anymore. I'm OK naming
it NotificationResources - it's much more concise.

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
File content/child/notifications/pending_notifications_tracker.h (right):

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
content/child/notifications/pending_notifications_tracker.h:72: const SkBitmap&
icon);
I imagine that you'll update these as a second step?

https://codereview.chromium.org/1634933006/diff/1/content/public/common/notif...
File content/public/common/notification_resources.h (right):

https://codereview.chromium.org/1634933006/diff/1/content/public/common/notif...
content/public/common/notification_resources.h:13: // Structure to hold the
resources associated with a Web Notification.
Perhaps add a TODO to clarify that more resources are intended to be added here?

https://codereview.chromium.org/1634933006/diff/1/content/public/common/notif...
content/public/common/notification_resources.h:16: // has failed.
"The bitmap may be empty if the developer did not provide an icon, or fetching
of the icon failed."

[Michael van Ouwerkerk, 2016-01-26 16:24:21]

Thanks Peter.

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:469: //
TODO(peter): Icons for Web Notifications are currently always requested for
On 2016/01/26 15:43:23, Peter Beverloo wrote:
> This is going to be an interesting TODO for us to think about as well.

Acknowledged. Didn't we talk about this earlier today as well?

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/push_messagi...
File chrome/browser/push_messaging/push_messaging_notification_manager.cc
(right):

https://codereview.chromium.org/1634933006/diff/1/chrome/browser/push_messagi...
chrome/browser/push_messaging/push_messaging_notification_manager.cc:340:
content::NotificationResources());
On 2016/01/26 15:43:23, Peter Beverloo wrote:
> s/content::// (you have a using declaration above)

Done.

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
File content/child/notifications/pending_notifications_tracker.cc (right):

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
content/child/notifications/pending_notifications_tracker.cc:11: #include
"content/public/common/notification_resources.h"
On 2016/01/26 15:43:23, Peter Beverloo wrote:
> Something to think about: so far we've named _most_ things in the //content
API
> paltform_notification_* to distinguish between the signalling notification
> system and this.
> 
> Going forth, with the former becoming a more prominent feature, and the latter
> largely being deprecated, this isn't such a large problem anymore. I'm OK
naming
> it NotificationResources - it's much more concise.

Right, it's true that "notification" is very overloaded throughout our codebase.
To be honest, I never found the "platform" prefix very clear and a more specific
prefix would have worked better for me, but not sure what. I'll take your cue on
consistency and readability on this one.

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
File content/child/notifications/pending_notifications_tracker.h (right):

https://codereview.chromium.org/1634933006/diff/1/content/child/notifications...
content/child/notifications/pending_notifications_tracker.h:72: const SkBitmap&
icon);
On 2016/01/26 15:43:23, Peter Beverloo wrote:
> I imagine that you'll update these as a second step?

These methods will probably be nuked from orbit [1]. A new PendingNotification
class will encapsulate fetching multiple resources for a single notification,
and the tracker will become a simpler holder for instances of
PendingNotification.

[1]
https://codereview.chromium.org/1620203004/diff/20001/content/child/notificat...

https://codereview.chromium.org/1634933006/diff/1/content/public/common/notif...
File content/public/common/notification_resources.h (right):

https://codereview.chromium.org/1634933006/diff/1/content/public/common/notif...
content/public/common/notification_resources.h:13: // Structure to hold the
resources associated with a Web Notification.
On 2016/01/26 15:43:23, Peter Beverloo wrote:
> Perhaps add a TODO to clarify that more resources are intended to be added
here?

Done.

https://codereview.chromium.org/1634933006/diff/1/content/public/common/notif...
content/public/common/notification_resources.h:16: // has failed.
On 2016/01/26 15:43:23, Peter Beverloo wrote:
> "The bitmap may be empty if the developer did not provide an icon, or fetching
> of the icon failed."

Done.

[Michael van Ouwerkerk, 2016-01-26 16:37:21]

Mike, could you take a look please for platform_notification_messages.h?

Avi, could you take a look please for content/public/.*notification?

[Michael van Ouwerkerk, 2016-01-26 16:37:42]

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

[Michael van Ouwerkerk, 2016-01-26 16:37:42]

Mike, could you take a look please for platform_notification_messages.h?

Avi, could you take a look please for content/public/.*notification?

[Avi (use Gerrit), 2016-01-27 02:52:44]

lgtm

[Mike West, 2016-01-27 06:25:37]

IPC LGTM.

[Michael van Ouwerkerk, 2016-01-27 10:09:10]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-01-27 10:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1634933006/#ps40001
(title: "Fix unit tests.")

[commit-bot: I haz the power, 2016-01-27 10:09:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634933006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634933006/40001

[commit-bot: I haz the power, 2016-01-27 10:15:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-27 10:16:27]

Description was changed from

==========
Use NotificationResources instead of a bare SkBitmap.

This prepares the notification feature plumbing for using multiple
resources when buttons can have icons. IPC messages cannot take more
than 5 arguments, so the idea is that all icon resources will be wrapped
in this struct.

BUG=581336
==========

to

==========
Use NotificationResources instead of a bare SkBitmap.

This prepares the notification feature plumbing for using multiple
resources when buttons can have icons. IPC messages cannot take more
than 5 arguments, so the idea is that all icon resources will be wrapped
in this struct.

BUG=581336

Committed: https://crrev.com/9a2f11e52fc01897767d686f3829cb4ced01bf47
Cr-Commit-Position: refs/heads/master@{#371765}
==========

[commit-bot: I haz the power, 2016-01-27 10:16:28]

Patchset 3 (id:??) landed as
https://crrev.com/9a2f11e52fc01897767d686f3829cb4ced01bf47
Cr-Commit-Position: refs/heads/master@{#371765}