Add Notification images (Blink + desktop implementation)

Add Notification images (Blink + desktop implementation)

Behind --enable-experimental-web-platform-features flag.
Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discussion

Screenshot: https://imgur.com/a/mUdgu

BUG=614456
Committed: https://crrev.com/578c5cc816365993c926a93514c9ae4ff6baa1af
Cr-Commit-Position: refs/heads/master@{#414177}

[johnme, 2016-08-22 18:32:34]

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

[Peter Beverloo, 2016-08-22 20:11:07]

Thanks!!

Please add a test to platform_notification_service_interactive_uitest.cc and
include a screenshot in the CLs description displaying what it looks like.

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

https://codereview.chromium.org/2261283002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:462: //
TODO(johnme): ensure image is not loaded when it will not be used.
I get why you have this TODO here, but this really isn't the best place for it—
it's effectively invisible. Let's not have it here? Consider adding it to
NotificationResourcesLoader.cpp instead.

https://codereview.chromium.org/2261283002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2261283002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_message_filter.cc:50: bool
ValidateNotificationResources(const NotificationResources& resources) {
We should probably check that the command line flags include
kEnableExperimentalWebPlatformFeatures when either width/height is non-zero.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp:116:
EXPECT_EQ(kNotificationSilent, notificationData.silent);
+EXPECT_EQ for the image

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:30:
maxHeightPx / (float) image.height());
nit: we don't do C-style casts in Chromium

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:33:
skia::ImageOperations::ResizeMethod quality = std::max(image.width(),
maxWidthPx) > 640 ? skia::ImageOperations::RESIZE_GOOD :
skia::ImageOperations::RESIZE_BEST;
I have two concerns about this:

  (1) It's unclear why you are doing this. Did you do performance measurements
that indicate that we should do GOOD resizing rather than BEST for larger
images? What are the results?
  (2) The `Notifications.Icon.ScaleDownTime` histogram will now measure
inconsistent things— it's impossible to know what kind of resizing was used.
Instead, these should be two histograms if we need different qualities.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h:20:
// largest supported scaling factor. TODO(johnme): Check sizes are correct.
Please do this prior to landing the CL.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h:22:
static const int kWebNotificationMaxImageHeightPx = 900; // 225 dip * 4
nit: consider s/const/constexpr/ in this file.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/notifications/notification.mojom
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/notification.mojom:62:
string image;
nit: we need to remove this file, no need to add this.

[Peter Beverloo, 2016-08-22 20:12:01]

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

[commit-bot: I haz the power, 2016-08-22 20:12:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 22:13:00]

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

[commit-bot: I haz the power, 2016-08-22 22:13:01]

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

[johnme, 2016-08-23 14:59:50]

Addressed review comments - thanks :)

> Please add a test to platform_notification_service_interactive_uitest.cc and
> include a screenshot in the CLs description displaying what it looks like.

Ack, I'll do that in a moment.

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

https://codereview.chromium.org/2261283002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:462: //
TODO(johnme): ensure image is not loaded when it will not be used.
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> I get why you have this TODO here, but this really isn't the best place for
it—
> it's effectively invisible. Let's not have it here? Consider adding it to
> NotificationResourcesLoader.cpp instead.

Done. Moved the badge one too.

https://codereview.chromium.org/2261283002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2261283002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_message_filter.cc:50: bool
ValidateNotificationResources(const NotificationResources& resources) {
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> We should probably check that the command line flags include
> kEnableExperimentalWebPlatformFeatures when either width/height is non-zero.

Done.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationDataTest.cpp:116:
EXPECT_EQ(kNotificationSilent, notificationData.silent);
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> +EXPECT_EQ for the image

I matched the TODO above: "// TODO(peter): Test the various icon properties when
ExecutionContext::completeURL() works in this test." :)

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:30:
maxHeightPx / (float) image.height());
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> nit: we don't do C-style casts in Chromium

Done.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp:33:
skia::ImageOperations::ResizeMethod quality = std::max(image.width(),
maxWidthPx) > 640 ? skia::ImageOperations::RESIZE_GOOD :
skia::ImageOperations::RESIZE_BEST;
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> I have two concerns about this:
> 
>   (1) It's unclear why you are doing this. Did you do performance measurements
> that indicate that we should do GOOD resizing rather than BEST for larger
> images? What are the results?
>   (2) The `Notifications.Icon.ScaleDownTime` histogram will now measure
> inconsistent things— it's impossible to know what kind of resizing was used.
> Instead, these should be two histograms if we need different qualities.

It seems we should use RESIZE_BETTER in this case based on the comment:
  // This is intended, for example, for generation of large thumbnails
  // (hundreds of pixels in each dimension) from large sources, where
  // a linear filter would produce too many artifacts but where
  // a RESIZE_HIGH might be too costly time-wise.

Indeed https://goto.google.com/wegnw shows that each resize already takes 750ms+
at the 99th percentile for Svelte users, which seems pretty damning. Based on
that and b/5292691 I'd support using RESIZE_BETTER for icons as well.

But based on our offline discussion, I've punted this change and left you a
TODO, as you seemed to be aware of easy ways of benchmarking different scaling
algorithms.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h:20:
// largest supported scaling factor. TODO(johnme): Check sizes are correct.
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> Please do this prior to landing the CL.

Ack, though gonna be tricky since it's platform-dependent.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h:22:
static const int kWebNotificationMaxImageHeightPx = 900; // 225 dip * 4
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> nit: consider s/const/constexpr/ in this file.

Done.

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/notifications/notification.mojom
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/notification.mojom:62:
string image;
On 2016/08/22 20:11:07, Peter Beverloo wrote:
> nit: we need to remove this file, no need to add this.

Done, ditto for content/browser/notifications/type_converters* as discussed.

