ServiceWorker: Introduce ServiceWorkerMessageEvent to replace MessageEvent (3/3).

ServiceWorker: Introduce ServiceWorkerMessageEvent to replace MessageEvent (3/3).

The message sent from Service Worker to its client will land at
navigator.serviceWorker.onmessage. The event type will be a new
ServiceWorkerMessageEvent which 'source' attribute could be
ServiceWorker or MessagePort.

spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#serviceworkermessage-event-section

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

[xiang, 2015-05-15 06:01:22]

xiang.long@intel.com changed reviewers:
+ falken@chromium.org, jsbell@chromium.org

[xiang, 2015-05-15 06:01:23]

PTAL, thanks.

[falken, 2015-05-18 03:40:22]

falken@chromium.org changed reviewers:
+ haraken@chromium.org

[falken, 2015-05-18 03:40:23]

We'll need an Intent to Ship strategy before landing.

Code looks reasonable, but I'm worried about all the custom bindings code. Is
there a better way? +haraken

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
File
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html
(right):

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html:4:
<script src="../../../resources/js-test.js"></script>
Kind of unfortunate that this won't be upstreamed. But it's fine for now: it
makes sense not to put this in http/tests and we have a ways to go to finally
upstream the tests we already have.

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html:70:
// FIXME: ServiceWorker object could not be created without http server.
Why not just test this separately in an http test?

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
File LayoutTests/fast/events/init-serviceworker-message-event.html (right):

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/init-serviceworker-message-event.html:11: function
runTest() {
why is this a function?

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/init-serviceworker-message-event.html:12: port = new
MessageChannel().port1;
let's make this a var

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl (right):

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:8:
CustomConstructor(DOMString type, optional ServiceWorkerMessageEventInit
eventInitDict),
CustomConstructor is listed as "strongly discouraged"
https://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Custo...

Can you explain why CustomConstructor is needed?

[haraken, 2015-05-18 03:43:00]

On 2015/05/18 03:40:23, falken wrote:
> We'll need an Intent to Ship strategy before landing.
> 
> Code looks reasonable, but I'm worried about all the custom bindings code. Is
> there a better way? +haraken

That looks unreasonable.

Would you elaborate on why you think each custom binding is needed? We can help
remove them.

[xiang, 2015-05-18 06:07:53]

On 2015/05/18 03:43:00, haraken wrote:
> On 2015/05/18 03:40:23, falken wrote:
> > We'll need an Intent to Ship strategy before landing.
> > 
> > Code looks reasonable, but I'm worried about all the custom bindings code.
Is
> > there a better way? +haraken
> 
> That looks unreasonable.
> 
> Would you elaborate on why you think each custom binding is needed? We can
help
> remove them.

The reason to use "any" instead of (ServiceWorker or MessagePort) is IDL union
type doesn't support dictionary access.
You can check a sample CL: https://codereview.chromium.org/1139873002/.

An alternative way is to use "EventTarget" instead of "any" like MessageEvent.
However then we need to add "toServiceWorker()" method to EventTarget, I don't
feel it is a good idea. So I choose to use "any" and implement the custom
constructor, as 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
template has specific handling for "any" attribute which seems not appropriate
for this "union" type.

[haraken, 2015-05-18 06:40:23]

On 2015/05/18 06:07:53, xiang wrote:
> On 2015/05/18 03:43:00, haraken wrote:
> > On 2015/05/18 03:40:23, falken wrote:
> > > We'll need an Intent to Ship strategy before landing.
> > > 
> > > Code looks reasonable, but I'm worried about all the custom bindings code.
> Is
> > > there a better way? +haraken
> > 
> > That looks unreasonable.
> > 
> > Would you elaborate on why you think each custom binding is needed? We can
> help
> > remove them.
> 
> The reason to use "any" instead of (ServiceWorker or MessagePort) is IDL union
> type doesn't support dictionary access.
> You can check a sample CL: https://codereview.chromium.org/1139873002/.
> 
> An alternative way is to use "EventTarget" instead of "any" like MessageEvent.
> However then we need to add "toServiceWorker()" method to EventTarget, I don't
> feel it is a good idea. So I choose to use "any" and implement the custom
> constructor, as 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> template has specific handling for "any" attribute which seems not appropriate
> for this "union" type.

Thanks!

bashi-san: Would it be possible to support 'any' type in union type?

[haraken, 2015-05-18 06:40:33]

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

[haraken, 2015-05-18 06:40:34]

+bashi

[bashi, 2015-05-18 23:39:11]

The reason the CL https://codereview.chromium.org/1139873002/ don't work is that
we need to use [InitializedByEventConstructor] extended attribute for 'any' type
in EventInit. If we have 'any' type support in IDL dictionary, we should be able
to implement ServiceWorkerMessageEvent with less custom bindings (I guess we
still need to use custom bindings to handle main/isolated world isolation).

I'm working on adding 'any' type support
(https://codereview.chromium.org/1130763006/). If this isn't urgent, I'd like
xiang to wait for my work.

[falken, 2015-05-19 01:41:05]

On 2015/05/18 23:39:11, bashi1 wrote:
> The reason the CL https://codereview.chromium.org/1139873002/ don't work is
that
> we need to use [InitializedByEventConstructor] extended attribute for 'any'
type
> in EventInit. If we have 'any' type support in IDL dictionary, we should be
able
> to implement ServiceWorkerMessageEvent with less custom bindings (I guess we
> still need to use custom bindings to handle main/isolated world isolation).
> 
> I'm working on adding 'any' type support
> (https://codereview.chromium.org/1130763006/). If this isn't urgent, I'd like
> xiang to wait for my work.

If that patch is all that we're waiting for, and would remove the need for
custom bindings code, I think it's fine to wait. Can we expect the patch to land
soonish? We don't plan to merge to M44 so have some time.

[bashi, 2015-05-20 05:22:16]

> If that patch is all that we're waiting for, and would remove the need for
> custom bindings code, I think it's fine to wait. Can we expect the patch
> to land soonish? We don't plan to merge to M44 so have some time.

I landed the CL, could you take another try? Our code generator should have
enough capability to implement ServiceWorkerMessageEvent, but I'm not fully
confident so please let me know if you encounter problems.

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl (right):

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:8:
CustomConstructor(DOMString type, optional ServiceWorkerMessageEventInit
eventInitDict),
On 2015/05/18 03:40:23, falken wrote:
> CustomConstructor is listed as "strongly discouraged"
>
https://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Custo...
> 
> Can you explain why CustomConstructor is needed?

I don't see why you need CustomConstructor. This should be:

Constructor(DOMString type, optional ServiceWorkerMessageEventInit
eventInitDict)

Also, you need to add ServiceWorkerMessageEventInit.idl. It will look like
following:

dictionary ServiceWorkerMessageEventInit : EventInit {
    any data;
    DOMString origin;
    DOMString lastEventId;
    (ServiceWorker or MessagePort)? source;
    sequence<MessagePort> ports;
};

In the constructor (i.e. ServiceWorkerMessageEvent::ServiceWorkerMessageEvent())
you need to initialize its members with the eventInitDict passed by the binding
layer. (See FetchEvent::FetchEvent() for instance)

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:12:
[InitializedByEventConstructor] readonly attribute DOMString origin;
Please drop [InitializedByEventConstructor]. It is no longer needed.

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:14:
[InitializedByEventConstructor, Custom=Getter] readonly attribute any source;
I guess you can use (ServiceWorker or MessagePort)? as the type of |source|.

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:15:
[InitializedByEventConstructor, Custom=Getter] readonly attribute any data;
Please be careful not to leak V8 objects to different isolated worlds.
Description of the CL https://codereview.chromium.org/1130763006/ might be
helpful to understand why and how.

https://codereview.chromium.org/1130113006/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:17: [Custom] void
initServiceWorkerMessageEvent([Default=Undefined] optional DOMString typeArg,
I think we should not implement this. I filed a bug to the spec.
https://github.com/slightlyoff/ServiceWorker/issues/701

[xiang, 2015-05-21 05:50:42]

Thanks bashi, seems most of these custom bindings are not needed anymore.
I have uploaded the unpolished CL.

Currently the "undefined" handling by dictionary_v8.cpp template is not aligned
with test for MessageEvent.
You can find the failures(marked as FIXME) in
https://codereview.chromium.org/1130113006/patch/40001/50002.
I'm not sure whether current behavior is acceptable or not.

[bashi, 2015-05-22 00:23:36]

https://codereview.chromium.org/1130113006/diff/40001/LayoutTests/fast/events...
File
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html
(right):

https://codereview.chromium.org/1130113006/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html:34:
//shouldBe("new ServiceWorkerMessageEvent('eventType', { data: undefined
}).data", "undefined");
Per spec, it should be null.

- In terms of IDL dictionary, "{ data: undefined }" equals to "{}". This means
that no value is specified for "data". No initialization happens for "data".
- The spec says that "When the object is created, this attribute must be
initialized to null".

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:281:
RefPtr<SerializedScriptValue> value =
SerializedScriptValueFactory::instance().createFromWire(message);
Help me understand: Do we really need to use SerializedScriptValue? In other
words, why we can't use ScriptValue?

If SerializedScriptValue is used to prevent leaking v8 values between isolated
worlds, we can rely on ScriptValue::v8ValueFor() instead of using
SerializedScriptValue.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp (right):

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:16: ,
m_origin(initializer.origin())
You can't use initializer.origin() without checking initializer.hasOrigin()
since ServiceWorkerMessageEventInit doesn't have default value for "origin".

Or, you can ask the spec authors to consider providing default values like
CustomEventInit.
https://dom.spec.whatwg.org/#interface-customevent

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:17: ,
m_lastEventId(initializer.lastEventId())
Ditto.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:18: ,
m_source(initializer.source())
Ditto.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:19: ,
m_ports(adoptPtrWillBeNoop(new MessagePortArray(initializer.ports())))
Ditto.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.h (right):

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.h:44:
SerializedScriptValue* serializedData() const { return m_serializedData.get(); }
Could you explain why do you need this?

The reason why CustomEvent has serializedDetail() is that CustomEvent has
initCustomEvent() which takes a SerializedScriptValue. We expose CustomEvent as
WebDOMCustomEvent and blink embedder (i.e. chrome's content layer) uses
initCustomEvent() to initialize it.

ServiceWorkerMessageEvent doesn't provide initXXX() method, and if we can remove
create() which takes SerializedScriptValue (see my comment on
ServiceWorkerContainer.cpp), I think we can remove m_serializedData.

[xiang, 2015-05-25 02:04:25]

Thanks for the review.

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
File
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html
(right):

https://codereview.chromium.org/1130113006/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html:70:
// FIXME: ServiceWorker object could not be created without http server.
On 2015/05/18 03:40:22, falken wrote:
> Why not just test this separately in an http test?

Done. Please check the serviceworker-message-event.html test.

https://codereview.chromium.org/1130113006/diff/40001/LayoutTests/fast/events...
File
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html
(right):

https://codereview.chromium.org/1130113006/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/constructors/serviceworker-message-event-constructor.html:34:
//shouldBe("new ServiceWorkerMessageEvent('eventType', { data: undefined
}).data", "undefined");
On 2015/05/22 00:23:36, bashi1 wrote:
> Per spec, it should be null.
> 
> - In terms of IDL dictionary, "{ data: undefined }" equals to "{}". This means
> that no value is specified for "data". No initialization happens for "data".
> - The spec says that "When the object is created, this attribute must be
> initialized to null".

I see, thanks!

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:281:
RefPtr<SerializedScriptValue> value =
SerializedScriptValueFactory::instance().createFromWire(message);
On 2015/05/22 00:23:36, bashi1 wrote:
> Help me understand: Do we really need to use SerializedScriptValue? In other
> words, why we can't use ScriptValue?
> 
> If SerializedScriptValue is used to prevent leaking v8 values between isolated
> worlds, we can rely on ScriptValue::v8ValueFor() instead of using
> SerializedScriptValue.

However when this method called from content layer v8 context is not entered
yet, I feel it would be better to leave the actual v8 value creation in the
binding code?

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp (right):

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:16: ,
m_origin(initializer.origin())
On 2015/05/22 00:23:36, bashi1 wrote:
> You can't use initializer.origin() without checking initializer.hasOrigin()
> since ServiceWorkerMessageEventInit doesn't have default value for "origin".
> 
> Or, you can ask the spec authors to consider providing default values like
> CustomEventInit.
> https://dom.spec.whatwg.org/#interface-customevent

Done.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:17: ,
m_lastEventId(initializer.lastEventId())
On 2015/05/22 00:23:36, bashi1 wrote:
> Ditto.

Done.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:18: ,
m_source(initializer.source())
On 2015/05/22 00:23:36, bashi1 wrote:
> Ditto.

Done.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:19: ,
m_ports(adoptPtrWillBeNoop(new MessagePortArray(initializer.ports())))
On 2015/05/22 00:23:36, bashi1 wrote:
> Ditto.

Done.

[bashi, 2015-05-26 00:19:01]

IDL dictionary use LGTM.

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/1130113006/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:281:
RefPtr<SerializedScriptValue> value =
SerializedScriptValueFactory::instance().createFromWire(message);
On 2015/05/25 02:04:25, xiang wrote:
> On 2015/05/22 00:23:36, bashi1 wrote:
> > Help me understand: Do we really need to use SerializedScriptValue? In other
> > words, why we can't use ScriptValue?
> > 
> > If SerializedScriptValue is used to prevent leaking v8 values between
isolated
> > worlds, we can rely on ScriptValue::v8ValueFor() instead of using
> > SerializedScriptValue.
> 
> However when this method called from content layer v8 context is not entered
> yet, I feel it would be better to leave the actual v8 value creation in the
> binding code?

Makes sense.

[haraken, 2015-05-26 00:25:52]

https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):

