ServiceWorker: Implement attributes of ExtendableMessageEvent

ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent in LayoutTests.
(Before this CL, regular MessageEvent was used in LayoutTests)

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#extendablemessage-event-section
Intent-to-ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
Committed: https://crrev.com/cde920bddbea22c192e1caa2c7c7ae0c821242c7
Cr-Commit-Position: refs/heads/master@{#375129}

[nhiroki, 2016-02-02 06:38:01]

Description was changed from

==========
(WIP) ServiceWorker: Implement attributes of ExtendableMessageEvent

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source' and enables ExtendableMessageEvent on LayoutTests.

BUG=543198
==========

[nhiroki, 2016-02-02 06:53:16]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source' and enables ExtendableMessageEvent on LayoutTests.

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source' and enables ExtendableMessageEvent on LayoutTests.

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

[nhiroki, 2016-02-02 06:53:45]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source' and enables ExtendableMessageEvent on LayoutTests.

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

[nhiroki, 2016-02-02 06:54:50]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

Depends on https://codereview.chromium.org/1632113004/

BUG=543198
==========

[nhiroki, 2016-02-02 06:55:26]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

Depends on https://codereview.chromium.org/1632113004/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

Depends on https://codereview.chromium.org/1632113004/

BUG=543198
==========

[nhiroki, 2016-02-02 06:59:48]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-02-02 06:59:57]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2016-02-02 07:00:06]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2016-02-02 07:00:12]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2016-02-02 07:00:21]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2016-02-02 07:00:27]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2016-02-02 07:08:27]

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

[nhiroki, 2016-02-02 07:08:28]

falken@, can you review this?

FYI:
- I'll implement 'origin' and 'source' attributes in a separate CL because they
need more work around IPC.
- Some layout tests have been running async operations w/o waitUntil on the
(Extendable)Message event. I'll make them appropriately wait for the async
operations in a separate CL.

Thank you!

[nhiroki, 2016-02-03 06:53:54]

On 2016/02/02 07:08:28, nhiroki wrote:
> - Some layout tests have been running async operations w/o waitUntil on the
> (Extendable)Message event. I'll make them appropriately wait for the async
> operations in a separate CL.

FYI: Uploaded a CL for this: https://codereview.chromium.org/1668473002/

[nhiroki, 2016-02-05 02:23:43]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

Depends on https://codereview.chromium.org/1632113004/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

[falken, 2016-02-05 02:28:52]

looks good. Should we add LayoutTests like the ones in
fast/events/constructors/message-event-constructor?

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp
(right):

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:43:
}
Ah, I see the ports() code came from MessageEvent.cpp. Maybe add a TODO in
ports(bool&) pointing to the one in MessageEvent.cpp?

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:52:
result =
ClientOrServiceWorkerOrMessagePort::fromMessagePort(m_sourceAsMessagePort);
else NOTREACHED?

[nhiroki, 2016-02-05 09:15:23]

On 2016/02/05 02:28:52, falken wrote:
> looks good. Should we add LayoutTests like the ones in
> fast/events/constructors/message-event-constructor?

Yes, we should. I'll make a separate CL for that.

[nhiroki, 2016-02-05 09:15:38]

Thank you!

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp
(right):

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:43:
}
On 2016/02/05 02:28:52, falken wrote:
> Ah, I see the ports() code came from MessageEvent.cpp. Maybe add a TODO in
> ports(bool&) pointing to the one in MessageEvent.cpp?

Done (Just copied the comment from MessageEvent.cpp. Is this ok?).

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:52:
result =
ClientOrServiceWorkerOrMessagePort::fromMessagePort(m_sourceAsMessagePort);
On 2016/02/05 02:28:52, falken wrote:
> else NOTREACHED?

I think the case actually happens when the event is manually created by the
ctor.

[falken, 2016-02-05 09:58:18]

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

[falken, 2016-02-05 09:58:18]

