Factor out a PendingNotificationTracker from the NotificationManager.

Factor out a PendingNotificationTracker from the NotificationManager.

The existing code ends up moving values between threads, whether by
value or hidden in a callback, causing instability and races in edge-
cases, for example when abruptly cancelling the load.

This patch introduces a PendingNotificationTracker, which completely
lives on the same thread as NotificationManager and will store all
information associated with the notification, ensuring that it never
inadvertently leaves the thread.

The NotificationImageLoader lives on the main thread and is in charge
of doing the actual fetch. Rather than storing either a callback with
the Notification's data, it knows a pending notification id given to
it by the PendingNotificationTracker. This allows us to re-associate
the data when the fetched resource(s) are available.

I'm adding some layout tests in the following patch to exercise the
image loader in a number of additional cases. Unit tests will be added
in a follow-up patch.

https://codereview.chromium.org/933153003/

BUG=458640
Committed: https://crrev.com/11d160807115e28b7b870a40a2ab14825f7cd365
Cr-Commit-Position: refs/heads/master@{#316921}

[Peter Beverloo, 2015-02-18 00:07:55]

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

[Peter Beverloo, 2015-02-18 00:07:55]

+mvanouwerkerk, mlamouri

I would appreciate an early review. This fixes 2 crashes I still see
occasionally happening, which'd be nice to squash out.

Basically, it makes sure that no data ever leaves the thread it's meant to be
on, whereas the current structure mixes that up a bit.

[mlamouri (slow - plz ping), 2015-02-18 14:53:59]

Few comments below but this is a big patch doing subtle changes on code I don't
know much so I might need more than one pass.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
File content/child/notifications/notification_image_loader.cc (left):

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:40:
DCHECK(worker_task_runner);
Would it make sense to have:
DCHECK(!worker_task_runner_->BelongsToCurrentThread()); ?

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:47: WebURL
imageUrl(image_url);
nit: coding style.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:88: }
What was wrong with GetDecodedImage? Maybe you could do something like:
  completede_ = true;
  SkBitmap icon = GetDecodedImage();

and add a:
  DCHECK(completed_);

in ::GetDecodedImage().

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:88: }
What was wrong with GetDecodedImage? Maybe you could do something like:
  completede_ = true;
  SkBitmap icon = GetDecodedImage();

and add a:
  DCHECK(completed_);

in ::GetDecodedImage().

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.cc:109: SkBitmap() /* icon */);
nit: you probably don't need that /* icon */ comment.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.cc:133: SkBitmap() /* icon */);
nit: /* icon */ is probably not needed.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.cc:147: for (; iter !=
active_page_notifications_.end(); ++iter) {
Here and below, could that be:
  for (auto iter : active_page_notifications)

?

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:41:
blink::WebNotificationDelegate* delegate,
Could you explain in the comment what |delegate| is for?

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:42: const SkBitmap& icon);
Could you move the |icon| above |delegate|?

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:50:
scoped_ptr<blink::WebNotificationShowCallbacks> callbacks,
Could you explain in the comment what |callbacks| and
|service_worker_registration_id| are for.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:51: const SkBitmap& icon);
Could you move the |icon| below |notification_data| so the first three arguments
are the same for both methods?

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/pending_notification_tracker.cc:130:
DCHECK(notification);
Could you add a test for that to try the situation where you:
- start a fetch
- cancel the fetch immediately

We should make sure that ::DidFetchPageNotification isn't called in that case.

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/pending_notification_tracker.h:49: void
FetchPageNotification(
nit: I find the name confusing, what about FetchPageNotificationResources()?

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/pending_notification_tracker.h:56: void
FetchPersistentNotification(
ditto

[Peter Beverloo, 2015-02-18 15:11:50]

Thanks for the review, Mounir!! PTAL.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
File content/child/notifications/notification_image_loader.cc (left):

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:40:
DCHECK(worker_task_runner);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Would it make sense to have:
> DCHECK(!worker_task_runner_->BelongsToCurrentThread()); ?

No, because it's possible for the worker_task_runner to be the main thread (in
case the document is being shown from a Document).

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:47: WebURL
imageUrl(image_url);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> nit: coding style.

Done.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_image_loader.cc:88: }
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> What was wrong with GetDecodedImage? Maybe you could do something like:
>   completede_ = true;
>   SkBitmap icon = GetDecodedImage();
> 
> and add a:
>   DCHECK(completed_);
> 
> in ::GetDecodedImage().

Nothing. Restored.

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.cc:109: SkBitmap() /* icon */);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> nit: you probably don't need that /* icon */ comment.

Done.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.cc:133: SkBitmap() /* icon */);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> nit: /* icon */ is probably not needed.

Done.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.cc:147: for (; iter !=
active_page_notifications_.end(); ++iter) {
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Here and below, could that be:
>   for (auto iter : active_page_notifications)
> 
> ?

Done.

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:41:
blink::WebNotificationDelegate* delegate,
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Could you explain in the comment what |delegate| is for?

Done.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:42: const SkBitmap& icon);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Could you move the |icon| above |delegate|?

