Implementation of renotify flag for Notifications.

Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound, lights,
and vibration are played again when a notification replaces a previous notification.

Changes for this issue are covered by Intent to Implement and Ship: Notification Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
Committed: https://crrev.com/39c49fe6e8dfe2f2ae3c7db261cb8e845692dfd2
Cr-Commit-Position: refs/heads/master@{#374709}

[harkness, 2016-02-02 10:08:21]

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

[harkness, 2016-02-02 10:08:21]

Please take a look for functionality when you have a chance.

[Michael van Ouwerkerk, 2016-02-02 10:46:01]

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

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

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

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

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

[commit-bot: I haz the power, 2016-02-02 11:56:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Beverloo, 2016-02-02 23:43:10]

Amazing. This seems really good to me, thank you! :-)

There are a few things that need to happen before we can land this:

- Please send out the Intent to Implement and Ship, and wait until it has three
LGTMs. (We'll effectively ship the feature once this lands, so the waiting is
significant.) Link to the e-mail in the CLs description.

- Please update the following layout test that verifies end-to-end database
serialization:

third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html

- There are a whole series of global interface listing layout tests that have to
be updated with the new values:

third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-dedicated-worker-expected.txt
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-shared-worker-expected.txt

https://codereview.chromium.org/1656243002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1656243002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data.proto:49: optional bool
renotify = 13;
nit: here and everywhere else, please keep the ordering aligned with the spec:

https://notifications.spec.whatwg.org/#api

(i.e. `renotify` goes under `timestamp` but above `silent`.)

https://codereview.chromium.org/1656243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationData.cpp
(right):

