New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Launch-OWP
Launch-Accessibility: ----
Launch-Exp-Leadership: ----
Launch-Leadership: ----
Launch-Legal: ----
Launch-M-Approved: ----
Launch-M-Target: ----
Launch-Privacy: ----
Launch-Security: ----
Launch-Test: ----
Launch-UI: ----

Blocking:
issue 674593


Participants' hotlists:
Non-Standard-IDL


Sign in to add a comment
link

Issue 659074: Extend MessageEvent to supplant ServiceWorkerMessageEvent

Reported by jungkee....@samsung.com, Oct 25 2016

Issue description

HTML extends MessageEvent to allow ServiceWorker as the type of source attribute: https://github.com/whatwg/html/pull/1944

Service worker will replace ServiceWorkerMessageEvent with this extended MessageEvent.

Related discussion: https://github.com/w3c/ServiceWorker/issues/989
 

Comment 1 by jungkee....@samsung.com, Oct 28 2016

Looking at the code, MessageEvent needs some work apart from the changes proposed in the OP.

Currently, the type of |source| attribute is EventTarget instead of the union. I'm not sure whether it'd be OK to use EventTarget for ServiceWorker too like by adding source->toServiceWorker() in isValidSource(EventTarget* source) function. That'd be the easiest way to tackle this change, but I guess service worker folks might not be happy to do so as ServiceWorkerMessageEvent was implementing the union.

So, I'd like to hear comments on how to tackle the issues:
 - Just use EventTarget as-is and improve MessageEvent in a separate issue/patch
 - Improve MessageEvent first and then tackle the issue in the OP.

/cc nhiroki@, jinho.bang@, foolip@, rbyers@

Comment 2 by jungkee....@samsung.com, Oct 28 2016

Cc: rbyers@chromium.org nhiroki@chromium.org

Comment 3 by nhiroki@chromium.org, Oct 28 2016

Components: Blink>Messaging
From SW's point of view, I'd prefer to improve MessageEvent first if it's not a tough job so that we can simply transplant code from ServiceWorkerMessageEvent to MessageEvent.

Comment 4 by jungkee....@samsung.com, Oct 28 2016

I'll try to do it using WindowOrMessagePortOrServiceWorker idl-gen-union'd type. If you have any better idea, please let me know.

Comment 5 by nhiroki@chromium.org, Oct 31 2016

It'd be the optimal way for now. There is an issue to refine the idl-generator to avoid such a verbose union type, but it's not fixed yet:
https://bugs.chromium.org/p/chromium/issues/detail?id=611545

Comment 6 by jungkee....@samsung.com, Oct 31 2016

I encountered some errors while trying this with WindowOrMessagePortOrServiceWorker class generated from MessageEvent.idl. Please see (WIP) CL: https://codereview.chromium.org/2466513002/ Comments would be appreciated.

Comment 7 by jungkee....@samsung.com, Nov 16 2016

Labels: -Type-Bug Type-Launch-OWP

Comment 8 by bugdroid1@chromium.org, Dec 14 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/faa6203e513b171743c9d9fd7121501e681186af

commit faa6203e513b171743c9d9fd7121501e681186af
Author: jungkee.song <jungkee.song@samsung.com>
Date: Wed Dec 14 18:38:17 2016

Deprecate ServiceWorkerMessageEvent in favor of MessageEvent

This extends MessageEvent to allow ServiceWorker as a type of source
attribute according to the changes in HTML and replaces the existing
ServiceWorkerMessageEvent code usage with this extended MessageEvent.

Related spec issue: https://github.com/w3c/ServiceWorker/issues/989
Related spec change:
 - HTML: https://github.com/whatwg/html/pull/1944
 - SW: https://github.com/w3c/ServiceWorker/commit/6c68bd87bdfd153186a5b9904e31676c56730e2f
Related WPT: https://github.com/w3c/web-platform-tests/pull/4186

Intent to Deprecate and Remove: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Xp9hmKyuOrI

BUG= 659074 

