Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Reporter
	Description • 11 years ago

This way devtools team would be able to start using them.

	(no longer active)
	Comment 2 • 11 years ago

I'd like to have Boris' blessing here first.

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

This seems ok to me, as long as what we implement follows the spec and the spec is fairly stable.  Andrea, are we there?

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

(In reply to Boris Zbarsky [:bz] from comment #3)
> This seems ok to me, as long as what we implement follows the spec and the
> spec is fairly stable.  Andrea, are we there?

not yet. currently we have just window-to-window. No worker support for MessagePort yet - bug 911972.
Plus, postMessage() is not following the spec fully - bug 912456

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

Unimplemented parts are OK here as far as I'm concerned.  What I'd like to avoid is chrome callers starting to depend on non-spec behavior and us then breaking them when we start to follow the spec.  Is bug 912456 something that could cause that?

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

Yes. Because the spec wants something like:

mp.postMessage(null, [port])

and we don't support this without that bug fixed.

The same is here:

mp.postMessage({ port: port });

should throw because the transferable object list doesn't contain the port. Here the correct way to write that code:

mp.postMessage({ port: port }, [port]);

Without that bug fixed, the first line doesn't throw.

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

Yeah, that would be a blocker in my book for enabling this for random extensions to use...

	(no longer active)
	Comment 8 • 11 years ago

(In reply to comment #7)
> Yeah, that would be a blocker in my book for enabling this for random
> extensions to use...

Yes, I think so too.

	Dave Townsend [:mossop]
	Comment 9 • 11 years ago

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> (In reply to comment #7)
> > Yeah, that would be a blocker in my book for enabling this for random
> > extensions to use...
> 
> Yes, I think so too.

What do we need in order to move bug 912456 forwards?

	Dave Townsend [:mossop]
	Comment 10 • 11 years ago

Now that bug 912456 is fixed can we move forwards here? Who might be able to work on this?

	(no longer active)
	Comment 11 • 11 years ago

I think so, but we should check with Andrea.  Not sure if Andrea has the cycles to work on this or not, I'll let him speak to that effect.  :-)

	(no longer active)
	Comment 12 • 11 years ago

(Oh, and I'm so sorry that I didn't see your comment before, Dave...  I'm close to declaring email bankruptcy.)

	Dave Townsend [:mossop]
	Comment 13 • 11 years ago

This is blocking us from landing some work. I have someone else who might be able to help get this exposed but I really want to hear whether we think we're ready to do that first.

	(no longer active)
	Comment 14 • 11 years ago

Andrea?

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

Boris, just a feedback because I don't know if this is the right approach. Thanks.

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

Comment on attachment 8423347 [details] [diff] [review]
chrome.patch

A few issues:

1)  Please don't reinvent xpc::WindowOrNull.
2)  The version of MessageChannel::Enabled that takes an nsPIDOMWindow* doesn't
    need a JSContext*, right?
3)  Checking the document URI seems pretty broken.  Shouldn't this be based on
    the principal?  Why do we care about resource:// exactly?  Seems like this
    should just be done for the IsCallerChrome() cases or something, no?
4)  Why do we need to check enabled in the structured clone bits at all?

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

Comment on attachment 8424895 [details] [diff] [review]
chrome.patch

This doesn't address comment 16 item 3.

Worse yet, this will disable if IsCallerChrome but the URI is not chrome: or resource:, no matter what the pref is set to.  That seems bogus.

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

gozala, do you really care about the scheme of the URI or do you want to have MessagePort/MessageChannel enabled for chrome code?

	Irakli Gozalishvili [:irakli] [:gozala] [@gozala] Reporter
	Comment 20 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #19)
> gozala, do you really care about the scheme of the URI or do you want to
> have MessagePort/MessageChannel enabled for chrome code?

Andrea, it really depends what we mean by enabling for chrome. If that would let chrome code create a message channel and post to a non-chrome privileged document so they could communicate with it, then I'd say it get's 90% of what we need. Only missing peace would be that document we're creating pipeline with won't be able to create ports to send them back, but that can probably wait until message channels are fully enabled.

If chrome code will send message port to a non chrome privileged document, but than that document won't be able to use it to communicate through that port, then enabling only for chrome is not enough I'm afraid.

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 10 years ago

bz, if we want to proceed with this bug, we have to allow chrome and 'resource' scheme url to use MessagePort/MessageChannel. How does it sound?

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

Comment on attachment 8433492 [details] [diff] [review]
chrome.patch

Just making this chromeonly would handle the 90% case.

But if we do want to expose to resource, we should be basing the check on the principal.  And in particular, we should NOT expose to chrome:// that is not system principal, imo.

	Andrea Marchesini [:baku] Assignee
	Comment 24 • 10 years ago

Here we have the principal check + a mochitest where we have a chrome page loading a html page and sending messages using MessagePort.

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

Comment on attachment 8434331 [details] [diff] [review]
chrome.patch

>+  if (!principal) {

Can't be null.  That's why it has no "Get" prefix.

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 26 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=23cf03ce4362

	Andrea Marchesini [:baku] Assignee
	Comment 27 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 28 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b28cd167b6fe

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b28cd167b6fe