Why? Everything but |icon| is present in show() and is fed through the
PendingNotificationTracker without being modified. |icon| is the thing that we
add "on top". It makes sense for it to be last.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:50:
scoped_ptr<blink::WebNotificationShowCallbacks> callbacks,
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Could you explain in the comment what |callbacks| and
> |service_worker_registration_id| are for.

Done.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/notification_manager.h:51: const SkBitmap& icon);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Could you move the |icon| below |notification_data| so the first three
arguments
> are the same for both methods?

Dito.

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/pending_notification_tracker.cc:130:
DCHECK(notification);
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> Could you add a test for that to try the situation where you:
> - start a fetch
> - cancel the fetch immediately
> 
> We should make sure that ::DidFetchPageNotification isn't called in that case.

I've included serviceworkerregistration-service-worker-image-abort.html as a
layout test in the following case that exercises this patch:

https://codereview.chromium.org/933153003/

I do want to include unit tests for this code, but those will be a bit tricky to
write.

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

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/pending_notification_tracker.h:49: void
FetchPageNotification(
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> nit: I find the name confusing, what about FetchPageNotificationResources()?

Done.

https://codereview.chromium.org/939513002/diff/1/content/child/notifications/...
content/child/notifications/pending_notification_tracker.h:56: void
FetchPersistentNotification(
On 2015/02/18 14:53:58, Mounir Lamouri wrote:
> ditto

Done.

[Peter Beverloo, 2015-02-18 18:28:09]

PTAL. This works awesomely well now.

[mlamouri (slow - plz ping), 2015-02-18 21:04:08]

I think it would be nice if you could pass a callback to the notifications
tracker instead of having it known about the manager. I would be fine with
having the notification tracker pass all the arguments to that callback but it
seems that it doesn't have to know what the manager wants to do nor even known
about the manager.

lgtm with that.

https://codereview.chromium.org/939513002/diff/60001/content/child/notificati...
File content/child/notifications/pending_notification_tracker.h (right):

https://codereview.chromium.org/939513002/diff/60001/content/child/notificati...
content/child/notifications/pending_notification_tracker.h:40: class
PendingNotificationTracker {
Could you make it plural? It would make it easier to understand it's actually
tracking multiple notifications at the same time. FWIW, the NotificationManager
member is even called |pending_notifications_|.

[Peter Beverloo, 2015-02-18 21:35:06]

New patchsets have been uploaded after l-g-t-m from mlamouri@chromium.org

[Peter Beverloo, 2015-02-18 21:44:55]

Done. Thank you for the suggestion, that made it a lot leaner :-).

https://codereview.chromium.org/939513002/diff/60001/content/child/notificati...
File content/child/notifications/pending_notification_tracker.h (right):

https://codereview.chromium.org/939513002/diff/60001/content/child/notificati...
content/child/notifications/pending_notification_tracker.h:40: class
PendingNotificationTracker {
On 2015/02/18 21:04:08, Mounir Lamouri wrote:
> Could you make it plural? It would make it easier to understand it's actually
> tracking multiple notifications at the same time. FWIW, the
NotificationManager
> member is even called |pending_notifications_|.

Done.

[mlamouri (slow - plz ping), 2015-02-18 21:51:58]

looks great, thanks!

Fix the nits, land that and go home! :)

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

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/notification_manager.cc:81: base::Unretained(this),
nit: could you point that you are using base::Unretained(this) because |this|
owns |pending_notificationts_|.

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/notification_manager.cc:110: base::Unretained(this),
ditto.

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/notification_manager.cc:110: base::Unretained(this),
ditto.

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

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:27:
NotificationResourcesFetchedCallback callback;
const T& ?

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:80:
PendingNotification* notification =
s/notification/pending_notification/
... and below.

[Peter Beverloo, 2015-02-18 21:57:49]

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

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/notification_manager.cc:81: base::Unretained(this),
On 2015/02/18 21:51:58, Mounir Lamouri wrote:
> nit: could you point that you are using base::Unretained(this) because |this|
> owns |pending_notificationts_|.

Done.

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/notification_manager.cc:110: base::Unretained(this),
On 2015/02/18 21:51:58, Mounir Lamouri wrote:
> ditto. 

Done.

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

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:27:
NotificationResourcesFetchedCallback callback;
On 2015/02/18 21:51:58, Mounir Lamouri wrote:
> const T& ?

We create the callback on the stack, so need to store it by value.

https://codereview.chromium.org/939513002/diff/100001/content/child/notificat...
content/child/notifications/pending_notifications_tracker.cc:80:
PendingNotification* notification =
On 2015/02/18 21:51:58, Mounir Lamouri wrote:
> s/notification/pending_notification/
> ... and below.

Done.

[Peter Beverloo, 2015-02-18 21:58:12]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2015-02-18 21:58:33]

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

[commit-bot: I haz the power, 2015-02-18 23:46:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-18 23:47:13]

Patchset 7 (id:??) landed as
https://crrev.com/11d160807115e28b7b870a40a2ab14825f7cd365
Cr-Commit-Position: refs/heads/master@{#316921}