The SW part lgtm but I'd like bashi@ to double-check the union type use, since
I'm not very familiar with that.

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp
(right):

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:43:
}
On 2016/02/05 09:15:37, nhiroki wrote:
> On 2016/02/05 02:28:52, falken wrote:
> > Ah, I see the ports() code came from MessageEvent.cpp. Maybe add a TODO in
> > ports(bool&) pointing to the one in MessageEvent.cpp?
> 
> Done (Just copied the comment from MessageEvent.cpp. Is this ok?).

OK with me. I also noticed the snippet is in ServiceWorkerMessageEvent.cpp,
maybe good to copy there too.

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:52:
result =
ClientOrServiceWorkerOrMessagePort::fromMessagePort(m_sourceAsMessagePort);
On 2016/02/05 09:15:37, nhiroki wrote:
> On 2016/02/05 02:28:52, falken wrote:
> > else NOTREACHED?
> 
> I think the case actually happens when the event is manually created by the
> ctor.

Hm, I'm not sure how the bindings works but I'm worried about C++ that doesn't
set the result arg. What happens if the user says x = event.source()? I guess
the expected behavior is undefined or null?

https://codereview.chromium.org/1658073002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp
(right):

https://codereview.chromium.org/1658073002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:39:
isNull = false;
oh, I think this should be isNull = true

[nhiroki, 2016-02-09 10:20:03]

Thank you for reviewing!

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp
(right):

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:43:
}
On 2016/02/05 09:58:18, falken wrote:
> On 2016/02/05 09:15:37, nhiroki wrote:
> > On 2016/02/05 02:28:52, falken wrote:
> > > Ah, I see the ports() code came from MessageEvent.cpp. Maybe add a TODO in
> > > ports(bool&) pointing to the one in MessageEvent.cpp?
> > 
> > Done (Just copied the comment from MessageEvent.cpp. Is this ok?).
> 
> OK with me. I also noticed the snippet is in ServiceWorkerMessageEvent.cpp,
> maybe good to copy there too.

Done.

https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:52:
result =
ClientOrServiceWorkerOrMessagePort::fromMessagePort(m_sourceAsMessagePort);
On 2016/02/05 09:58:18, falken wrote:
> On 2016/02/05 09:15:37, nhiroki wrote:
> > On 2016/02/05 02:28:52, falken wrote:
> > > else NOTREACHED?
> > 
> > I think the case actually happens when the event is manually created by the
> > ctor.
> 
> Hm, I'm not sure how the bindings works but I'm worried about C++ that doesn't
> set the result arg. What happens if the user says x = event.source()? I guess
> the expected behavior is undefined or null?

According to the spec, null is the expected value:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...

It looks like the bindings layer creates |source| as a default union type that
represents 'null' in JS world [1], so technically we don't have to take care of
the case here. However, explicitly setting the null value would be more
understandable, so I added the "else" clause to handle the case. Let's wait for
bashi-san's comment :)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

https://codereview.chromium.org/1658073002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp
(right):

https://codereview.chromium.org/1658073002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:39:
isNull = false;
On 2016/02/05 09:58:18, falken wrote:
> oh, I think this should be isNull = true

Good catch! Fixed.

[nhiroki, 2016-02-09 10:25:26]

bashi-san, can you check the the union type use in
"ExtendableMessageEvent::source"? The context is as follows:

On 2016/02/09 10:20:03, nhiroki (slow) wrote:
>
https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:52:
> result =
> ClientOrServiceWorkerOrMessagePort::fromMessagePort(m_sourceAsMessagePort);
> On 2016/02/05 09:58:18, falken wrote:
> > On 2016/02/05 09:15:37, nhiroki wrote:
> > > On 2016/02/05 02:28:52, falken wrote:
> > > > else NOTREACHED?
> > > 
> > > I think the case actually happens when the event is manually created by
the
> > > ctor.
> > 
> > Hm, I'm not sure how the bindings works but I'm worried about C++ that
doesn't
> > set the result arg. What happens if the user says x = event.source()? I
guess
> > the expected behavior is undefined or null?
> 
> According to the spec, null is the expected value:
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
> 
> It looks like the bindings layer creates |source| as a default union type that
> represents 'null' in JS world [1], so technically we don't have to take care
of
> the case here. However, explicitly setting the null value would be more
> understandable, so I added the "else" clause to handle the case. Let's wait
for
> bashi-san's comment :)
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

