Added reply field to blink NotificationEvent (not yet piped through)

Added reply field to blink NotificationEvent (not yet piped through)

Added a blink test for notificationevent.reply

BUG=599859
Committed: https://crrev.com/1b8bafc32800ae1a24795369a9e694adf78ba6d3
Cr-Commit-Position: refs/heads/master@{#422116}

[awdf, 2016-09-30 10:41:18]

Description was changed from

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a (failing) test for notificationevent.reply

BUG=
==========

to

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a blink test for notificationevent.reply

BUG=
==========

[awdf, 2016-09-30 10:43:26]

Description was changed from

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a blink test for notificationevent.reply

BUG=
==========

to

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a blink test for notificationevent.reply

BUG=599859
==========

[awdf, 2016-09-30 10:43:26]

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

[Peter Beverloo, 2016-09-30 11:10:22]

lgtm

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/resources/serviceworker-notification-event.js
(right):

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/resources/serviceworker-notification-event.js:32:
assert_equals(event.action, '');
nit: assert that |event.reply| is either an empty string or NULL?

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationEvent.idl
(right):

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationEvent.idl:14:
readonly attribute DOMString reply;
If we were to land this as-is, then the following feature check would pass
regardless of whether inline replies are enabled:

    NotificationEvent.prototype.hasOwnProperty('reply')

That breaks feature detection, and will confuse developers. Instead, we should
guard this behind the following annotation:

    [RuntimeEnabled=NotificationInlineReplies]

That means that the attribute will only be exposed if the
NotificationInlineReplies feature is enabled, which is, in turn, guarded by the
--enable-experimental-web-platform-features command line flag.

Because this changes the Web Exposed API surface, even if it's not in stable,
you'll also need to update another file:

   
LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt

That file lists all the interfaces, as well as their methods and attributes,
that are exposed by Blink.

(I'd strongly recommend that you just run try jobs immediately after uploading a
CL, or a new version of a CL. They're great at identifying these things :-))

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationEventInit.idl
(right):

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationEventInit.idl:10:
DOMString action = null;
You're changing the default value from `action` to NULL, and add `reply` as an
empty string. It seems to me like defaulting both to empty strings is the way to
go, following the spec?

(Or do you think NULL as the default value would make more sense?)

[awdf, 2016-09-30 12:14:15]

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

[commit-bot: I haz the power, 2016-09-30 12:14:23]

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

[awdf, 2016-09-30 12:31:42]

just running the trybots on patch 1 to see what kind of error message I would
have got if you hadn't told me..

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/notifications/NotificationEventInit.idl
(right):

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/notifications/NotificationEventInit.idl:10:
DOMString action = null;
On 2016/09/30 11:10:22, Peter Beverloo wrote:
> You're changing the default value from `action` to NULL, and add `reply` as an
> empty string. It seems to me like defaulting both to empty strings is the way
to
> go, following the spec?
> 
> (Or do you think NULL as the default value would make more sense?)

oops! I meant to set reply = null, so that a deliberately empty reply could be
distinguished. But given the developer can check if the feature is enabled by
the presence of the field at all, there seems no point in distinguishing empty
string from null in this case, so I'll change it to an empty string, like with
action.

[awdf, 2016-09-30 12:33:59]

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

[commit-bot: I haz the power, 2016-09-30 12:34:21]

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

[awdf, 2016-09-30 13:52:54]

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/notifications/resources/serviceworker-notification-event.js
(right):

https://codereview.chromium.org/2380273002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/notifications/resources/serviceworker-notification-event.js:32:
assert_equals(event.action, '');
On 2016/09/30 11:10:22, Peter Beverloo wrote:
> nit: assert that |event.reply| is either an empty string or NULL?

Done.

[awdf, 2016-09-30 13:53:21]

The CQ bit was checked by awdf@chromium.org

[awdf, 2016-09-30 13:53:21]

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/2380273002/#ps40001
(title: "Add a test for default value of notificationevent.reply")

[commit-bot: I haz the power, 2016-09-30 13:53:31]

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-09-30 15:20:37]

Description was changed from

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a blink test for notificationevent.reply

BUG=599859
==========

to

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a blink test for notificationevent.reply

BUG=599859
==========

[commit-bot: I haz the power, 2016-09-30 15:20:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-30 15:22:43]

Description was changed from

==========
Added reply field to blink NotificationEvent (not yet piped through)


Added a blink test for notificationevent.reply

BUG=599859
==========

to

==========
Added reply field to blink NotificationEvent (not yet piped through)

Added a blink test for notificationevent.reply

BUG=599859

Committed: https://crrev.com/1b8bafc32800ae1a24795369a9e694adf78ba6d3
Cr-Commit-Position: refs/heads/master@{#422116}
==========

[commit-bot: I haz the power, 2016-09-30 15:22:44]

Patchset 3 (id:??) landed as
https://crrev.com/1b8bafc32800ae1a24795369a9e694adf78ba6d3
Cr-Commit-Position: refs/heads/master@{#422116}