Move notification resource loading from content/child to blink

Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader, NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification, PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko

Committed: https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f
Cr-Commit-Position: refs/heads/master@{#388174}

[Michael van Ouwerkerk, 2016-03-31 16:17:39]

Description was changed from

==========
Load notification resources from blink.

BUG=592188
==========

to

==========
Load notification resources from blink.

This is a work in progress that will probably need to be split into smaller CLs.

BUG=592188
COMMIT=false
==========

[Michael van Ouwerkerk, 2016-03-31 16:17:50]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 16:18:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/1

[Michael van Ouwerkerk, 2016-03-31 16:18:36]

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

[Michael van Ouwerkerk, 2016-03-31 16:18:37]

Peter: for your entertainment. I'll let you know when it is really for your
review :-)

[commit-bot: I haz the power, 2016-03-31 16:28:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 16:28:10]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Michael van Ouwerkerk, 2016-04-07 17:35:51]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 17:36:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/20001

[commit-bot: I haz the power, 2016-04-07 17:47:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-07 17:47:40]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Michael van Ouwerkerk, 2016-04-12 12:16:18]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 12:16:27]

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

[commit-bot: I haz the power, 2016-04-12 13:08:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 13:08:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael van Ouwerkerk, 2016-04-12 13:27:27]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 13:27:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/60001

[Michael van Ouwerkerk, 2016-04-12 13:58:34]

Description was changed from

==========
Load notification resources from blink.

This is a work in progress that will probably need to be split into smaller CLs.

BUG=592188
COMMIT=false
==========

to

==========
Move notification resource loading from content/child to blink.


BUG=592188
==========

[Michael van Ouwerkerk, 2016-04-12 14:00:23]

Peter, could you take a look please? There seems to be an issue with the
SharedBuffer in NotificationImageLoader, but I don't think that will change much
other stuff. If this CL is too large, let's talk about how to cut it up.

[commit-bot: I haz the power, 2016-04-12 14:26:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 14:26:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael van Ouwerkerk, 2016-04-12 15:39:28]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 15:39:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/80001

[Michael van Ouwerkerk, 2016-04-12 16:15:48]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 16:16:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/100001

[Michael van Ouwerkerk, 2016-04-12 17:02:57]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 17:03:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/120001

[commit-bot: I haz the power, 2016-04-12 18:32:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 18:32:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael van Ouwerkerk, 2016-04-13 13:14:01]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-13 13:14:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/140001

[Michael van Ouwerkerk, 2016-04-13 13:14:59]

Description was changed from

==========
Move notification resource loading from content/child to blink.


BUG=592188
==========

to

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.

BUG=592188
==========

[commit-bot: I haz the power, 2016-04-13 13:31:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 13:31:13]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Michael van Ouwerkerk, 2016-04-13 13:47:10]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-13 13:47:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/160001

[commit-bot: I haz the power, 2016-04-13 15:53:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 15:53:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Beverloo, 2016-04-13 18:32:28]

This looks really good, Michael! This CL will also enable us to move to Mojo
much more easily :-).

Regarding the ASAN failure, do you think we might be leaking the
NotificationImageLoader instance in tests? Would be good to figure out where
that is happening.

We should also carefully consider what we want in regards to indentation. The
intermediate state this CL introduces is not great. Let's check when the Blink
-> Chromium style conversion is planned to occur.

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