Thank you!

[nhiroki, 2016-02-09 10:26:55]

On 2016/02/09 10:25:26, nhiroki (slow) wrote:
> bashi-san, can you check the the union type use in
> "ExtendableMessageEvent::source"? The context is as follows:
> 
> On 2016/02/09 10:20:03, nhiroki (slow) wrote:
> >
>
https://codereview.chromium.org/1658073002/diff/140001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.cpp:52:
> > result =
> > ClientOrServiceWorkerOrMessagePort::fromMessagePort(m_sourceAsMessagePort);
> > On 2016/02/05 09:58:18, falken wrote:
> > > On 2016/02/05 09:15:37, nhiroki wrote:
> > > > On 2016/02/05 02:28:52, falken wrote:
> > > > > else NOTREACHED?
> > > > 
> > > > I think the case actually happens when the event is manually created by
> the
> > > > ctor.
> > > 
> > > Hm, I'm not sure how the bindings works but I'm worried about C++ that
> doesn't
> > > set the result arg. What happens if the user says x = event.source()? I
> guess
> > > the expected behavior is undefined or null?
> > 
> > According to the spec, null is the expected value:
> >
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
> > 
> > It looks like the bindings layer creates |source| as a default union type
that
> > represents 'null' in JS world [1], so technically we don't have to take care
> of
> > the case here. However, explicitly setting the null value would be more
> > understandable, so I added the "else" clause to handle the case. Let's wait
> for
> > bashi-san's comment :)
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> 
> Thank you!

