Closed Bug 848294 Opened 7 years ago Closed 6 years ago

Update MessageEvent to be compatible with the spec

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: smaug, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(1 file, 4 obsolete files)

Do this after bug 847583.
Attached patch patch (obsolete) — Splinter Review
Attachment #800819 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
MessagePortList.webidl was missing.
Attachment #800819 - Attachment is obsolete: true
Attachment #800819 - Flags: review?(bzbarsky)
Attachment #800828 - Flags: review?(bzbarsky)
Assignee: nobody → amarchesini
Keywords: dev-doc-needed
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.
Attachment #800828 - Flags: review?(bzbarsky) → review+
Blocks: 913770
> >+    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?
Attached patch patch (obsolete) — Splinter Review
We still need to update MessageEvent in workers. Can it be a follow up?
Attachment #800828 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=5923a1a197c9
Attachment #801091 - Attachment is obsolete: true
> 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.
Flags: needinfo?(bzbarsky)
Oh, aligning workers with this in a followup seems fine to me.
Two more things:

1)  MessagePortList might want to be [ArrayClass]
2)  MessagePortList might want to be [NoInterfaceObject]
And if we do #2 we don't have to change test_interfaces.html.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9563af1fa1 should fix the struct vs class issue on MSVC.
https://hg.mozilla.org/mozilla-central/rev/4f9563af1fa1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Andrea, see comment 10?
Flags: needinfo?(amarchesini)
(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Depends on: 914621
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/83075b26f91b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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.
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.
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
Flags: needinfo?(amarchesini)
I just summited a patch to reintroduce this method. Bug 949376.
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.