848294 - Update MessageEvent to be compatible with the spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Do this after bug 847583.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch

MessagePortList.webidl was missing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 800828 [details] [diff] [review]
patch

>+++ b/content/events/src/nsDOMMessageEvent.cpp
>+    event->mOrigin.Assign(aParam.mOrigin.Value());

Why not operator= like for mLastEventId?

>+    nsCOMPtr<nsIXPConnectWrappedNative> wrappedNative;

Hrm.  This is annoying; I guess we can't just make "source" nsISupports in the WebIDL because there's no way to extract a MessagePort from such an nsISupports?  :(

In any case, the code here is wrong because if "source":null is passed it will throw from the binding.  But per spec, null should be allowed.  So we should, imo, make it:

  object? source = null;

in the IDL and then no have to worry about WasPassed bits.  You'll need to worry about null, though.  Add tests for { "source":null }?

>+      nsISupports* supports = wrappedNative->Native();
>+      nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(supports);

do_QueryWrappedNative?

>+      if (window) {

And no need for that either.  So I think you can just do:

  if (wrappedNative) {
    mWindowSource = do_QueryWrappedNative(wrappedNative);
  }

>+      if (NS_SUCCEEDED(rv)) {
>+        event->mPortSource = port;

And else need to throw because the passed-in object was neither Window nor MessagePort.  Again, please add tests.

>+    for (uint32_t i = 0, len = aParam.mPorts.Value().Value().Length(); i < len; ++i) {

Why?  Just make the MessagePortList take a const nsTArray<nsRefPtr<MessagePort> reference, and pass aParam.mPorts.Value().Value() directly to it.  Except you want to not do that if aParam.mPorts.Value().IsNull().  Add a test that passes {"ports":null} in the dictionary?

>+++ b/content/events/src/nsDOMMessageEvent.h
>+#include "mozilla/dom/UnionTypes.h"

Please, no: it pulls in tons of headers.  Just forward-declare the union struct here in the header, then #include UnionTypes in the .cpp.

>+++ b/dom/webidl/MessageEvent.webidl
>+  // TODO bug 767926 - This should be: (WindowProxy or MessagePort)? source;

Please file a followup bug for fixing that, mark it dependent on bug 767926.

>+  // TODO This should be: MessagePort[]? ports;
>+  // TODO This should be: readonly attribute MessagePort[]? ports;

No, they shouldn't.  What should happen is spec issues raised to fix the spec.

r=me with that all done.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that I filed spec bug https://www.w3.org/Bugs/Public/show_bug.cgi?id=23176 and posted http://lists.w3.org/Archives/Public/public-webapps/2013JulSep/0445.html

Comment 5

[Andrea Marchesini [:baku]]

> >+    for (uint32_t i = 0, len = aParam.mPorts.Value().Value().Length(); i < len; ++i) {
> 
> Why?  Just make the MessagePortList take a const
> nsTArray<nsRefPtr<MessagePort> reference, and pass
> aParam.mPorts.Value().Value() directly to it.  Except you want to not do
> that if aParam.mPorts.Value().IsNull().  Add a test that passes
> {"ports":null} in the dictionary?

I cannot pass a Sequence<OwningNonNull<MessagePort>> as argument for a nsTArray<nsRefPtr<MessagePort>>.
But I can change MessagePort in order to use Sequence<OwningNonNull<MessagePort>>, What do you think?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch

We still need to update MessageEvent in workers. Can it be a follow up?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=5923a1a197c9

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I cannot pass a Sequence<OwningNonNull<MessagePort>>

Oh, indeed.

> But I can change MessagePort in order to use Sequence<OwningNonNull<MessagePort>>

You mean MessagePortList?  Since this is the only consumer, I think that change would be just fine.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, aligning workers with this in a followup seems fine to me.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Two more things:

1)  MessagePortList might want to be [ArrayClass]
2)  MessagePortList might want to be [NoInterfaceObject]

Comment 11

[Boris Zbarsky [:bzbarsky]]

And if we do #2 we don't have to change test_interfaces.html.

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9563af1fa1 should fix the struct vs class issue on MSVC.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f9563af1fa1

Comment 14

[Boris Zbarsky [:bzbarsky]]

Andrea, see comment 10?

Comment 15

[Andrea Marchesini [:baku]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Andrea, see comment 10?

Yes. This bug is not fixed yet. I didn't land the patch. I have a new version with these 2 comments applied, but it's not 'green' enough on try. I hope to have something ready for today and then land it.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: ports.patch

https://tbpl.mozilla.org/?tree=Try&rev=8ee7b038a997

Comment 17

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83075b26f91b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83075b26f91b

Comment 19

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26/Site_Compatibility

Comment 20

[predrag]

It would be nice if there was some information on how to switch from initMessageEvent to the new initEvent. Right now it looks like it is nowhere to be found.

Comment 21

[Jean-Yves Perrier [:teoli]]

I'm resetting dev-doc-needed here, it happens that our documentation is not complete: e.g. the constructor added here is not documented.

:kohei, please don't set dev-doc-complete when only a subset of the docs is updated. Thank you.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

baku, do you recall why initMessageEvent was removed? Was it not in the spec at that point or what?
Since it is there now https://html.spec.whatwg.org/multipage/comms.html#the-messageevent-interfaces
See https://github.com/whatwg/html/pull/223

Comment 23

[Andrea Marchesini [:baku]]

I just summited a patch to reintroduce this method. Bug 949376.