... and please review third_party/WebKit/Source/bindings/* as an OWNER

[bashi, 2016-02-10 00:38:55]

I missed this... Sorry for late review.

As wrote in comments, this could cause memory leak (not your fault though).
ServiceWorkerMessageEvent has the same problem so I'd suggest fixing
ServiceWorkerMessageEvent first then apply the fix to this CL.

I'll prepare a fix for ServiceWorkerMessageEvent.

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
(left):

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
ServiceWorkerMessageEvent* event =
V8ServiceWorkerMessageEvent::toImpl(info.Holder());
It seems that ServiceWorkerMessageEvent also causes memory leak...

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
ExtendableMessageEvent* event = V8ExtendableMessageEvent::toImpl(info.Holder());
Please refer V8CustomEvent::detailAttributeGetterCustom() to see how to
implement a getter for "any" types. We should consider where the |data| comes
from and shouldn't expose it to different worlds.

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.h
(right):

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.h:42:
ScriptValue m_data;
We cannot hold a ScriptValue in a DOM impl class. It adds a strong reference
from Blink to V8, resulting a memory leak. See
https://code.google.com/p/chromium/issues/detail?id=501866 and
https://code.google.com/p/chromium/issues/detail?id=529941 for details.

A quick fix would be a custom constructor which stores |data| as a hidden value
of the wrapper object like https://codereview.chromium.org/1383543004/

I do feel that this is a design bug in the binding layer (sorry about that).
haraken-san was trying to solve this by trace wrapper proposal[1] and the V8
team took over the proposal. I guess that they will start working on it once
Oilpan is stabilized.

[1]
https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP...

[nhiroki, 2016-02-10 03:34:01]

On 2016/02/10 00:38:55, bashi1 wrote:
> I missed this... Sorry for late review.
> 
> As wrote in comments, this could cause memory leak (not your fault though).
> ServiceWorkerMessageEvent has the same problem so I'd suggest fixing
> ServiceWorkerMessageEvent first then apply the fix to this CL.
> 
> I'll prepare a fix for ServiceWorkerMessageEvent.

Thank you for your comments! I got the situation. I'll wait for your fix and
remake this patch based on that.

[nhiroki, 2016-02-10 07:40:37]

(Not updated the CL yet)

On 2016/02/10 03:34:01, nhiroki (slow) wrote:
> On 2016/02/10 00:38:55, bashi1 wrote:
> > I missed this... Sorry for late review.
> > 
> > As wrote in comments, this could cause memory leak (not your fault though).
> > ServiceWorkerMessageEvent has the same problem so I'd suggest fixing
> > ServiceWorkerMessageEvent first then apply the fix to this CL.
> > 
> > I'll prepare a fix for ServiceWorkerMessageEvent.
> 
> Thank you for your comments! I got the situation. I'll wait for your fix and
> remake this patch based on that.

bashi-san's patch was landed, so I'll resume this CL...
https://codereview.chromium.org/1688573002/

[nhiroki, 2016-02-10 08:08:57]

bashi@, can you take another look? I recreated the CL based on your CL. Thanks!

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
ExtendableMessageEvent* event = V8ExtendableMessageEvent::toImpl(info.Holder());
On 2016/02/10 00:38:55, bashi1 wrote:
> Please refer V8CustomEvent::detailAttributeGetterCustom() to see how to
> implement a getter for "any" types. We should consider where the |data| comes
> from and shouldn't expose it to different worlds.

Done.

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.h
(right):

https://codereview.chromium.org/1658073002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ExtendableMessageEvent.h:42:
ScriptValue m_data;
On 2016/02/10 00:38:55, bashi1 wrote:
> We cannot hold a ScriptValue in a DOM impl class. It adds a strong reference
> from Blink to V8, resulting a memory leak. See
> https://code.google.com/p/chromium/issues/detail?id=501866 and
> https://code.google.com/p/chromium/issues/detail?id=529941 for details.
> 
> A quick fix would be a custom constructor which stores |data| as a hidden
value
> of the wrapper object like https://codereview.chromium.org/1383543004/
> 
> I do feel that this is a design bug in the binding layer (sorry about that).
> haraken-san was trying to solve this by trace wrapper proposal[1] and the V8
> team took over the proposal. I guess that they will start working on it once
> Oilpan is stabilized.
> 
> [1]
>
https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP...

Done.

[bashi, 2016-02-10 08:23:03]

https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
void V8ExtendableMessageEvent::constructorCustom(const
v8::FunctionCallbackInfo<v8::Value>& info)
Is it possible to share the implementation with V8ServiceWorkerMessageEvent by
using templates? It looks almost the same except types. I guess we can put the
template header file in Source/bindings/modules/v8.

[nhiroki, 2016-02-10 09:48:41]

https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
void V8ExtendableMessageEvent::constructorCustom(const
v8::FunctionCallbackInfo<v8::Value>& info)
On 2016/02/10 08:23:03, bashi1 wrote:
> Is it possible to share the implementation with V8ServiceWorkerMessageEvent by
> using templates? It looks almost the same except types. I guess we can put the
> template header file in Source/bindings/modules/v8.

Thank you for your suggestion. I'm now trying to templatize these functions.

I have a question. How can I get the typename "V8ExtendableMessageEventInit"
from the typename "ExtendableMessageEventInit" programmatically? I found
V8TypeOf<ExtendableMessageEvent>::Type, but I couldn't find a similar helper for
ExtendableMessageEventInit...

[bashi, 2016-02-10 10:21:08]

https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
void V8ExtendableMessageEvent::constructorCustom(const
v8::FunctionCallbackInfo<v8::Value>& info)
On 2016/02/10 09:48:41, nhiroki (slow) wrote:
> On 2016/02/10 08:23:03, bashi1 wrote:
> > Is it possible to share the implementation with V8ServiceWorkerMessageEvent
by
> > using templates? It looks almost the same except types. I guess we can put
the
> > template header file in Source/bindings/modules/v8.
> 
> Thank you for your suggestion. I'm now trying to templatize these functions.
> 
> I have a question. How can I get the typename "V8ExtendableMessageEventInit"
> from the typename "ExtendableMessageEventInit" programmatically? I found
> V8TypeOf<ExtendableMessageEvent>::Type, but I couldn't find a similar helper
for
> ExtendableMessageEventInit...

Oops, I'll add it. Sorry for inconvenience.

[nhiroki, 2016-02-11 00:39:32]

On 2016/02/10 10:21:08, bashi1 wrote:
>
https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp
> (right):
> 
>
https://codereview.chromium.org/1658073002/diff/260001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/modules/v8/custom/V8ExtendableMessageEventCustom.cpp:14:
> void V8ExtendableMessageEvent::constructorCustom(const
> v8::FunctionCallbackInfo<v8::Value>& info)
> On 2016/02/10 09:48:41, nhiroki (slow) wrote:
> > On 2016/02/10 08:23:03, bashi1 wrote:
> > > Is it possible to share the implementation with
V8ServiceWorkerMessageEvent
> by
> > > using templates? It looks almost the same except types. I guess we can put
> the
> > > template header file in Source/bindings/modules/v8.
> > 
> > Thank you for your suggestion. I'm now trying to templatize these functions.
> > 
> > I have a question. How can I get the typename "V8ExtendableMessageEventInit"
> > from the typename "ExtendableMessageEventInit" programmatically? I found
> > V8TypeOf<ExtendableMessageEvent>::Type, but I couldn't find a similar helper
> for
> > ExtendableMessageEventInit...
> 
> Oops, I'll add it. Sorry for inconvenience.

Wow, your CL was already landed. Thank you! :)
https://codereview.chromium.org/1686033002/

[nhiroki, 2016-02-12 04:10:48]

Templatized duplicate binding code. Bashi-san, can you take another look?
Thanks!

[bashi, 2016-02-12 04:43:57]

bindings/ LGTM. Thank you for addressing comments!

[nhiroki, 2016-02-12 04:51:33]

nhiroki@chromium.org changed reviewers:
+ tkent@chromium.org

[nhiroki, 2016-02-12 04:51:34]

Kent-san, can you review Source/web/* and public/web/*? Thanks!

[nhiroki, 2016-02-12 04:55:21]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.
(Before this CL, regular MessageEvent is used on LayoutTests instead)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

[nhiroki, 2016-02-12 04:55:58]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent on LayoutTests.
(Before this CL, regular MessageEvent is used on LayoutTests instead)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent in LayoutTests.
(Before this CL, regular MessageEvent was used in LayoutTests)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

[tkent, 2016-02-12 05:06:22]

On 2016/02/12 at 04:51:34, nhiroki wrote:
> Kent-san, can you review Source/web/* and public/web/*? Thanks!

lgtm

[nhiroki, 2016-02-12 05:17:01]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-02-12 05:17:02]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1658073002/#ps280001
(title: "templatize duplicate code")

[commit-bot: I haz the power, 2016-02-12 05:17:35]

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

[commit-bot: I haz the power, 2016-02-12 05:36:58]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent in LayoutTests.
(Before this CL, regular MessageEvent was used in LayoutTests)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent in LayoutTests.
(Before this CL, regular MessageEvent was used in LayoutTests)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

[commit-bot: I haz the power, 2016-02-12 05:36:59]

Committed patchset #9 (id:280001)

[commit-bot: I haz the power, 2016-02-16 22:41:49]

Description was changed from

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent in LayoutTests.
(Before this CL, regular MessageEvent was used in LayoutTests)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198
==========

to

==========
ServiceWorker: Implement attributes of ExtendableMessageEvent

This CL implements attributes of ExtendableMessageEvent other than 'origin'
and 'source', and enables ExtendableMessageEvent in LayoutTests.
(Before this CL, regular MessageEvent was used in LayoutTests)

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

The most part of this CL was originally implemented by xiang.long@intel.com:
http://crrev.com/1156703003/

BUG=543198

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

[commit-bot: I haz the power, 2016-02-16 22:41:50]

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