https://codereview.chromium.org/1656243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationData.cpp:43:
exceptionState.throwTypeError("Notifications which set the renotify flag must
specify a valid tag.");
a valid tag -> a non-empty tag

The only validity we care about it that it's non-zero in length, so we can be
specific here.

[harkness, 2016-02-03 11:24:28]

https://codereview.chromium.org/1656243002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database_data.proto (right):

https://codereview.chromium.org/1656243002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database_data.proto:49: optional bool
renotify = 13;
On 2016/02/02 23:43:10, Peter Beverloo wrote:
> nit: here and everywhere else, please keep the ordering aligned with the spec:
> 
> https://notifications.spec.whatwg.org/#api
> 
> (i.e. `renotify` goes under `timestamp` but above `silent`.)

Done.

https://codereview.chromium.org/1656243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/NotificationData.cpp
(right):

https://codereview.chromium.org/1656243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/NotificationData.cpp:43:
exceptionState.throwTypeError("Notifications which set the renotify flag must
specify a valid tag.");
On 2016/02/02 23:43:10, Peter Beverloo wrote:
> a valid tag -> a non-empty tag
> 
> The only validity we care about it that it's non-zero in length, so we can be
> specific here.

Done.

[Peter Beverloo, 2016-02-03 11:34:38]

lgtm % passing the Intent to Implement and Ship, and mentioning it in the CL
description

https://codereview.chromium.org/1656243002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html
(right):

https://codereview.chromium.org/1656243002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:42:
renotify: false,
false -> true

"false" is the default value, so if something went wrong in the serialization
step it would still end up being false. Testing for true requires the full data
round-trip to work.

[harkness, 2016-02-03 13:45:44]

Just waiting on the Implement and Ship now, then I'll send it out to other
reviewers.

Could you kick off trybots for me?

https://codereview.chromium.org/1656243002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html
(right):

https://codereview.chromium.org/1656243002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/notifications/serviceworker-notification-properties.html:42:
renotify: false,
On 2016/02/03 11:34:38, Peter Beverloo wrote:
> false -> true
> 
> "false" is the default value, so if something went wrong in the serialization
> step it would still end up being false. Testing for true requires the full
data
> round-trip to work.

Done.

[Peter Beverloo, 2016-02-03 13:49:34]

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

[commit-bot: I haz the power, 2016-02-03 13:50:34]

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

[commit-bot: I haz the power, 2016-02-03 15:42:39]

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

[commit-bot: I haz the power, 2016-02-03 15:42:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Michael van Ouwerkerk, 2016-02-04 10:19:21]

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

[commit-bot: I haz the power, 2016-02-04 10:19:58]

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

[commit-bot: I haz the power, 2016-02-04 11:46:36]

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

[commit-bot: I haz the power, 2016-02-04 11:46:37]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-02-08 09:58:43]

Description was changed from

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

BUG=442133
==========

to

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Implement to Implement and Ship:
Notification Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
==========

[harkness, 2016-02-08 10:06:34]

harkness@chromium.org changed reviewers:
+ avi@chromium.org, dewittj@chromium.org, mkwst@chromium.org, philipj@opera.com

[harkness, 2016-02-08 10:06:35]

This code is waiting for LGTM3 on Intent to Implement and Ship, but I wanted to
get the ball rolling on OWNERS review. I won't submit until I get LGTM3.

dewittj@chromium.org: Please review changes in ui/message_center

mkwst@chromium.org: Please review changes in
content/common/platform_notification_messages.h

avi@chromium.org: Please review changes in
content/public/common/platform_notification_data.h

philipj@opera.com: Please review changes in third_party/WebKit/Layout
Tests/virtual/stable

[philipj_slow, 2016-02-08 11:09:17]

third_party/WebKit/LayoutTests/virtual/stable LGTM

[Michael van Ouwerkerk, 2016-02-08 17:18:22]

lgtm with nit

https://codereview.chromium.org/1656243002/diff/100001/content/child/notifica...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1656243002/diff/100001/content/child/notifica...
content/child/notifications/notification_data_conversions_unittest.cc:84:
TEST(NotificationDataConversionsTest, ToWebNotificationData) {
Please also add coverage for this case.

[Avi (use Gerrit), 2016-02-08 17:23:54]

content/public lgtm

[dewittj, 2016-02-08 17:42:11]

ui/message_center lgtm

[harkness, 2016-02-09 11:06:30]

https://codereview.chromium.org/1656243002/diff/100001/content/child/notifica...
File content/child/notifications/notification_data_conversions_unittest.cc
(right):

https://codereview.chromium.org/1656243002/diff/100001/content/child/notifica...
content/child/notifications/notification_data_conversions_unittest.cc:84:
TEST(NotificationDataConversionsTest, ToWebNotificationData) {
On 2016/02/08 17:18:22, Michael van Ouwerkerk wrote:
> Please also add coverage for this case.

Done.

[Michael van Ouwerkerk, 2016-02-09 17:36:42]

still lgtm

[Michael van Ouwerkerk, 2016-02-10 10:08:30]

Description was changed from

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Implement to Implement and Ship:
Notification Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
==========

to

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Intent to Implement and Ship: Notification
Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
==========

[Michael van Ouwerkerk, 2016-02-10 10:14:15]

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

[Michael van Ouwerkerk, 2016-02-10 10:14:16]

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

[Michael van Ouwerkerk, 2016-02-10 10:17:11]

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

[commit-bot: I haz the power, 2016-02-10 10:17:18]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-02-10 15:19:53]

ipc lgtm

[harkness, 2016-02-10 16:55:54]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-02-10 16:55:54]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
avi@chromium.org, philipj@opera.com, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1656243002/#ps120001
(title: "Added unit tests")

[commit-bot: I haz the power, 2016-02-10 16:56:08]

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

[commit-bot: I haz the power, 2016-02-10 17:12:10]

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

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

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...)

[harkness, 2016-02-10 17:32:04]

The CQ bit was checked by harkness@chromium.org

[harkness, 2016-02-10 17:32:04]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
peter@chromium.org, philipj@opera.com, kenrb@chromium.org,
mvanouwerkerk@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1656243002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-10 17:34:26]

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

[commit-bot: I haz the power, 2016-02-10 20:15:34]

Description was changed from

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Intent to Implement and Ship: Notification
Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
==========

to

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Intent to Implement and Ship: Notification
Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
==========

[commit-bot: I haz the power, 2016-02-10 20:15:35]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:30:41]

Description was changed from

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Intent to Implement and Ship: Notification
Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133
==========

to

==========
Implementation of renotify flag for Notifications.

Implement and plumb the renotify flag. This flag controls whether alert sound,
lights,
and vibration are played again when a notification replaces a previous
notification.

Changes for this issue are covered by Intent to Implement and Ship: Notification
Renotify.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/k5n129-lM1A

BUG=442133

Committed: https://crrev.com/39c49fe6e8dfe2f2ae3c7db261cb8e845692dfd2
Cr-Commit-Position: refs/heads/master@{#374709}
==========

[commit-bot: I haz the power, 2016-02-16 22:30:42]

Patchset 8 (id:??) landed as
https://crrev.com/39c49fe6e8dfe2f2ae3c7db261cb8e845692dfd2
Cr-Commit-Position: refs/heads/master@{#374709}