Ship the Notification.action and Notification.vibration properties

Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

Intent to ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/0mBO8Q5FhTo/2OagzgagBgAJ

BUG=547716, 442132
Committed: https://crrev.com/b16af0ae12f9d37558e95cb92222c4313df098c0
Cr-Commit-Position: refs/heads/master@{#403191}

[Peter Beverloo, 2016-05-12 14:54:14]

peter@chromium.org changed reviewers:
+ bashi@chromium.org, domenic@chromium.org, haraken@chromium.org,
johnme@chromium.org

[Peter Beverloo, 2016-05-12 14:54:14]

+johnme for notifications
+bashi/haraken for bindings and the updated Notification::actions implementation
+domenic for strict equality behaviour

PTAL

Even if we don't technically violate the specification, reference equality
evaluating to false strikes me as unexpected.

Are we OK with this behaviour, or should we find a solution for this first?

(I'll send an Intent to Ship and get API owner approval when we settle on an
answer.)

[haraken, 2016-05-12 14:56:31]

bindings/ LGTM.

> `notification.actions === notification.actions`
> `notification.vibrate === notification.vibrate`

bashi-san?

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

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

[Michael van Ouwerkerk, 2016-05-12 15:05:00]

\o/ drive-by cheers and nit :-)

https://codereview.chromium.org/1974033003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html
(right):

https://codereview.chromium.org/1974033003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html:63:
assert_array_equals(notification.vibrate, []);
nit: use emptyNotification here

[Peter Beverloo, 2016-05-12 15:10:14]

Thank you Michael!

https://codereview.chromium.org/1974033003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html
(right):

https://codereview.chromium.org/1974033003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/notification-properties.html:63:
assert_array_equals(notification.vibrate, []);
On 2016/05/12 15:04:59, Michael van Ouwerkerk wrote:
> nit: use emptyNotification here

Done.

[domenic, 2016-05-12 15:53:15]

> to also freeze the individual NotificationAction objects contained therein

This is a spec violation

> This, according to bashi@, is not actually a violation of the spec:

It is not a violation of the FrozenArray spec, but it is a violation of the
[SameObject] spec:

> [SameObject] readonly attribute FrozenArray<NotificationAction> actions;

I think vibrate should have [SameObject] as well; I will file a spec issue.

Please do not ship with this === behavior, nor with the extra non-spec-compliant
freezing of NotificationAction objects.

[domenic, 2016-05-12 15:53:50]

On 2016/05/12 at 15:53:15, domenic wrote:
> > to also freeze the individual NotificationAction objects contained therein
> 
> This is a spec violation

I'm sorry, I was wrong on this and did not read the spec closely. It is strange
that the spec freezes these, but I guess it does. My apologies.

The non-[SameObject] behavior is still problematic.

[Peter Beverloo, 2016-05-12 17:40:50]

Description was changed from

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

One potential point of contention is that the strict equality operator
does not work as one might expect. That means that the following two
expressions evaluate to false:

  `notification.actions === notification.actions`
  `notification.vibrate === notification.vibrate`

This, according to bashi@, is not actually a violation of the spec:
  https://codereview.chromium.org/1890733002

BUG=547716, 442132
==========

to

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

BUG=547716, 442132
==========

[Peter Beverloo, 2016-05-12 17:41:46]

I have added [SameObject] to both properties by storing them in ScriptValue
class-members, but am not sure whether this is the right thing to do.

bashi, harakan, would you have guidance here? (Notification.cpp/h)

I'll follow-up on the spec side of things in the meantime.

[bashi, 2016-05-12 23:34:31]

On 2016/05/12 17:41:46, Peter Beverloo wrote:
> I have added [SameObject] to both properties by storing them in ScriptValue
> class-members, but am not sure whether this is the right thing to do.
> 
> bashi, harakan, would you have guidance here? (Notification.cpp/h)
> 
> I'll follow-up on the spec side of things in the meantime.

Basically you shouldn't use ScriptValue just for [SameObject], but we haven't
implemented [SameObject] so this would be a workaround.

If you aren't in a rush, it would be better to implement [SameObject] in the
binding layer first. It would take 2-3 days. WDYT?

[bashi, 2016-05-12 23:35:09]

bashi@chromium.org changed reviewers:
+ yukishiino@chromium.org

[bashi, 2016-05-12 23:35:10]

+yukishiino@

[Yuki, 2016-05-13 15:23:11]

On 2016/05/12 23:35:10, bashi1 wrote:
> +yukishiino@

