Generalize validation of developer input for Web Notifications

Generalize validation of developer input for Web Notifications

Next to removing a whole bunch of duplicated logic, this CL
also adds unit tests verifying the developer-data to a
WebNotificationData conversion.

Additionally, as a follow-up on the following CL, the "auto"
value will now be propagated as expected for persistent notifications.

https://codereview.chromium.org/1262023003/

BUG=516339
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200150

[Peter Beverloo, 2015-08-03 14:55:27]

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

[Peter Beverloo, 2015-08-03 14:55:28]

+johnme, please take a look.

[johnme, 2015-08-03 15:41:36]

Nice, much cleaner :)

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:256: // TODO(peter): The
specification requires this to be [SameObject] - match this.
Is this a change from before, or an old TODO that still applies?

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/Notification.h:99: HeapVector<NotificationAction>
actions() const;
does returning a new HeapVector each time affect whether the sequence is the
same object / is immutable?

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/NotificationData.cpp (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/NotificationData.cpp:65: const size_t maxActions =
Notification::maxActions();
Nit: seems more legible if you don't inline this.

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/NotificationData.cpp:72: // TODO(johnme): Ensure
that |action.action()| isn't empty.
https://codereview.chromium.org/1263043003 has been on the CQ for 2 hours, and
should land soon. You'll probably need to rebase this accordingly.

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/NotificationDataTest.cpp (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/NotificationDataTest.cpp:159: // The stored actions
will be capped to |maxActions| entries.
Might be nice to test that it's the first N entries that get used.

[Peter Beverloo, 2015-08-03 17:29:30]

PTAL

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/Notification.cpp:256: // TODO(peter): The
specification requires this to be [SameObject] - match this.
On 2015/08/03 15:41:36, johnme wrote:
> Is this a change from before, or an old TODO that still applies?

TODO that still applies. I don't think we're doing the right thing now. (We need
to cache the ScriptValue, not the serialized data.)

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/Notification.h:99: HeapVector<NotificationAction>
actions() const;
On 2015/08/03 15:41:36, johnme wrote:
> does returning a new HeapVector each time affect whether the sequence is the
> same object / is immutable?

It affects whether it returns the same object. Since we haven't shipped this yet
I'd rather solve it together with the data and vibration pattern above (we need
to cache these values).

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/NotificationData.cpp (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/NotificationData.cpp:65: const size_t maxActions =
Notification::maxActions();
On 2015/08/03 15:41:36, johnme wrote:
> Nit: seems more legible if you don't inline this.

Acknowledged.

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/NotificationData.cpp:72: // TODO(johnme): Ensure
that |action.action()| isn't empty.
On 2015/08/03 15:41:36, johnme wrote:
> https://codereview.chromium.org/1263043003 has been on the CQ for 2 hours, and
> should land soon. You'll probably need to rebase this accordingly.

Done.

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
File Source/modules/notifications/NotificationDataTest.cpp (right):

https://codereview.chromium.org/1260793007/diff/20001/Source/modules/notifica...
Source/modules/notifications/NotificationDataTest.cpp:159: // The stored actions
will be capped to |maxActions| entries.
On 2015/08/03 15:41:36, johnme wrote:
> Might be nice to test that it's the first N entries that get used.

Done.

[johnme, 2015-08-06 10:31:43]

lgtm

[Peter Beverloo, 2015-08-07 04:52:02]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-08-07 04:52:02]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/1260793007/#ps120001
(title: "fix actions")

[commit-bot: I haz the power, 2015-08-07 04:52:17]

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

[commit-bot: I haz the power, 2015-08-07 06:04:28]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200150