Fetch notification action icons and pass them through in resources.

Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its callback.
* Adds a unit test for this area of code.

BUG=581336, 423039
Committed: https://crrev.com/897451590bd2134f8952c40c5758b217c923ab03
Cr-Commit-Position: refs/heads/master@{#374881}

[Michael van Ouwerkerk, 2016-02-03 18:28:26]

Description was changed from

==========
Fetch notification action icons and pass them through in resources.

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

to

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

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

[Michael van Ouwerkerk, 2016-02-03 18:28:49]

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

[Michael van Ouwerkerk, 2016-02-03 18:28:49]

Peter, could you take a look please?

[Peter Beverloo, 2016-02-05 15:43:22]

I really like this, thanks! A bunch of comments, but aside from some excess data
copies nothing major.

You might also want to refer (and own) Issue 423039.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.cc:37: bool
hasResourcesToFetch(const blink::WebNotificationData& notification_data) {
nit: HasResourcesToFetch (Chromium style)

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.cc:94:
notifications_tracker_.FetchNotificationResources(
DCHECK_EQ(0u, notification_data.actions.size());

No harm in keeping the post-fetch DCHECK as well, but I'd rather we check
*before* downloading resources since HasResourcesToFetch() would happily return
true.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.cc:133: if
(!notification_data.actions.isEmpty()) {
It would be great to document why this is significant. What about:

    // Action button indices are expected to have an corresponding icon bitmap,
    // which may be empty when the developer provided no (or an invalid) icon.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notification.cc (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:23:
action_icons_(std::vector<SkBitmap>(notification_data_.actions.size())),
Initialization list declarations will construct the member with the given
arguments. No need for the additional move here, you can just write:

  action_icons(notification_data_.actions.size())

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:24:
fetches_finished_barrier_closure_(BarrierClosure(
Why don't you have to specify base:: here?

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:43: NotificationResources
PendingNotification::GetResources() {
This will make a copy of all the resources. Since this isn't marked as const,
can we perhaps ensure that it'll only be called once and swap memory instead?

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:58: // Convert to GURL
before sending to the main thread (crbug.com/458640).
Per your request, a suggestion for an updated comment with more details:

  // Explicitly convert the WebURL to a GURL before passing it to a different
  // thread. This is important because WebURLs must not be passed between
  // threads, and per base::Bind() semantics conversion would otherwise be done
  // on the receiving thread.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:63:
base::ThreadTaskRunnerHandle::Get()));
Something to consider: since we know both the target thread and the main thread
here, rather than using ThreadTaskRunnerHandle::Get() in the
NotificationImageLoader we could simplify that code a bit by passing it in the
constructor, re-adding the main thread DCHECK in StartOnMainThread() and remove
the if(has_main_thread) statements in the destructors, since we know we'll have
it.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:78:
action_icons_[action_index] = action_icon;
DCHECK_LT(action_index, action_icons_.size()); ?

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notification.h (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.h:34: const
blink::WebNotificationData& notification_data,
The only reason that PendingNotification stores the |notification_data| is to
access it in FetchResources(), which is called on the same thread.

The WebNotificationData object can be arbitrarily large (developer-provided
data), we can avoid this copy by taking a const& argument in FetchResources()
instead and only using what we need there.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.h:68:
std::set<scoped_refptr<NotificationImageLoader>> image_loaders_;
nit: May as well use a std::vector<> for this, since we don't use set semantics.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notifications_tracker.cc (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:33:
PendingNotification* pending_notification = new PendingNotification(
What about writing this down as follows to make ownership more explicit?

===============
  scoped_ptr<PendingNotification> pending_notification(
      new PendingNotification(main_thread_task_runner_, notification_data,
                              fetches_finished_callback));
  pending_notification->FetchResources();

  pending_notifications_.AddWithID(
      pending_notification.release(), notification_id);
===============

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:59:
resources_callback.Run(pending_notification->GetResources());
For historical context, we once had a crash here (neé
DidFetchPersistentNotification) where the Lookup() failed in a release build,
and we'd crash after accessing memory due to calling a method on a nullptr.

We haven't seen this in a long time, but if you're into defensive programming
the following is a bit more crash-safe (since failing here isn't the end of the
world) -

===============
if (!pending_notification) {
  NOTREACHED();
  return;
}
===============

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notifications_tracker.h (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:33: using
NotificationResourcesCallback =
This can live in the PendingNotificationsTracker class since it's only used
there. (Which'll allow you to drop the Notification prefix, too.)

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:48: // may be null,
but if it is not it can be used to cancel the fetches.
nit: s/but if it is not it/but non-null values/

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:50: void
FetchNotificationResources(
Up to you, but for consistency reasons with FetchesFinished you may want to
consider calling this FetchResources(). The fact that it's for notifications is
implied by the class name.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:57: bool
CancelResourceFetches(blink::WebNotificationDelegate* delegate);
Should this method explicitly disallow non-NULL values (+DCHECK)? It would catch
cases where we for whatever reason end up trying to cancel persistent
notifications.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notifications_tracker_unittest.cc
(right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:37: const
char kTestDataPath[] = "content/test/data/notifications/";
micro nit: blank line above here

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:74: return
&resources_[index];
nit: DCHECK_LT(index, resources_.size())?

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:85: void
runLoop() {
nit: RunLoop

Why not use base::RunLoop().RunUntilIdle()?

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:91:
blink::WebUnitTestSupport* unitTestSupport() {
nit: UnitTestSupport

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:106:
file_path = file_path.Append(FILE_PATH_LITERAL(kTestDataPath + file_name));
I think you can fix the build issues by using the following instead:

file_path =
file_path.Append(FILE_PATH_LITERAL(kTestDataPath)).Append(file_name);

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:150:
base::Unretained(this), 0));
nit: 0 /* index */

dito elsewhere in this file

[Michael van Ouwerkerk, 2016-02-08 14:37:32]

Description was changed from

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

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

to

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

BUG=581336,423039
==========

[Michael van Ouwerkerk, 2016-02-08 14:38:53]

Thanks for the review Peter. Please take another look.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.cc:37: bool
hasResourcesToFetch(const blink::WebNotificationData& notification_data) {
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: HasResourcesToFetch (Chromium style)

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.cc:94:
notifications_tracker_.FetchNotificationResources(
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> DCHECK_EQ(0u, notification_data.actions.size());
> 
> No harm in keeping the post-fetch DCHECK as well, but I'd rather we check
> *before* downloading resources since HasResourcesToFetch() would happily
return
> true.

Sure. Let's put that at the start of the function then.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/notification_manager.cc:133: if
(!notification_data.actions.isEmpty()) {
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> It would be great to document why this is significant. What about:
> 
>     // Action button indices are expected to have an corresponding icon
bitmap,
>     // which may be empty when the developer provided no (or an invalid) icon.

Yes, it's a subtle one.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notification.cc (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:23:
action_icons_(std::vector<SkBitmap>(notification_data_.actions.size())),
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> Initialization list declarations will construct the member with the given
> arguments. No need for the additional move here, you can just write:
> 
>   action_icons(notification_data_.actions.size())

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:24:
fetches_finished_barrier_closure_(BarrierClosure(
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> Why don't you have to specify base:: here?

Dunno, maybe because it has BASE_EXPORT? Certainly it has precedence, but it
seems only right to specify the namespace. Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:43: NotificationResources
PendingNotification::GetResources() {
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> This will make a copy of all the resources. Since this isn't marked as const,
> can we perhaps ensure that it'll only be called once and swap memory instead?

It's not that bad, the bulky pixel data is thread-safe ref counted, and a deep
copy of that data is only made in the various copyTo methods of SkBitmap. It's
easy to add a call to std::move but then the contract of this method changes
significantly, without much gain. Do you think that's worthwhile?

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:58: // Convert to GURL
before sending to the main thread (crbug.com/458640).
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> Per your request, a suggestion for an updated comment with more details:
> 
>   // Explicitly convert the WebURL to a GURL before passing it to a different
>   // thread. This is important because WebURLs must not be passed between
>   // threads, and per base::Bind() semantics conversion would otherwise be
done
>   // on the receiving thread.

Nice. Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:63:
base::ThreadTaskRunnerHandle::Get()));
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> Something to consider: since we know both the target thread and the main
thread
> here, rather than using ThreadTaskRunnerHandle::Get() in the
> NotificationImageLoader we could simplify that code a bit by passing it in the
> constructor, re-adding the main thread DCHECK in StartOnMainThread() and
remove
> the if(has_main_thread) statements in the destructors, since we know we'll
have
> it.

Cool. Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.cc:78:
action_icons_[action_index] = action_icon;
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> DCHECK_LT(action_index, action_icons_.size()); ?

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notification.h (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.h:34: const
blink::WebNotificationData& notification_data,
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> The only reason that PendingNotification stores the |notification_data| is to
> access it in FetchResources(), which is called on the same thread.
> 
> The WebNotificationData object can be arbitrarily large (developer-provided
> data), we can avoid this copy by taking a const& argument in FetchResources()
> instead and only using what we need there.

Ugh. The constructor does a bit more than that, and I like to have a sane
internal state after constructing. But, I would like to avoid this copy, so
Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notification.h:68:
std::set<scoped_refptr<NotificationImageLoader>> image_loaders_;
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: May as well use a std::vector<> for this, since we don't use set
semantics.

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notifications_tracker.cc (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:33:
PendingNotification* pending_notification = new PendingNotification(
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> What about writing this down as follows to make ownership more explicit?
> 
> ===============
>   scoped_ptr<PendingNotification> pending_notification(
>       new PendingNotification(main_thread_task_runner_, notification_data,
>                               fetches_finished_callback));
>   pending_notification->FetchResources();
> 
>   pending_notifications_.AddWithID(
>       pending_notification.release(), notification_id);
> ===============

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:59:
resources_callback.Run(pending_notification->GetResources());
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> For historical context, we once had a crash here (neé
> DidFetchPersistentNotification) where the Lookup() failed in a release build,
> and we'd crash after accessing memory due to calling a method on a nullptr.
> 
> We haven't seen this in a long time, but if you're into defensive programming
> the following is a bit more crash-safe (since failing here isn't the end of
the
> world) -
> 
> ===============
> if (!pending_notification) {
>   NOTREACHED();
>   return;
> }
> ===============

It would be nice to then link to a bug with that crash. I'm not sure how that
would happen though, as this state is pretty much managed by this class itself
on a single thread.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notifications_tracker.h (right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:33: using
NotificationResourcesCallback =
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> This can live in the PendingNotificationsTracker class since it's only used
> there. (Which'll allow you to drop the Notification prefix, too.)

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:48: // may be null,
but if it is not it can be used to cancel the fetches.
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: s/but if it is not it/but non-null values/

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:50: void
FetchNotificationResources(
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> Up to you, but for consistency reasons with FetchesFinished you may want to
> consider calling this FetchResources(). The fact that it's for notifications
is
> implied by the class name.

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.h:57: bool
CancelResourceFetches(blink::WebNotificationDelegate* delegate);
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> Should this method explicitly disallow non-NULL values (+DCHECK)? It would
catch
> cases where we for whatever reason end up trying to cancel persistent
> notifications.

Sure, we can explicitly disallow null values (and document that).

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
File content/child/notifications/pending_notifications_tracker_unittest.cc
(right):

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:37: const
char kTestDataPath[] = "content/test/data/notifications/";
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> micro nit: blank line above here

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:74: return
&resources_[index];
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: DCHECK_LT(index, resources_.size())?

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:85: void
runLoop() {
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: RunLoop
> 
> Why not use base::RunLoop().RunUntilIdle()?

Because of the warning in the documentation for RunUntilIdle. Perhaps a bit on
the paranoid side for a unit test, I can get rid of this helper.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:91:
blink::WebUnitTestSupport* unitTestSupport() {
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: UnitTestSupport

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:106:
file_path = file_path.Append(FILE_PATH_LITERAL(kTestDataPath + file_name));
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> I think you can fix the build issues by using the following instead:
> 
> file_path =
> file_path.Append(FILE_PATH_LITERAL(kTestDataPath)).Append(file_name);

Done.

https://codereview.chromium.org/1644083002/diff/40001/content/child/notificat...
content/child/notifications/pending_notifications_tracker_unittest.cc:150:
base::Unretained(this), 0));
On 2016/02/05 15:43:21, Peter Beverloo wrote:
> nit: 0 /* index */
> 
> dito elsewhere in this file

Done.

[Michael van Ouwerkerk, 2016-02-08 14:50:20]

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

[Michael van Ouwerkerk, 2016-02-08 14:50:20]

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

[Michael van Ouwerkerk, 2016-02-08 14:51:57]

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

[Michael van Ouwerkerk, 2016-02-08 14:51:57]

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

[Michael van Ouwerkerk, 2016-02-10 13:53:49]

Peter, Avi, could you take a look please? Thanks :-)

[Avi (use Gerrit), 2016-02-10 17:45:38]

content public api lgtm

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
content/child/notifications/notification_manager.cc:334:
DCHECK_EQ(notification_resources.action_icons.size(), 0u);
expected values first

[Peter Beverloo, 2016-02-10 18:14:32]

lgtm!

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
File content/child/notifications/pending_notification.h (right):

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
content/child/notifications/pending_notification.h:41: NotificationResources
GetResources();
nit: const qualifier on the method.

[Michael van Ouwerkerk, 2016-02-10 18:23:44]

Thanks!

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
content/child/notifications/notification_manager.cc:334:
DCHECK_EQ(notification_resources.action_icons.size(), 0u);
On 2016/02/10 17:45:37, Avi wrote:
> expected values first

Done.

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
File content/child/notifications/pending_notification.h (right):

https://codereview.chromium.org/1644083002/diff/140001/content/child/notifica...
content/child/notifications/pending_notification.h:41: NotificationResources
GetResources();
On 2016/02/10 18:14:32, Peter Beverloo wrote:
> nit: const qualifier on the method.

Done.

[Michael van Ouwerkerk, 2016-02-10 18:25:28]

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

[Michael van Ouwerkerk, 2016-02-10 18:25:28]

Ken, could you take a look at content/common/platform_notification_messages.h
please as Mike is traveling?

[kenrb, 2016-02-10 20:17:49]

ipc lgtm

[Michael van Ouwerkerk, 2016-02-11 09:52:19]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-02-11 09:52:19]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org, kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/1644083002/#ps180001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-11 09:52:46]

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

[commit-bot: I haz the power, 2016-02-11 09:52:48]

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

[commit-bot: I haz the power, 2016-02-11 10:59:11]

Description was changed from

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

BUG=581336,423039
==========

to

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

BUG=581336,423039
==========

[commit-bot: I haz the power, 2016-02-11 10:59:14]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-02-11 11:00:13]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-02-16 22:35:11]

Description was changed from

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

BUG=581336,423039
==========

to

==========
Fetch notification action icons and pass them through in resources.

* Adds an action_icons field to NotificationResources
* Turns PendingNotification into a real class.
* PendingNotification fetches potentially multiple resources before running its
callback.
* Adds a unit test for this area of code.

BUG=581336,423039

Committed: https://crrev.com/897451590bd2134f8952c40c5758b217c923ab03
Cr-Commit-Position: refs/heads/master@{#374881}
==========

[commit-bot: I haz the power, 2016-02-16 22:35:12]

Patchset 10 (id:??) landed as
https://crrev.com/897451590bd2134f8952c40c5758b217c923ab03
Cr-Commit-Position: refs/heads/master@{#374881}