https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7: dictionary
ServiceWorkerMessageEventInit : EventInit {

This is a question against the spec, but why doesn't
ServiceWorkerMessageEventInit inherit from MessageEventInit?

Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent. I'm
curious why ServiceWorkerMessageEvent is not speced so that it inherits from
MessageEvent.

[falken, 2015-05-26 00:50:40]

On 2015/05/26 00:25:52, haraken wrote:
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):
> 
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7: dictionary
> ServiceWorkerMessageEventInit : EventInit {
> 
> This is a question against the spec, but why doesn't
> ServiceWorkerMessageEventInit inherit from MessageEventInit?
> 
> Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent. I'm
> curious why ServiceWorkerMessageEvent is not speced so that it inherits from
> MessageEvent.

Maybe it's not possible because ServiceWorkerMessageEvent.source's type is
different from MessageEvent.source? I asked on
https://github.com/slightlyoff/ServiceWorker/issues/669 anyway.

[xiang, 2015-05-26 01:43:01]

On 2015/05/26 00:50:40, falken wrote:
> On 2015/05/26 00:25:52, haraken wrote:
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl
(right):
> > 
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7:
dictionary
> > ServiceWorkerMessageEventInit : EventInit {
> > 
> > This is a question against the spec, but why doesn't
> > ServiceWorkerMessageEventInit inherit from MessageEventInit?
> > 
> > Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent.
I'm
> > curious why ServiceWorkerMessageEvent is not speced so that it inherits from
> > MessageEvent.
> 
> Maybe it's not possible because ServiceWorkerMessageEvent.source's type is
> different from MessageEvent.source? I asked on
> https://github.com/slightlyoff/ServiceWorker/issues/669 anyway.

There's another link on extending MessageEvent
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199.

[haraken, 2015-05-26 02:00:31]

On 2015/05/26 01:43:01, xiang wrote:
> On 2015/05/26 00:50:40, falken wrote:
> > On 2015/05/26 00:25:52, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > > File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > > Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7:
> dictionary
> > > ServiceWorkerMessageEventInit : EventInit {
> > > 
> > > This is a question against the spec, but why doesn't
> > > ServiceWorkerMessageEventInit inherit from MessageEventInit?
> > > 
> > > Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent.
> I'm
> > > curious why ServiceWorkerMessageEvent is not speced so that it inherits
from
> > > MessageEvent.
> > 
> > Maybe it's not possible because ServiceWorkerMessageEvent.source's type is
> > different from MessageEvent.source? I asked on
> > https://github.com/slightlyoff/ServiceWorker/issues/669 anyway.
> 
> There's another link on extending MessageEvent
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199.

Are you planning to fix the spec before landing the CL? Or are you planning to
land the CL and fix the spec later? (This is a bit unfortunate but I'm ok with
it.)

[falken, 2015-05-27 05:09:18]

On 2015/05/26 02:00:31, haraken wrote:
> On 2015/05/26 01:43:01, xiang wrote:
> > On 2015/05/26 00:50:40, falken wrote:
> > > On 2015/05/26 00:25:52, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > > > File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > > > Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7:
> > dictionary
> > > > ServiceWorkerMessageEventInit : EventInit {
> > > > 
> > > > This is a question against the spec, but why doesn't
> > > > ServiceWorkerMessageEventInit inherit from MessageEventInit?
> > > > 
> > > > Also ServiceWorkerMessageEvent duplicates a lot of code from
MessageEvent.
> > I'm
> > > > curious why ServiceWorkerMessageEvent is not speced so that it inherits
> from
> > > > MessageEvent.
> > > 
> > > Maybe it's not possible because ServiceWorkerMessageEvent.source's type is
> > > different from MessageEvent.source? I asked on
> > > https://github.com/slightlyoff/ServiceWorker/issues/669 anyway.
> > 
> > There's another link on extending MessageEvent
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199.
> 
> Are you planning to fix the spec before landing the CL? Or are you planning to
> land the CL and fix the spec later? (This is a bit unfortunate but I'm ok with
> it.)

We should do an Intent to Ship for this. It'd be smoother to wait for spec issue
to settle.

[jungkees, 2015-05-27 10:51:33]

jungkee.song@samsung.com changed reviewers:
+ jungkee.song@samsung.com

[jungkees, 2015-05-27 10:51:33]

https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):

https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7: dictionary
ServiceWorkerMessageEventInit : EventInit {
On 2015/05/26 00:25:52, haraken wrote:
> 
> This is a question against the spec, but why doesn't
> ServiceWorkerMessageEventInit inherit from MessageEventInit?
> 
> Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent. I'm
> curious why ServiceWorkerMessageEvent is not speced so that it inherits from
> MessageEvent.

falken brought me here.

Subclassing MessageEvent interface was the way we tried first, but it ends up
wonfix:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199
One of the reasons is the type of source attribute is radically different from
that of the MessageEvent, and extending HTML text has not happened.

Another reason is ExtendableMessageEvent (a friend of ServiceWorkerMessageEvent)
had to extend both ExtendableEvent (for its waitUntil() method in SW context)
and MessageEvent (for its source attribute as mentioned). We couldn't do it with
subclassing MessageEvent and redefine waitUntil() in the subclass apart from the
ExtendableEvent interface. Please see some of the rationale discussed in the
spec issue here:
https://github.com/slightlyoff/ServiceWorker/issues/669#issuecomment-88860069.

[xiang, 2015-05-28 06:21:25]

On 2015/05/27 10:51:33, jungkees wrote:
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):
> 
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7: dictionary
> ServiceWorkerMessageEventInit : EventInit {
> On 2015/05/26 00:25:52, haraken wrote:
> > 
> > This is a question against the spec, but why doesn't
> > ServiceWorkerMessageEventInit inherit from MessageEventInit?
> > 
> > Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent.
I'm
> > curious why ServiceWorkerMessageEvent is not speced so that it inherits from
> > MessageEvent.
> 
> falken brought me here.
> 
> Subclassing MessageEvent interface was the way we tried first, but it ends up
> wonfix:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199
> One of the reasons is the type of source attribute is radically different from
> that of the MessageEvent, and extending HTML text has not happened.
> 
> Another reason is ExtendableMessageEvent (a friend of
ServiceWorkerMessageEvent)
> had to extend both ExtendableEvent (for its waitUntil() method in SW context)
> and MessageEvent (for its source attribute as mentioned). We couldn't do it
with
> subclassing MessageEvent and redefine waitUntil() in the subclass apart from
the
> ExtendableEvent interface. Please see some of the rationale discussed in the
> spec issue here:
> https://github.com/slightlyoff/ServiceWorker/issues/669#issuecomment-88860069.

Seems the spec decision has been made.
falken@ is it OK to continue the implementation? Or will it better to wait for
the intent to ship pass?

[falken, 2015-05-28 06:26:46]

On 2015/05/28 06:21:25, xiang wrote:
> On 2015/05/27 10:51:33, jungkees wrote:
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl
(right):
> > 
> >
>
https://codereview.chromium.org/1130113006/diff/60001/Source/modules/servicew...
> > Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:7:
dictionary
> > ServiceWorkerMessageEventInit : EventInit {
> > On 2015/05/26 00:25:52, haraken wrote:
> > > 
> > > This is a question against the spec, but why doesn't
> > > ServiceWorkerMessageEventInit inherit from MessageEventInit?
> > > 
> > > Also ServiceWorkerMessageEvent duplicates a lot of code from MessageEvent.
> I'm
> > > curious why ServiceWorkerMessageEvent is not speced so that it inherits
from
> > > MessageEvent.
> > 
> > falken brought me here.
> > 
> > Subclassing MessageEvent interface was the way we tried first, but it ends
up
> > wonfix:
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28199
> > One of the reasons is the type of source attribute is radically different
from
> > that of the MessageEvent, and extending HTML text has not happened.
> > 
> > Another reason is ExtendableMessageEvent (a friend of
> ServiceWorkerMessageEvent)
> > had to extend both ExtendableEvent (for its waitUntil() method in SW
context)
> > and MessageEvent (for its source attribute as mentioned). We couldn't do it
> with
> > subclassing MessageEvent and redefine waitUntil() in the subclass apart from
> the
> > ExtendableEvent interface. Please see some of the rationale discussed in the
> > spec issue here:
> >
https://github.com/slightlyoff/ServiceWorker/issues/669#issuecomment-88860069.
> 
> Seems the spec decision has been made.
> falken@ is it OK to continue the implementation? Or will it better to wait for
> the intent to ship pass?

We were a little sloppy in the past, but I think we should either do an intent
to ship or land behind a feature flag for non-trivial changes like this one. I'd
prefer just an intent to ship so we don't have to muck with flags. Would you be
willing to write and send an intent to ship to blink-dev, or would you rather I
take care of that?

It'd be nice to get confirmation from Mozilla that they are OK with the spec
since it's settled so recently.

[xiang, 2015-06-15 02:32:03]

> 
> We were a little sloppy in the past, but I think we should either do an intent
> to ship or land behind a feature flag for non-trivial changes like this one.
I'd
> prefer just an intent to ship so we don't have to muck with flags. Would you
be
> willing to write and send an intent to ship to blink-dev, or would you rather
I
> take care of that?
> 
> It'd be nice to get confirmation from Mozilla that they are OK with the spec
> since it's settled so recently.

We have sent the
intent(https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64)
and seems Mozilla is OK with this spec change. I'd like to continue update this
CL.

I just rebased the CL and made the union type on stack only. PTAL, thanks.

[falken, 2015-06-18 11:45:09]

On 2015/06/15 02:32:03, xiang wrote:
> > 
> > We were a little sloppy in the past, but I think we should either do an
intent
> > to ship or land behind a feature flag for non-trivial changes like this one.
> I'd
> > prefer just an intent to ship so we don't have to muck with flags. Would you
> be
> > willing to write and send an intent to ship to blink-dev, or would you
rather
> I
> > take care of that?
> > 
> > It'd be nice to get confirmation from Mozilla that they are OK with the spec
> > since it's settled so recently.
> 
> We have sent the
>
intent(https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64)
> and seems Mozilla is OK with this spec change. I'd like to continue update
this
> CL.
> 
> I just rebased the CL and made the union type on stack only. PTAL, thanks.

I'm not ignoring this CL, but the intent to ship is still unresolved. Tried to
ping the thread, apparently the leads are busy now. Will escalate pinging if not
done next week.

[falken, 2015-06-25 03:49:35]

ServiceWorker LGTM.

https://codereview.chromium.org/1130113006/diff/80001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworker-message-event.html
(right):

https://codereview.chromium.org/1130113006/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworker-message-event.html:23: var e
= new ServiceWorkerMessageEvent('evenType', {source: worker});
eventType

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:15: [Custom=Getter]
readonly attribute any data;
Can you put these in the same order as the spec, for easier comparison?

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:16: readonly
attribute MessagePort[] ports;
The spec is "MessagePort[]?"

[haraken, 2015-06-25 04:18:49]

bindings and oilpan LGTM

https://codereview.chromium.org/1130113006/diff/80001/Source/bindings/modules...
File Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1130113006/diff/80001/Source/bindings/modules...
Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp:13:
static v8::Local<v8::Value> cacheState(v8::Isolate* isolate,
v8::Local<v8::Object> event, v8::Local<v8::Value> data)

Can we remove this helper function?

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:24: if
(initializer.source().isMessagePort())

else if

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.h (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.h:53: ScriptValue
m_data;

This will cause a memory leak in some artificial scenario, but it's a known
issue (we have the same issue in MessageEvent etc). The binding team will
address it soon.

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:16: readonly
attribute MessagePort[] ports;
On 2015/06/25 03:49:35, falken wrote:
> The spec is "MessagePort[]?"

Can we add a test for this?

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:12:
sequence<MessagePort> ports;

Is it intentional that you use sequence<MessagePort> in
ServiceWorkerMessageEventInit but use MessagePort[] in
ServiceWorkerMessageEvent?

[xiang, 2015-06-29 02:32:24]

Thanks for the review, CL updated.

https://codereview.chromium.org/1130113006/diff/80001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworker-message-event.html
(right):

https://codereview.chromium.org/1130113006/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworker-message-event.html:23: var e
= new ServiceWorkerMessageEvent('evenType', {source: worker});
On 2015/06/25 03:49:35, falken wrote:
> eventType

Done.

https://codereview.chromium.org/1130113006/diff/80001/Source/bindings/modules...
File Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1130113006/diff/80001/Source/bindings/modules...
Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp:13:
static v8::Local<v8::Value> cacheState(v8::Isolate* isolate,
v8::Local<v8::Object> event, v8::Local<v8::Value> data)
On 2015/06/25 04:18:48, haraken wrote:
> 
> Can we remove this helper function?

Done.

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.cpp:24: if
(initializer.source().isMessagePort())
On 2015/06/25 04:18:48, haraken wrote:
> 
> else if

Done.

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:15: [Custom=Getter]
readonly attribute any data;
On 2015/06/25 03:49:35, falken wrote:
> Can you put these in the same order as the spec, for easier comparison?

Done.

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:16: readonly
attribute MessagePort[] ports;
On 2015/06/25 03:49:35, falken wrote:
> The spec is "MessagePort[]?"

Done.

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:16: readonly
attribute MessagePort[] ports;
On 2015/06/25 04:18:49, haraken wrote:
> On 2015/06/25 03:49:35, falken wrote:
> > The spec is "MessagePort[]?"
> 
> Can we add a test for this?

The fast/events/constructors/serviceworker-message-event-constructor.html will
verfiy this change.

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:12:
sequence<MessagePort> ports;
On 2015/06/25 04:18:49, haraken wrote:
> 
> Is it intentional that you use sequence<MessagePort> in
> ServiceWorkerMessageEventInit but use MessagePort[] in
> ServiceWorkerMessageEvent?

Yes, this's defined by spec to use "sequence" in ServiceWorkerMessageEventInit.

[haraken, 2015-06-29 02:37:14]

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):

https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:12:
sequence<MessagePort> ports;
On 2015/06/29 02:32:24, xiang wrote:
> On 2015/06/25 04:18:49, haraken wrote:
> > 
> > Is it intentional that you use sequence<MessagePort> in
> > ServiceWorkerMessageEventInit but use MessagePort[] in
> > ServiceWorkerMessageEvent?
> 
> Yes, this's defined by spec to use "sequence" in
ServiceWorkerMessageEventInit.

+bashi@: Is it common to use a sequence in an EventInit but use an array in an
Event?

[xiang, 2015-07-01 04:38:48]

xiang.long@intel.com changed reviewers:
+ mkwst@chromium.org

[xiang, 2015-07-01 04:41:53]

On 2015/06/29 02:37:14, haraken wrote:
>
https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
> File Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl (right):
> 
>
https://codereview.chromium.org/1130113006/diff/80001/Source/modules/servicew...
> Source/modules/serviceworkers/ServiceWorkerMessageEventInit.idl:12:
> sequence<MessagePort> ports;
> On 2015/06/29 02:32:24, xiang wrote:
> > On 2015/06/25 04:18:49, haraken wrote:
> > > 
> > > Is it intentional that you use sequence<MessagePort> in
> > > ServiceWorkerMessageEventInit but use MessagePort[] in
> > > ServiceWorkerMessageEvent?
> > 
> > Yes, this's defined by spec to use "sequence" in
> ServiceWorkerMessageEventInit.
> 
> +bashi@: Is it common to use a sequence in an EventInit but use an array in an
> Event?

MessageEvent(https://html.spec.whatwg.org/multipage/comms.html#messageevent) has
a similar pattern.
The usage seems reasonable per
spec(http://heycam.github.io/webidl/#idl-sequence), and "Sequences must not be
used as the type of an attribute or constant".

mkwst@, would you please review the "public/platform/" change again? Thanks!

[Mike West, 2015-07-01 08:14:13]

public/platform LGTM.

The layout test failures look real, though. You'll need to rebaseline, I
believe.

[Mike West, 2015-07-01 08:14:13]

public/platform LGTM.

The layout test failures look real, though. You'll need to rebaseline, I
believe.

[xiang, 2015-07-06 02:40:14]

The CQ bit was checked by xiang.long@intel.com

[xiang, 2015-07-06 02:40:15]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org,
falken@chromium.org, haraken@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1130113006/#ps140001
(title: "make more tests pass")

[commit-bot: I haz the power, 2015-07-06 02:40:25]

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

[commit-bot: I haz the power, 2015-07-06 04:26:34]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198297