https://codereview.chromium.org/1847863002/diff/160001/content/child/notifica...
content/child/notifications/notification_manager.cc:84: DisplayPageNotification(
Can we coalesce that method into this one now? Dito re: showPersistent and
DisplayPersistentNotification?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/DEPS (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/DEPS:8: "+skia/ext",
I would be good to document in the CL description why this added dependency is
necessary.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:152:
bind<NotificationResourcesLoader*>(&Notification::didLoadResources,
Why is it necessary to define the `UnboundParameters` template type? Because of
currying? Other users seem to omit it.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:402:
m_prepareShowMethodRunner->stop();
Maybe add a comment that the |m_loader|, when active, will stop itself?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.h:134: // instance
until the operation completes.
Out of interest, do you think this comment is still relevant?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:33:
if (m_isStopped)
I don't think we'll ever hit this case unless someone calls start() multiple
times?

    DCHECK(!m_isStopped);

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:38:
// TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137.
Note: While I'm sure you're aware, ThreadableLoaderOptions has a `timeout`
member that should make this much easier now.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:38:
// TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137.
Should we have a TODO on adding a FetchInitiatorTypeName for notifications?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:40:
threadableLoaderOptions.preflightPolicy = PreventPreflight;
To confirm: We prevent pre-flight requests since we're allowing cross-origin
requests and thus don't need CORS checks, which is OK because they're images,
right?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:53:
m_threadableLoader = ThreadableLoader::create(*executionContext, this,
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:79:
void NotificationImageLoader::didFinishLoading(unsigned long resourceIdentifier,
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:24:
class NotificationImageLoader final
As a meta comment, it would be nice to document classes with their purpose even
if said documentation is only two or three lines long.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:45:
void didFinishLoading(unsigned long resourceIdentifier,
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:54:
ThreadState::current()->registerPreFinalizer(this);
DCHECK(m_completionCallback)

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:61:
void NotificationResourcesLoader::start(
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:87:
std::unique_ptr<WebNotificationResources>
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:134:
m_badge = scaleDownIfNeeded(image, kMaxBadgeSizePx);
As a.. refactoring opportunity, have you considered doing the resizing as a
separate step when all requests have finished loading? This will make it
significantly easier to use a worker thread to do so, moving that operation of
the script thread as well.

No need to do so in this CL, as this is just moving code.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:138:
void NotificationResourcesLoader::didLoadActionIcon(size_t actionIndex,
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:34:
using CompletionCallback = Function<void(NotificationResourcesLoader*)>;
Docs? Would be good to document that the instance is passed because we store a
set of 'em in ServiceWorkerRegistrationNotifications. (It looks a bit odd when
looking at Notification.cpp, which intuitively is where most people start.)

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:36:
PassOwnPtr<CompletionCallback>);
micro nit: nowrap

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:43:
// more than once.
Is there any way we can DCHECK for this contract? Even if not completely
thorough, something like the following could be a start:

    DCHECK(!m_pendingRequestCount && m_actionIcons.empty());

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:57:
PassOwnPtr<NotificationImageLoader::ImageCallback>);
micro nit: nowrap

(This is invalid per Chromium style too.)

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:66:
// Stops every loader in |m_imageLoaders|.
nit: Also document that this is the pre-finalizer. (The macro on line 32 is easy
to miss.)

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp:259:
} // namespace blink
Did you solve the redirection crash you found?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:141:
ASSERT(notificationManager);
ASSERT or DCHECK?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h:47:
void prepareShow(ExecutionContext*, const WebNotificationData&,
PassOwnPtr<WebNotificationShowCallbacks>);
micro nit: It would be nice to logically separate the supplementable stuff from
the constructors/functionality/member bits with blank lines.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:180:
"`ThreadState::current()->registerPreFinalizer(this);`".
Do you want to separate out this change (and the change to ThreadState.h)?

[haraken, 2016-04-14 02:05:51]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, hiroshige@chromium.org

[haraken, 2016-04-14 02:05:53]

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:152:
bind<NotificationResourcesLoader*>(&Notification::didLoadResources,
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Why is it necessary to define the `UnboundParameters` template type? Because
of
> currying? Other users seem to omit it.

+hiroshige for how to write bind functions.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:130:
WeakPersistentThisPointer<ServiceWorkerRegistrationNotifications>(this),

+hiroshige

[Michael van Ouwerkerk, 2016-04-14 12:01:50]

Description was changed from

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.

BUG=592188
==========

to

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
==========

[Michael van Ouwerkerk, 2016-04-14 13:40:54]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 13:41:14]

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

[Michael van Ouwerkerk, 2016-04-14 13:42:12]

Thanks Peter! Please take another look. I couldn't reproduce the ASAN issue
locally -_-

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

https://codereview.chromium.org/1847863002/diff/160001/content/child/notifica...
content/child/notifications/notification_manager.cc:84: DisplayPageNotification(
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Can we coalesce that method into this one now? Dito re: showPersistent and
> DisplayPersistentNotification?

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/DEPS (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/DEPS:8: "+skia/ext",
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> I would be good to document in the CL description why this added dependency is
> necessary.

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:152:
bind<NotificationResourcesLoader*>(&Notification::didLoadResources,
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Why is it necessary to define the `UnboundParameters` template type? Because
of
> currying? Other users seem to omit it.

You don't need to specify it when there are no unbound params, when you're
creating a closure. If there are then it is required in order to build. I dunno
what tricks base::Bind did to avoid this verbosity.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:152:
bind<NotificationResourcesLoader*>(&Notification::didLoadResources,
On 2016/04/14 02:05:52, haraken wrote:
> On 2016/04/13 18:32:27, Peter Beverloo wrote:
> > Why is it necessary to define the `UnboundParameters` template type? Because
> of
> > currying? Other users seem to omit it.
> 
> +hiroshige for how to write bind functions.

I'd be happy to see a working example of how to bind std::unique_ptr! I tried
adding it in FunctionalTest but no permutation of syntaxes would build.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:402:
m_prepareShowMethodRunner->stop();
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Maybe add a comment that the |m_loader|, when active, will stop itself?

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.h:134: // instance
until the operation completes.
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Out of interest, do you think this comment is still relevant?

It appears to be an antique. I've moved it to a special place I have for such
things.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:33:
if (m_isStopped)
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> I don't think we'll ever hit this case unless someone calls start() multiple
> times?
> 
>     DCHECK(!m_isStopped);

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:38:
// TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137.
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Note: While I'm sure you're aware, ThreadableLoaderOptions has a `timeout`
> member that should make this much easier now.

Yeah that's why it's here. I'm not sure we have enough data yet to pick a useful
timeout.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:38:
// TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137.
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> Should we have a TODO on adding a FetchInitiatorTypeName for notifications?

Sure... what is it used for?

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:40:
threadableLoaderOptions.preflightPolicy = PreventPreflight;
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> To confirm: We prevent pre-flight requests since we're allowing cross-origin
> requests and thus don't need CORS checks, which is OK because they're images,
> right?

Acknowledged.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:53:
m_threadableLoader = ThreadableLoader::create(*executionContext, this,
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:79:
void NotificationImageLoader::didFinishLoading(unsigned long resourceIdentifier,
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:24:
class NotificationImageLoader final
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> As a meta comment, it would be nice to document classes with their purpose
even
> if said documentation is only two or three lines long.

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:45:
void didFinishLoading(unsigned long resourceIdentifier,
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:54:
ThreadState::current()->registerPreFinalizer(this);
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> DCHECK(m_completionCallback)

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:61:
void NotificationResourcesLoader::start(
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:87:
std::unique_ptr<WebNotificationResources>
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:134:
m_badge = scaleDownIfNeeded(image, kMaxBadgeSizePx);
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> As a.. refactoring opportunity, have you considered doing the resizing as a
> separate step when all requests have finished loading? This will make it
> significantly easier to use a worker thread to do so, moving that operation of
> the script thread as well.
> 
> No need to do so in this CL, as this is just moving code.

I've added a TODO at scaleDownIfNeeded. In the current implementation, at least
the scaling is done as the resources finish loading, potentially distributing
the tasks over some time.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:138:
void NotificationResourcesLoader::didLoadActionIcon(size_t actionIndex,
On 2016/04/13 18:32:27, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:34:
using CompletionCallback = Function<void(NotificationResourcesLoader*)>;
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> Docs? Would be good to document that the instance is passed because we store a
> set of 'em in ServiceWorkerRegistrationNotifications. (It looks a bit odd when
> looking at Notification.cpp, which intuitively is where most people start.)

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:36:
PassOwnPtr<CompletionCallback>);
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> micro nit: nowrap

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:43:
// more than once.
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> Is there any way we can DCHECK for this contract? Even if not completely
> thorough, something like the following could be a start:
> 
>     DCHECK(!m_pendingRequestCount && m_actionIcons.empty());

I've added m_started for this.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:57:
PassOwnPtr<NotificationImageLoader::ImageCallback>);
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> micro nit: nowrap
> 
> (This is invalid per Chromium style too.)

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:66:
// Stops every loader in |m_imageLoaders|.
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> nit: Also document that this is the pre-finalizer. (The macro on line 32 is
easy
> to miss.)

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp:259:
} // namespace blink
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> Did you solve the redirection crash you found?

Yes. It wasn't actually about the redirect, but there is a static_cast to
|DocumentThreadableLoaderClient*| in DocumentThreadableLoader::redirectReceived
that I triggered due to a misunderstanding of the meaning and purpose of
isDocumentThreadableLoaderClient.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:130:
WeakPersistentThisPointer<ServiceWorkerRegistrationNotifications>(this),
On 2016/04/14 02:05:52, haraken wrote:
> 
> +hiroshige

So I think now that this should be ok. The problem I had was rooted in a cycle
of GC and non-GC types, that caused persistents to not be cleared in
TreadState::cleanup.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:141:
ASSERT(notificationManager);
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> ASSERT or DCHECK?

DCHECK then. I think the codebase-wide refactor is in progress, but no need to
use ASSERT in this new code.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h:47:
void prepareShow(ExecutionContext*, const WebNotificationData&,
PassOwnPtr<WebNotificationShowCallbacks>);
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> micro nit: It would be nice to logically separate the supplementable stuff
from
> the constructors/functionality/member bits with blank lines.

Done.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md:180:
"`ThreadState::current()->registerPreFinalizer(this);`".
On 2016/04/13 18:32:28, Peter Beverloo wrote:
> Do you want to separate out this change (and the change to ThreadState.h)?

Done.

[hiroshige, 2016-04-14 14:49:22]

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:152:
bind<NotificationResourcesLoader*>(&Notification::didLoadResources,
On 2016/04/14 13:42:11, Michael van Ouwerkerk wrote:
> On 2016/04/14 02:05:52, haraken wrote:
> > On 2016/04/13 18:32:27, Peter Beverloo wrote:
> > > Why is it necessary to define the `UnboundParameters` template type?
Because
> > of
> > > currying? Other users seem to omit it.
> > 
> > +hiroshige for how to write bind functions.
> 
> I'd be happy to see a working example of how to bind std::unique_ptr! I tried
> adding it in FunctionalTest but no permutation of syntaxes would build.

Do you want to call WTF::Function binding unique_ptr multiple times? If so,
probably we need something like base::Owned(). If not (i.e. we only call such
Function once), then more support for move-only types will resolve it.
+tzik@

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:130:
WeakPersistentThisPointer<ServiceWorkerRegistrationNotifications>(this),
On 2016/04/14 02:05:52, haraken wrote:
> 
> +hiroshige
FYI for haraken: WeakPersistentThisPointer<> is just like binding WeakPersistent
as |this| pointer, with null check before invocation.

[hiroshige, 2016-04-14 14:49:36]

hiroshige@chromium.org changed reviewers:
+ tzik@chromium.org

[Michael van Ouwerkerk, 2016-04-14 14:54:41]

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.cpp:152:
bind<NotificationResourcesLoader*>(&Notification::didLoadResources,
On 2016/04/14 14:49:22, hiroshige wrote:
> On 2016/04/14 13:42:11, Michael van Ouwerkerk wrote:
> > On 2016/04/14 02:05:52, haraken wrote:
> > > On 2016/04/13 18:32:27, Peter Beverloo wrote:
> > > > Why is it necessary to define the `UnboundParameters` template type?
> Because
> > > of
> > > > currying? Other users seem to omit it.
> > > 
> > > +hiroshige for how to write bind functions.
> > 
> > I'd be happy to see a working example of how to bind std::unique_ptr! I
tried
> > adding it in FunctionalTest but no permutation of syntaxes would build.
> 
> Do you want to call WTF::Function binding unique_ptr multiple times? If so,
> probably we need something like base::Owned(). If not (i.e. we only call such
> Function once), then more support for move-only types will resolve it.
> +tzik@

I wanted to curry a param that was to be moved. It's a regular class, wrapped in
OwnPtr, but std::unique_ptr is nicer. The WTF::Function is to be called only
once.

[commit-bot: I haz the power, 2016-04-14 15:42:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 15:42:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael van Ouwerkerk, 2016-04-14 15:54:49]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 15:54:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/200001

[commit-bot: I haz the power, 2016-04-14 17:45:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 17:45:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael van Ouwerkerk, 2016-04-15 11:05:15]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 11:05:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/220001

[Peter Beverloo, 2016-04-15 11:07:29]

A few more thoughts, I'll debug the ASAN issue today as well.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/Notification.h:134: // instance
until the operation completes.
On 2016/04/14 13:42:11, Michael van Ouwerkerk wrote:
> On 2016/04/13 18:32:27, Peter Beverloo wrote:
> > Out of interest, do you think this comment is still relevant?
> 
> It appears to be an antique. I've moved it to a special place I have for such
> things.

I am fond of left-hand-side redness as well.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp:38:
// TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137.
On 2016/04/14 13:42:11, Michael van Ouwerkerk wrote:
> On 2016/04/13 18:32:27, Peter Beverloo wrote:
> > Should we have a TODO on adding a FetchInitiatorTypeName for notifications?
> 
> Sure... what is it used for?

I assume for DevTools' Initiator column in the network tab when no source
file/line are available.

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:134:
m_badge = scaleDownIfNeeded(image, kMaxBadgeSizePx);
On 2016/04/14 13:42:11, Michael van Ouwerkerk wrote:
> On 2016/04/13 18:32:28, Peter Beverloo wrote:
> > As a.. refactoring opportunity, have you considered doing the resizing as a
> > separate step when all requests have finished loading? This will make it
> > significantly easier to use a worker thread to do so, moving that operation
of
> > the script thread as well.
> > 
> > No need to do so in this CL, as this is just moving code.
> 
> I've added a TODO at scaleDownIfNeeded. In the current implementation, at
least
> the scaling is done as the resources finish loading, potentially distributing
> the tasks over some time.

I.e. unpredictable performance from the developer's point of view, but since
that's no worse than it is today being aware of the TODO'll do.

https://codereview.chromium.org/1847863002/diff/200001/content/child/notifica...
File content/child/notifications/notification_manager.h (right):

https://codereview.chromium.org/1847863002/diff/200001/content/child/notifica...
content/child/notifications/notification_manager.h:56: // displayed.
This seems like an odd place to document behaviour of overridden methods?

Perhaps we should not bother since this code will move down to Blink before too
long anyhow?

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:135:
LOG(WARNING) << __FUNCTION__;
I presume this is for figuring out the ASAN problem?

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:32:
, public ContextLifecycleObserver {
We can remove some magic here by making ServiceWorkerRegistrationNotifications
the ContextLifecycleObserver instead, and calling through to stop() on each
loader in the set.

[commit-bot: I haz the power, 2016-04-15 12:15:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 12:15:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael van Ouwerkerk, 2016-04-18 11:24:49]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 11:24:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847863002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847863002/240001

[Michael van Ouwerkerk, 2016-04-18 11:26:53]

Thanks Peter. Please take another look.

https://codereview.chromium.org/1847863002/diff/200001/content/child/notifica...
File content/child/notifications/notification_manager.h (right):

https://codereview.chromium.org/1847863002/diff/200001/content/child/notifica...
content/child/notifications/notification_manager.h:56: // displayed.
On 2016/04/15 11:07:29, Peter Beverloo wrote:
> This seems like an odd place to document behaviour of overridden methods?
> 
> Perhaps we should not bother since this code will move down to Blink before
too
> long anyhow?

Let's not then. Done.

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:135:
LOG(WARNING) << __FUNCTION__;
On 2016/04/15 11:07:29, Peter Beverloo wrote:
> I presume this is for figuring out the ASAN problem?

Acknowledged.

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h:32:
, public ContextLifecycleObserver {
On 2016/04/15 11:07:29, Peter Beverloo wrote:
> We can remove some magic here by making ServiceWorkerRegistrationNotifications
> the ContextLifecycleObserver instead, and calling through to stop() on each
> loader in the set.

Done.

[commit-bot: I haz the power, 2016-04-18 12:31:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 12:31:45]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-04-18 15:22:37]

lgtm

May want to add a line to the CL description about some of the tests we can now
write (i.e. resource loading through Service Workers) :-)

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:12:
#include "third_party/skia/include/core/SkBitmap.h"
nit: could forward declare SkBitmap

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:44:
NotificationResourcesLoader::NotificationResourcesLoader(PassOwnPtr<CompletionCallback>
completionCallback) : m_started(false),
m_completionCallback(completionCallback), m_pendingRequestCount(0)
nit: initialization list on its own lines, elsewhere too

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp:33:
static const int kMaxIconSizePx = 320;
nit: these are constants in an anonymous namespace, that's two reasons to not
need "static" :)

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp:48:
// Clean up (and silence the asan bot).
nit: the asan bot remark is not relevant

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h:37:
virtual ~ServiceWorkerRegistrationNotifications();
nit: This doesn't have to be virtual given that this is a final class (unless
you can use `override`). GarbageCollectedFinalized will cast to T prior to
calling the destructor.

[Michael van Ouwerkerk, 2016-04-18 17:03:15]

Thanks Peter.

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h:12:
#include "third_party/skia/include/core/SkBitmap.h"
On 2016/04/18 15:22:37, Peter Beverloo wrote:
> nit: could forward declare SkBitmap

Done.

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:44:
NotificationResourcesLoader::NotificationResourcesLoader(PassOwnPtr<CompletionCallback>
completionCallback) : m_started(false),
m_completionCallback(completionCallback), m_pendingRequestCount(0)
On 2016/04/18 15:22:37, Peter Beverloo wrote:
> nit: initialization list on its own lines, elsewhere too

Done.

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp:33:
static const int kMaxIconSizePx = 320;
On 2016/04/18 15:22:37, Peter Beverloo wrote:
> nit: these are constants in an anonymous namespace, that's two reasons to not
> need "static" :)

Done.

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp:48:
// Clean up (and silence the asan bot).
On 2016/04/18 15:22:37, Peter Beverloo wrote:
> nit: the asan bot remark is not relevant

Done.

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
(right):

https://codereview.chromium.org/1847863002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h:37:
virtual ~ServiceWorkerRegistrationNotifications();
On 2016/04/18 15:22:37, Peter Beverloo wrote:
> nit: This doesn't have to be virtual given that this is a final class (unless
> you can use `override`). GarbageCollectedFinalized will cast to T prior to
> calling the destructor.

Done.

[Michael van Ouwerkerk, 2016-04-18 17:04:58]

Description was changed from

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
==========

to

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=haraken,kinuko
==========

[Michael van Ouwerkerk, 2016-04-18 17:05:15]

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

[Michael van Ouwerkerk, 2016-04-18 17:06:05]

TBR haraken for modules/DEPS and kinuko for blink_platform_impl.cc

Thanks!

[Michael van Ouwerkerk, 2016-04-18 17:06:26]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-04-18 17:06:26]

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/1847863002/#ps260001
(title: "Address peter's comments.")

[commit-bot: I haz the power, 2016-04-18 17:06:55]

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

[Michael van Ouwerkerk, 2016-04-18 17:10:25]

The CQ bit was unchecked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-04-18 17:10:36]

Description was changed from

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=haraken,kinuko
==========

to

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko
==========

[Michael van Ouwerkerk, 2016-04-18 17:11:24]

Kentaro: I have it on good advice that maybe TBR'ing a DEPS change is not
recommended :-) Could you take a look please?

[haraken, 2016-04-19 01:12:52]

In terms of implementation, WebKit changes LGTM

https://codereview.chromium.org/1847863002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:83:
imageLoader->stop();

Would it be better to clear m_imageLoaders to avoid stop() from getting called
twice?

[kinuko, 2016-04-19 02:31:41]

Just noticed this, \o/ no more loader code in content!

(lgtm for content/child changes)

[Michael van Ouwerkerk, 2016-04-19 09:47:31]

Thanks for the reviews!

https://codereview.chromium.org/1847863002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/1847863002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:83:
imageLoader->stop();
On 2016/04/19 01:12:52, haraken wrote:
> 
> Would it be better to clear m_imageLoaders to avoid stop() from getting called
> twice?

Not sure, as there is already a guard in that stop method, it is safe. Both
NotificationResourcesLoader and NotificationImageLoader are on-heap, so I'd like
to avoid manually clearing pointers, it seems confusing.

[Michael van Ouwerkerk, 2016-04-19 09:47:41]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2016-04-19 09:47:49]

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

[commit-bot: I haz the power, 2016-04-19 09:52:16]

Description was changed from

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko
==========

to

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko
==========

[commit-bot: I haz the power, 2016-04-19 09:52:19]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-04-19 09:53:17]

Description was changed from

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko
==========

to

==========
Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick
off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader,
NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification,
PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko

Committed: https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f
Cr-Commit-Position: refs/heads/master@{#388174}
==========

[commit-bot: I haz the power, 2016-04-19 09:53:19]

Patchset 14 (id:??) landed as
https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f
Cr-Commit-Position: refs/heads/master@{#388174}