[johnme, 2016-08-23 15:00:13]

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

[johnme, 2016-08-23 15:00:14]

mkwst@chromium.org: Please review changes in
content/common/platform_notification_messages.h and
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in - thanks!

[johnme, 2016-08-23 15:19:35]

johnme@chromium.org changed reviewers:
- mkwst@chromium.org

[johnme, 2016-08-23 15:19:35]

-mkwst since he's OOO.

[johnme, 2016-08-23 17:08:26]

johnme@chromium.org changed reviewers:
+ kbr@chromium.org, nasko@chromium.org

[johnme, 2016-08-23 17:08:26]

kbr@chromium.org: Please review changes in
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

nasko@chromium.org: Please review changes in content/common/ and content/public/

Thanks!

[Ken Russell (switch to Gerrit), 2016-08-23 20:35:39]

RuntimeEnabledFeatures.in lgtm

[johnme, 2016-08-24 14:40:08]

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

[commit-bot: I haz the power, 2016-08-24 14:40:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2016-08-24 15:21:11]

lgtm

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h
(right):

https://codereview.chromium.org/2261283002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h:20:
// largest supported scaling factor. TODO(johnme): Check sizes are correct.
On 2016/08/23 14:59:49, johnme wrote:
> On 2016/08/22 20:11:07, Peter Beverloo wrote:
> > Please do this prior to landing the CL.
> 
> Ack, though gonna be tricky since it's platform-dependent.

Right now it'd come down to max([android dips] * 4, [desktop dips] * 2), right?
We have to refine these numbers when new (native) platforms are added, and the
browser process should be authoritative over their intended size.

https://codereview.chromium.org/2261283002/diff/60001/content/browser/notific...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2261283002/diff/60001/content/browser/notific...
content/browser/notifications/notification_message_filter.cc:57: }
We're going to have to remember to remove this one when moving to status=stable.

[commit-bot: I haz the power, 2016-08-24 15:56:25]

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

[commit-bot: I haz the power, 2016-08-24 15:56:26]

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

[johnme, 2016-08-24 17:01:13]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-08-24 17:01:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2261283002/#ps80001
(title: "Check IPC drawsNothing rather than isNull")

[commit-bot: I haz the power, 2016-08-24 17:02:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 17:11:15]

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

[commit-bot: I haz the power, 2016-08-24 17:11:17]

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

[johnme, 2016-08-24 18:07:09]

johnme@chromium.org changed reviewers:
- nasko@chromium.org

[johnme, 2016-08-24 18:07:10]

-nasko, who's OOO

[johnme, 2016-08-24 18:08:33]

johnme@chromium.org changed reviewers:
+ avi@chromium.org, tsepez@chromium.org

[johnme, 2016-08-24 18:08:34]

tsepez: Please review changes in content/common/platform_notification_messages.h

avi: Please stamp changes in content/public/common/

Thanks!

[Tom Sepez, 2016-08-24 18:10:55]

Messages LGTM.

[johnme, 2016-08-24 20:00:52]

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

[commit-bot: I haz the power, 2016-08-24 20:02:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-08-24 20:43:23]

Description was changed from

==========
Add Notification images (Blink + desktop implementation)

Not yet hooked up on Android.

BUG=614456
==========

to

==========
Add Notification images (Blink + desktop implementation)

Behind a flag. Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://i.imgur.com/lnGOY2d.png

BUG=614456
==========

[johnme, 2016-08-24 21:02:48]

Description was changed from

==========
Add Notification images (Blink + desktop implementation)

Behind a flag. Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://i.imgur.com/lnGOY2d.png

BUG=614456
==========

to

==========
Add Notification images (Blink + desktop implementation)

Behind --enable-experimental-web-platform-features flag.
Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://imgur.com/a/mUdgu

BUG=614456
==========

[commit-bot: I haz the power, 2016-08-24 21:56:22]

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

[commit-bot: I haz the power, 2016-08-24 21:56:23]

Dry run: This issue passed the CQ dry run.

[johnme, 2016-08-24 22:09:06]

johnme@chromium.org changed reviewers:
+ piman@chromium.org
- avi@chromium.org

[johnme, 2016-08-24 22:09:07]

-avi (seems to be out) +piman

[piman, 2016-08-24 22:12:20]

content/ LGTM

[johnme, 2016-08-24 22:14:11]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-08-24 22:15:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 22:39:08]

Description was changed from

==========
Add Notification images (Blink + desktop implementation)

Behind --enable-experimental-web-platform-features flag.
Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://imgur.com/a/mUdgu

BUG=614456
==========

to

==========
Add Notification images (Blink + desktop implementation)

Behind --enable-experimental-web-platform-features flag.
Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://imgur.com/a/mUdgu

BUG=614456
==========

[commit-bot: I haz the power, 2016-08-24 22:39:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-24 22:42:10]

Description was changed from

==========
Add Notification images (Blink + desktop implementation)

Behind --enable-experimental-web-platform-features flag.
Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://imgur.com/a/mUdgu

BUG=614456
==========

to

==========
Add Notification images (Blink + desktop implementation)

Behind --enable-experimental-web-platform-features flag.
Not yet hooked up on Android.

Intent to implement:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FQxPB5GEQjo/discus...

Screenshot: https://imgur.com/a/mUdgu

BUG=614456

Committed: https://crrev.com/578c5cc816365993c926a93514c9ae4ff6baa1af
Cr-Commit-Position: refs/heads/master@{#414177}
==========

[commit-bot: I haz the power, 2016-08-24 22:42:11]

Patchset 5 (id:??) landed as
https://crrev.com/578c5cc816365993c926a93514c9ae4ff6baa1af
Cr-Commit-Position: refs/heads/master@{#414177}