Just started to implement [SameObject] at http://crrev.com/1980553002 .
Will continue to work on this next week.

[Peter Beverloo, 2016-05-16 12:47:52]

Fantastic, I rebased on top of your CL yukishiino@! :D

Would you mind confirming my changes in Notification.cpp? I'll get the spec in
sync and send out an Intent to Ship in the meantime.

[Yuki, 2016-05-16 13:23:08]

LGTM.  Sorry for having you guys wait for a long time, both FrozenArray and
[SameObject].  My CL will be landed soon.

[Yuki, 2016-05-17 02:09:47]

On 2016/05/16 13:23:08, Yuki wrote:
> LGTM.  Sorry for having you guys wait for a long time, both FrozenArray and
> [SameObject].  My CL will be landed soon.

Just FYI, my CL http://crrev.com/1980553002 got landed.

[bashi, 2016-05-17 02:42:12]

https://codereview.chromium.org/1974033003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/1974033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:334: actions[i]
= freezeV8Object(toV8(action, scriptState), scriptState->isolate());
Not directly related to this CL but it seems that the notification spec depends
on ECMAScript. Generally speaking IDL doesn't (and shouldn't) depend on any
specific language so this requirement sounds like a layer violation to me.

We may want to file a bug to WebIDL to add a feature to freeze an object, e.g.
FrozenArray<Frozen<NotificationAction>>.

Just my 2 cents.

[Michael van Ouwerkerk, 2016-05-23 10:33:03]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org

[Yuki, 2016-05-27 13:10:46]

Sorry for the revert of my CL.  I landed a new support of [SameObject] and
[SaveSameObject] at https://crrev.com/2008823002 .

Could you use [SaveSameObject] in your IDL?

[Peter Beverloo, 2016-06-29 18:06:09]

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

[Peter Beverloo, 2016-06-29 18:06:09]

Yuki: Please take another look— done, and tests seem to pass :)

+Rick for Web Exposed tests

[Rick Byers, 2016-06-29 18:20:31]

Description was changed from

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

BUG=547716, 442132
==========

to

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

Intent to ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/0mBO8Q5FhTo/2Oagzgag...

BUG=547716, 442132
==========

[Rick Byers, 2016-06-29 18:24:17]

On 2016/06/29 18:06:09, Peter Beverloo wrote:
> Yuki: Please take another look— done, and tests seem to pass :)
> 
> +Rick for Web Exposed tests

I added a link to the intent thread in your CL description.

webexposed LGTM
platform/ also LGTM (since it looks like you don't yet have an OWNER for that).

Thanks for making the effort to do this right everyone!

[Rick Byers, 2016-06-29 18:25:06]

Also, please be sure to update the chromestatus entry with the appropriate
milestone (guessing you may merge to 53 if this misses the branch?).

[Yuki, 2016-06-30 03:43:12]

LGTM.  Thank you for patiently working on this.

[Peter Beverloo, 2016-06-30 15:37:53]

Thanks Rick and Yuki! I'll update the ChromeStatus entry once this lands.

[Peter Beverloo, 2016-06-30 15:38:06]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-06-30 15:38:07]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1974033003/#ps80001
(title: "Ship the Notification.action and Notification.vibration properties")

[commit-bot: I haz the power, 2016-06-30 15:38:18]

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-06-30 16:16:28]

Description was changed from

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

Intent to ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/0mBO8Q5FhTo/2Oagzgag...

BUG=547716, 442132
==========

to

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

Intent to ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/0mBO8Q5FhTo/2Oagzgag...

BUG=547716, 442132
==========

[commit-bot: I haz the power, 2016-06-30 16:16:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-30 16:20:00]

Description was changed from

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

Intent to ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/0mBO8Q5FhTo/2Oagzgag...

BUG=547716, 442132
==========

to

==========
Ship the Notification.action and Notification.vibration properties

This CL changes the return types of both properties to FrozenArray<>,
amends Notification.actions to also freeze the individual
NotificationAction objects contained therein, and enables the properties
by default.

In addition, [SameObject] semantics have been added to the two
properties, which now mean that they will continue to return the same
value, making the equality operator work as expected.

Intent to ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/0mBO8Q5FhTo/2Oagzgag...

BUG=547716, 442132

Committed: https://crrev.com/b16af0ae12f9d37558e95cb92222c4313df098c0
Cr-Commit-Position: refs/heads/master@{#403191}
==========

[commit-bot: I haz the power, 2016-06-30 16:20:01]

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