Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 11 years ago

Do this after bug 847583.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 11 years ago

MessagePortList.webidl was missing.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 5 • 11 years ago

> >+    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?

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 11 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

Two more things:

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

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 11 years ago

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

	(no longer active)
	Comment 12 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 11 years ago

Andrea, see comment 10?

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

(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.

	Andrea Marchesini [:baku] Assignee
	Comment 16 • 11 years ago

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

	(no longer active)
	Comment 17 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

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

	Kohei Yoshino [:kohei]
	Comment 19 • 11 years ago

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

	predrag
	Comment 20 • 11 years ago

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.

	Jean-Yves Perrier [:teoli]
	Comment 21 • 9 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 22 • 9 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 23 • 9 years ago

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