Set MessageEvent.source to the newly created port for shared workers' connect events

Set MessageEvent.source to the newly created port for shared workers' connect events

Set MessageEvent.source to the newly created port for shared workers' connect events
instead of previously null, as per the latest specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#dom-messageevent-source
http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#dom-sharedworker

BUG=275289
R=abarth@chromium.org

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

[abarth-chromium, 2013-08-19 19:24:57]

You've added a bunch of comments, but I don't think they're accurate.  The
MessageEvent constructor can be used to put an arbitrary EventTarget in that
slot.

Please add tests that cover this case and make sure our behavior is correct.

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.cpp
File Source/core/dom/MessageEvent.cpp (right):

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.cpp:66: }
For example, the ASSERT could go here (and in other constructors).

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.h
File Source/core/dom/MessageEvent.h (right):

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.h:132: RefPtr<EventTarget> m_source; // A Window or
a MessagePort.
Can we ASSERT this fact somewhere rather than having a comment?

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.idl
File Source/core/dom/MessageEvent.idl (right):

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.idl:34: [InitializedByEventConstructor] readonly
attribute EventTarget source; // May be a Window or a MessagePort
Is that enforced by the constructor?

I would remove this comment.

[do-not-use, 2013-08-19 19:45:57]

Note that I haven't added a test that tries constructing a MessageEvent in
JavaScript by passing a MessagePort as source because this is not supported by
the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#messag...

In the specification, MessageEventInit.source can only be a DOMWindow. This may
be a bug so I filed:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23011

Therefore, I haven't changed the WebExposed behavior yet when it comes to the
MessageEvent construction. Right now, it is only possible to construct a
MessageEvent with a MessagePort as source on the native side.

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.h
File Source/core/dom/MessageEvent.h (right):

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.h:56: static PassRefPtr<MessageEvent>
create(PassOwnPtr<MessagePortArray> ports, PassRefPtr<MessagePort> source = 0)
This factory takes a MessagePort, the others take a DOMWindow. The constructor
is private. Therefore, we make sure that m_source is either a DOMWindow or a
MessagePort.

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.h:132: RefPtr<EventTarget> m_source; // A Window or
a MessagePort.
On 2013/08/19 19:24:58, abarth wrote:
> Can we ASSERT this fact somewhere rather than having a comment?

Yes, I can add an assertion but this would mean adding a toMessagePort() virtual
function member to EventTarget (similarly to toDOMWindow()).

I actually did that at first and then decided to enforce that MessageEvent is
constructed using a DOMWindow / MessagePort using the factory functions as it
seemed a bit less intrusive. I am however happy to switch to a an assertion if
you prefer.

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.idl
File Source/core/dom/MessageEvent.idl (right):

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.idl:34: [InitializedByEventConstructor] readonly
attribute EventTarget source; // May be a Window or a MessagePort
On 2013/08/19 19:24:58, abarth wrote:
> Is that enforced by the constructor?
> 
> I would remove this comment.

The specification is using (Window or MessagePort) type, which our bindings
generator does not support. Instead, I decided to use a common parent type
(EventTarget) and add a comment. I do think that the comment helps understanding
what source is.

[abarth-chromium, 2013-08-19 20:01:50]

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.h
File Source/core/dom/MessageEvent.h (right):

https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
Source/core/dom/MessageEvent.h:80: static PassRefPtr<MessageEvent> create(const
AtomicString& type, const MessageEventInit& initializer)
This constructor is the problematic one.  With your CL, I believe you can now
(incorrectly) construct a MessageEvent with a Node (or any other EventTarget)
using a DOM4-style constructor:

var evt = new MessageEvent({
  source: document.body,
  // etc...
});

[do-not-use, 2013-08-19 20:22:49]

On 2013/08/19 20:01:50, abarth wrote:
> https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent.h
> File Source/core/dom/MessageEvent.h (right):
> 
>
https://codereview.chromium.org/23319002/diff/1/Source/core/dom/MessageEvent....
> Source/core/dom/MessageEvent.h:80: static PassRefPtr<MessageEvent>
create(const
> AtomicString& type, const MessageEventInit& initializer)
> This constructor is the problematic one.  With your CL, I believe you can now
> (incorrectly) construct a MessageEvent with a Node (or any other EventTarget)
> using a DOM4-style constructor:
> 
> var evt = new MessageEvent({
>   source: document.body,
>   // etc...
> });

I kept source in MessageEventInit as a RefPtr<DOMWindow> to comply with the
specification. Therefore, you cannot construct it using anything else that a
Window as Source for now. 

The generated code looks like:
bool fillMessageEventInit(MessageEventInit& eventInit, const Dictionary&
options)
{
    // ...
    options.get("source", eventInit.source); // Calls Dictionary::get(const
String&, RefPtr<DOMWindow>&) const;
    options.get("ports", eventInit.ports);
    return true;
}

If the spec gets updated to accept source as a (Window or MessagePort) in
MessageEventInit, then I will need to use RefPtr<EventTarget> type in
MessageEventInit on native side and I will indeed need to be careful and check
what kind of EventTarget it is. However, right now this is not an issue. I will
need to ignore source if it is not a DOMWindow or a MessagePort.

When we arrive to that point, we will need an EventTarget::toMessagePort()
anyway. So we might as well introduce it now and add assertions in the
constructors as you suggested (except for the one taking a MessageEventInit).

[abarth-chromium, 2013-08-19 20:25:07]

On 2013/08/19 20:22:49, Christophe Dumez wrote:
> I kept source in MessageEventInit as a RefPtr<DOMWindow> to comply with the
> specification. Therefore, you cannot construct it using anything else that a
> Window as Source for now.

Great!  Can you please add a test and some ASSERTS to validate that this remains
true?

[do-not-use, 2013-08-19 20:26:47]

On 2013/08/19 20:25:07, abarth wrote:
> On 2013/08/19 20:22:49, Christophe Dumez wrote:
> > I kept source in MessageEventInit as a RefPtr<DOMWindow> to comply with the
> > specification. Therefore, you cannot construct it using anything else that a
> > Window as Source for now.
> 
> Great!  Can you please add a test and some ASSERTS to validate that this
remains
> true?

Yes, I will do that and reupload.

[do-not-use, 2013-08-19 20:47:55]

I added assertions and tests as suggested.

https://codereview.chromium.org/23319002/diff/13001/LayoutTests/fast/events/c...
File LayoutTests/fast/events/constructors/message-event-constructor.html
(right):

https://codereview.chromium.org/23319002/diff/13001/LayoutTests/fast/events/c...
LayoutTests/fast/events/constructors/message-event-constructor.html:76:
shouldBe("new MessageEvent('eventType', { ports: [channel.port1], source:
channel.port1 }).source", "null");
I added this check as well since I assume this is a bug in the specification and
we'll likely fix this later.

https://codereview.chromium.org/23319002/diff/13001/LayoutTests/fast/events/c...
LayoutTests/fast/events/constructors/message-event-constructor.html:81:
shouldBe("new MessageEvent('eventType', { source: document.body }).source",
"null");
Added a Node check as suggested.

[abarth-chromium, 2013-08-19 21:35:52]

LGTM

Thanks!

[commit-bot: I haz the power, 2013-08-20 05:52:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23319002/13001

[commit-bot: I haz the power, 2013-08-20 06:00:26]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-08-20 09:21:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23319002/13001

[commit-bot: I haz the power, 2013-08-20 09:25:20]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[do-not-use, 2013-08-20 11:10:54]

Committed patchset #2 manually as r156385 (presubmit successful).

[arv (Not doing code reviews), 2013-08-20 13:52:29]

LGTM