Review-Url: https://codereview.chromium.org/2466513002
Cr-Commit-Position: refs/heads/master@{#438545}

[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt
[delete] https://crrev.com/45369974f8ad223059462980229a9735e6ee6865/third_party/WebKit/LayoutTests/fast/events/constructors/serviceworker-message-event-constructor-expected.txt
[delete] https://crrev.com/45369974f8ad223059462980229a9735e6ee6865/third_party/WebKit/LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/LayoutTests/http/tests/serviceworker/postmessage-to-client.html
[delete] https://crrev.com/45369974f8ad223059462980229a9735e6ee6865/third_party/WebKit/LayoutTests/http/tests/serviceworker/serviceworker-message-event.html
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/LayoutTests/imported/wpt/html/browsers/origin/cross-origin-objects/cross-origin-objects-exceptions-expected.txt
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/bindings/modules/BUILD.gn
[delete] https://crrev.com/45369974f8ad223059462980229a9735e6ee6865/third_party/WebKit/Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/bindings/modules/v8/custom/custom.gni
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/core/events/EventTarget.cpp
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/core/events/EventTarget.h
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/core/events/MessageEvent.cpp
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/core/events/MessageEvent.h
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/core/events/MessageEvent.idl
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/modules/modules_idl_files.gni
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/modules/serviceworkers/BUILD.gn
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.h
[modify] https://crrev.com/faa6203e513b171743c9d9fd7121501e681186af/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
[delete] https://crrev.com/45369974f8ad223059462980229a9735e6ee6865/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp
[delete] https://crrev.com/45369974f8ad223059462980229a9735e6ee6865/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.h

Comment 9 by falken@chromium.org, Dec 19 2016

Cc: dk...@chromium.org
Labels: M-57
Status: Started (was: Assigned)
This landed and will ship in M57. Who tracks Launch-OWP? Should I leave this in Status: Starting?

Comment 11 by foolip@chromium.org, Dec 19 2016

Cc: jmedley@chromium.org
jmedley@ should be able to advise on how to handle Launch-OWP.

Comment 12 by nhiroki@chromium.org, Feb 24 2017

Any updates on this? Can I mark this as Fixed?

Comment 13 by falken@chromium.org, Feb 24 2017

Status: Fixed (was: Started)
I guess so.

Comment 14 by foolip@chromium.org, Feb 24 2017

Status: Assigned (was: Fixed)
ServiceWorkerMessageEvent.idl and related files still exist, and it looks like a script could still call new ServiceWorkerMessageEvent(). The interface itself should also be removed, per https://groups.google.com/a/chromium.org/d/msg/blink-dev/Xp9hmKyuOrI/kKd6IuxPAwAJ

Comment 15 by jungkee....@samsung.com, Feb 24 2017

Oh I think I missed the IDL files. I'll fix them. Thanks for the notice.

Comment 16 by falken@chromium.org, Feb 24 2017

Labels: M-58
Status: Started (was: Assigned)
Good catch foolip@.

So for milestone-based platform change tracking purposes:
- In 57, onmessage takes a MessageEvent instead of ServiceWorkerMessageEvent
- In 58, ServiceWorkerMessageEvent interface will be removed

Comment 17 by jungkee....@samsung.com, Feb 24 2017

foolip@, falken@,

> it looks like a script could still call new ServiceWorkerMessageEvent()

I checked https://codereview.chromium.org/2466513002 and tested again. ServiceWorkerMessageEvent.idl and ServiceWorkerMessageEventInit.idl were excluded from the build. Accessing the constructor raises uncaught ReferenceError. (self.ServiceWorkerMessageEvent returns undefined.) If you encountered any particular case where you could access the constructor, please share it with me. Otherwise, I think I can just remove those idl files.

Additionally, I found this reference:
third_party/WebKit/Source/devtools/devtools-node-modules/third_party/node_modules/eslint/node_modules/globals/globals.json:1221:	"ServiceWorkerMessageEvent": false,

I guess this should also be removed?

Comment 18 by falken@chromium.org, Feb 24 2017

That looks like third-party imported code. Probably it could be removed but likely can't be part of a Chromium commit.

I notice references in bindings/. These could probably be removed as well:
$ git gs ServiceWorkerMessageEvent
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:120:    friend class V8ServiceWorkerMessageEventInternal;
third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h:5:#ifndef V8ServiceWorkerMessageEventInternal_h
third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h:6:#define V8ServiceWorkerMessageEventInternal_h
third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h:14:class V8ServiceWorkerMessageEventInternal {
third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h:25:void V8ServiceWorkerMessageEventInternal::constructorCustom(
third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h:73:void V8ServiceWorkerMessageEventInternal::dataAttributeGetterCustom(
third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h:111:#endif  // V8ServiceWorkerMessageEventInternal_h
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:8:#include "bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h"
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:  V8ServiceWorkerMessageEventInternal::constructorCustom<
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:20:  V8ServiceWorkerMessageEventInternal::dataAttributeGetterCustom<
third_party/WebKit/Source/bindings/modules/v8/v8.gni:24:                    "V8ServiceWorkerMessageEventInternal.h",
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:11:    // Constructor(DOMString type, optional ServiceWorkerMessageEventInit eventInitDict),
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:14:] interface ServiceWorkerMessageEvent : Event {
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7:dictionary ServiceWorkerMessageEventInit : EventInit {

Comment 19 by foolip@chromium.org, Feb 24 2017

OK, I was just guessing based on the existence of the IDL file, turns out it was already removed from third_party/WebKit/Source/bindings/modules/BUILD.gn and is now just a stray file in the repo.

I'm not sure what the devtools file is, I'd git blame to find someone who knows.

Comment 20 by jungkee....@samsung.com, Feb 24 2017

V8ExtendableMessageEventCustom.cpp is referencing definitions in V8ServiceWorkerMessageEventInternal.h. Removing the dependency there and remove V8ServiceWorkerMessageEventInternal.h would be great. Let me look at it.

Comment 21 by lunalu@chromium.org, Mar 31 2017

Blocking: 674593
Is there any plan to standardize ServiceWorkerMessageEvent and ServiceWorkerMessageEventInit?

Comment 22 by mek@chromium.org, Mar 31 2017

as far as I know, the plan (as described in this bug) is the opposite; to get rid of ServiceWorkerMessageEvent and replace them with regular MessageEvent. Reading back through these comments it seems like that even already happened, and just for some reason the actual IDL files haven't been removed yet? Not sure what happened between February and now.

Comment 23 by jmedley@google.com, Apr 25 2017

I was about to send an email on the same question. Does anyone know what's going on?

Comment 24 by foolip@chromium.org, Apr 25 2017

https://codereview.chromium.org/2837353003 is an attempt to finish and close this issue.

Comment 25 by foolip@chromium.org, Apr 26 2017

Labels: -M-58
The actual removal was in https://codereview.chromium.org/2466513002 which landed in M-57, so removing the M-58 label.

Comment 26 by bugdroid1@chromium.org, Apr 26 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2369b7f3dec2f78a760ed52bf64badb2240a1968

commit 2369b7f3dec2f78a760ed52bf64badb2240a1968
Author: foolip <foolip@chromium.org>
Date: Wed Apr 26 10:34:04 2017

Remove remaining traces of ServiceWorkerMessageEvent

The ServiceWorkerMessageEvent.* files were already removed from BUILD.gn:
https://codereview.chromium.org/2466513002

This inlines the templatized helpers in V8ServiceWorkerMessageEventInternal.h
into non-templated code in V8ExtendableMessageEventCustom.cpp, which is now the
only place the helpers are used.

BUG= 659074 

Review-Url: https://codereview.chromium.org/2837353003
Cr-Commit-Position: refs/heads/master@{#467291}

[modify] https://crrev.com/2369b7f3dec2f78a760ed52bf64badb2240a1968/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
[delete] https://crrev.com/29b3b5cf99713f88fd9b5739dcbb2f476e2721e0/third_party/WebKit/Source/bindings/modules/v8/V8ServiceWorkerMessageEventInternal.h
[modify] https://crrev.com/2369b7f3dec2f78a760ed52bf64badb2240a1968/third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
[modify] https://crrev.com/2369b7f3dec2f78a760ed52bf64badb2240a1968/third_party/WebKit/Source/bindings/modules/v8/v8.gni
[delete] https://crrev.com/29b3b5cf99713f88fd9b5739dcbb2f476e2721e0/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl
[delete] https://crrev.com/29b3b5cf99713f88fd9b5739dcbb2f476e2721e0/third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl

Comment 27 by foolip@chromium.org, Apr 26 2017

Status: Fixed (was: Started)

Sign in to add a comment