Boris Zbarsky [:bzbarsky] Assignee
	Description • 8 years ago

Need to check whether the spec makes sense.

	Ben Kelly [:bkelly, not reviewing]
	Comment 1 • 8 years ago

The spec is basically:

interface WindowOrWorkerGlobalScope {
  [Replaceable] readonly attribute USVString origin;
};

"The origin attribute's getter must return this object's relevant setting object's origin, serialized."

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 8 years ago

Yeah.  That doesn't seem unreasonable.  For the window case, we'd just GetUTFOrigin(GetPrincipal(), aRetVal).  

For the worker case, there's the mLocationInfo.mOrigin on the WorkerPrivate, but this does NOT seem like the right thing.  In particular, it's munged by WorkerPrivateParent<Derived>::SetBaseURI, right?  Or will that always end up with something with the right origin string?  It's not clear to me why it would, necessarily.

There's also mLoadInfo.mPrincipal, but we don't really want to do GetUTFOrigin on a worker thread, right?  Can we just grab the origin in WorkerPrivateParent<Derived>::SetPrincipal, and will that be the right origin?

	Ben Kelly [:bkelly, not reviewing]
	Comment 3 • 8 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> There's also mLoadInfo.mPrincipal, but we don't really want to do
> GetUTFOrigin on a worker thread, right?  Can we just grab the origin in
> WorkerPrivateParent<Derived>::SetPrincipal, and will that be the right
> origin?

I think that's the right thing to do, but I haven't looked in detail.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 7 years ago

Parts 3 and 4 correspond to https://github.com/w3c/testharness.js/pull/241 and https://github.com/w3c/testharness.js/pull/242 respectively, so they're getting review upstream too.

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 7 years ago

Comment on attachment 8835607 [details] [diff] [review]
part 1.  Change the HTML general interfaces web platform test to have WindowOrWorkerGlobalScope

Review of attachment 8835607 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/dom/interfaces.html
@@ +2502,4 @@
>  
>  [NoInterfaceObject, Exposed=(Window,Worker)]
> +interface WindowOrWorkerGlobalScope {
> +  DOMString btoa(DOMString btoa);

nit:  The spec also has this comment:

  // base64 utility methods

	Ben Kelly [:bkelly, not reviewing]
	Comment 12 • 7 years ago

Comment on attachment 8835608 [details] [diff] [review]
part 2.  Split the IDLs out of the HTML interfaces web platform test so we can run it in both windows and workers

Review of attachment 8835608 [details] [diff] [review]:
-----------------------------------------------------------------

Trusting that the .idl files are right.  Splinter did not really make it easy to read the hg copy + changes here.

::: testing/web-platform/tests/html/dom/interfaces.html
@@ +217,5 @@
>  };
> +
> +function fetchData(url) {
> +  return new Promise(function(resolve, reject) {
> +    var xhr = new XMLHttpRequest();

The `fetch()` API has been shipped by chrome, firefox, safari, and edge.  Might be easier to just use that here since you want a promise anyway?

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 7 years ago

Comment on attachment 8835610 [details] [diff] [review]
part 4.  Fix idlharness to properly handle FrozenArray return types

Review of attachment 8835610 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/dom/interfaces.html
@@ +201,5 @@
>      CloseEvent: ['new CloseEvent("close")'],
>      AbstractWorker: [],
>      Worker: [],
>      SharedWorker: [],
> +    MessageEvent: ['new MessageEvent("message", { data: 5 })'],

This exercises the FrozenArray test via the ports member?

	Ben Kelly [:bkelly, not reviewing]
	Comment 14 • 7 years ago

Comment on attachment 8835609 [details] [diff] [review]
part 3.  Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members

Review of attachment 8835609 [details] [diff] [review]:
-----------------------------------------------------------------

Can you describe the problem that needs to be solved here?  I kind of understand what you are doing, but its unclear to me exactly what is being fixed.

	Ben Kelly [:bkelly, not reviewing]
	Comment 15 • 7 years ago

Comment on attachment 8835611 [details] [diff] [review]
part 5.  Run the HTML interfaces web platform test in workers as well as windows

Review of attachment 8835611 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/html/dom/interfaces.worker.js.ini
@@ +1,4 @@
> +[interfaces.worker.html]
> +  type: testharness
> +  [Path2D interface: existence and properties of interface object]
> +    expected: FAIL

These failures match the failures we get in window tests, right?

::: testing/web-platform/tests/html/dom/interfaces.worker.js
@@ +21,5 @@
> +};
> +
> +function fetchData(url) {
> +  return new Promise(function(resolve, reject) {
> +    var xhr = new XMLHttpRequest();

Again I think we could use fetch().

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 7 years ago

Comment on attachment 8835612 [details] [diff] [review]
part 6.  Implement WindowOrWorkerGlobalScope.origin

Review of attachment 8835612 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +9962,5 @@
>  
>  void
> +nsGlobalWindow::GetOrigin(nsAString& aOrigin)
> +{
> +  MOZ_ASSERT(IsInnerWindow());

This is a cheap test, so lets use MOZ_DIAGNOSTIC_ASSERT().

::: dom/base/nsGlobalWindow.h
@@ +1003,5 @@
>                        const mozilla::dom::Optional<int32_t>& aTimeout,
>                        const mozilla::dom::Sequence<JS::Value>& /* unused */,
>                        mozilla::ErrorResult& aError);
>    void ClearInterval(int32_t aHandle);
> +  void GetOrigin(nsAString& aOrigin);

Sad GetPrincipal() prevents us from making this const.

::: dom/workers/WorkerPrivate.cpp
@@ +1873,5 @@
>  
>    MOZ_ALWAYS_SUCCEEDS(
>      PrincipalToPrincipalInfo(aPrincipal, mPrincipalInfo));
> +
> +  nsContentUtils::GetUTFOrigin(aPrincipal, mOrigin);

SetPrincipalOnMainThread() is about to become fallible in bug 1338782 (I'm going to land today).  Please change this to return the nsresult.

::: dom/workers/WorkerScope.h
@@ +139,5 @@
>    void
>    ClearInterval(int32_t aHandle);
>  
>    void
> +  GetOrigin(nsAString& aOrigin);

I think this can be const.

	Ben Kelly [:bkelly, not reviewing]
	Comment 17 • 7 years ago

NI for comment 14.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 7 years ago

Good catch; I need to give that commit a much better commit message.

So there are two problems being solved in attachment 8835609 [details] [diff] [review], and maybe I should pull them out into separate commits...  then again, this already got merged as a single commit upstream, so there's not much point.  And upstream won't have the better commit message either.  :(

1) Consider this interface:

  [Exposed=(Window,Worker)]
  interface Foo {
    [Exposed=(Window)]
    void someMethod();
  };

Without my changes, idlharness will check for a someMethod method on Foo.prototype in all globals where the Foo interface is exposed, and will fail in a worker, because someMethod is not exposed there.  That's what the blocks ending with "continue" in IdlInterface.prototype.test_members and IdlInterface.prototype.test_interface_of fix.  This needs the "default_set" argument to exposure set computation, because if there isn't any exposure set specified on a member, it gets the exposure set of its interface.

2)  Consider this interface:

  [Exposed=SharedWorker]
  interface Foo {
  };

The code without my changes does:

        var globals = exposed.length === 1
                    ? exposed[0].rhs.value
                    : ["Window"];

which sets "globals" to the _string_ "SharedWorker".  Then it does member.exposed = exposed_in(globals).  expose_in tries to figure out which sort of global we're running in.  The interesting part is _just_ visible in the context in the diff.  Say we're running in a dedicated worker.  Then this test:

     if ('DedicatedWorkerGlobalScope' in self &&
         self instanceof DedicatedWorkerGlobalScope) {

tests true and this return value:

         return globals.indexOf("Worker") >= 0 ||
                globals.indexOf("DedicatedWorker") >= 0;

returns true, because if you recall the variable globals is the _string_ "SharedWorker", so globals.indexOf("Worker") >= 0.  Yay Python typing.

The fix for this is to make sure that globals ends up as the _list_ ["SharedWorker"] in this case, so our indexOf checks in exposed_in do the right things.  That's the:

+    if (typeof set == "string") {
+        set = [ set ];

bit.

	Ben Kelly [:bkelly, not reviewing]
	Comment 19 • 7 years ago

Comment on attachment 8835609 [details] [diff] [review]
part 3.  Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members

Thanks!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 7 years ago

> nit:  The spec also has this comment:

Added.

> Splinter did not really make it easy to read the hg copy + changes here.

:(  Anyway, I just hg copied and then deleted stuff (and added a few comments, I guess).  No other changes.

> Might be easier to just use that [fetch] here since you want a promise anyway?

Good point.  Done:

  function fetchData(url) {
    return fetch(url).then((response) => response.text());
  }

I agree this is much nicer.  ;)

> This exercises the FrozenArray test via the ports member?

Yes, exactly.

> These failures match the failures we get in window tests, right?

No.  There are three sources of failures here:

1) The Path2D failures are because per spec a bunch of the canvas stuff is exposed in workers, but we don't implement any of that yet.

2) We have an onlanguagechange event on Window, but we don't have it on WorkerGlobalScope.  Of course it's not like we ever fire languagechange events.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1154779

3) We don't support SharedWorker inside a worker.

> Again I think we could use fetch().

Done.

> This is a cheap test, so lets use MOZ_DIAGNOSTIC_ASSERT().

Done.

> Sad GetPrincipal() prevents us from making this const.

Hmm.  I could probably make GetPrincipal() const.  I'll throw a changeset on top of these.

> SetPrincipalOnMainThread() is about to become fallible

Updated:

  rv = nsContentUtils::GetUTFOrigin(aPrincipal, mOrigin);
  NS_ENSURE_SUCCESS(rv, rv);

> I think this can be const.

Yep, done.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 7 years ago

> Hmm.  I could probably make GetPrincipal() const.  I'll throw a changeset on top of these.

No, this doesn't work, because GetParentInternal() is _very_ hard to make const (e.g. it can return "this" in some cases).  So not going to worry about this for now.

	Pulsebot
	Comment 22 • 7 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb9b3880581
part 1.  Change the HTML general interfaces web platform test to have WindowOrWorkerGlobalScope.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/1907c1299943
part 2.  Split the IDLs out of the HTML interfaces web platform test so we can run it in both windows and workers.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a928f98527cc
part 3.  Fix idlharness to properly handle single-element exposure sets on interfaces and exposure sets on members.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/622aa19582d4
part 4.  Fix idlharness to properly handle FrozenArray return types.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ee2ce41916
part 5.  Run the HTML interfaces web platform test in workers as well as windows.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f74cfa4af59
part 6.  Implement WindowOrWorkerGlobalScope.origin.  r=bkelly

	Carsten Book [:Tomcat]
	Comment 23 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/abb9b3880581
https://hg.mozilla.org/mozilla-central/rev/1907c1299943
https://hg.mozilla.org/mozilla-central/rev/a928f98527cc
https://hg.mozilla.org/mozilla-central/rev/622aa19582d4
https://hg.mozilla.org/mozilla-central/rev/a3ee2ce41916
https://hg.mozilla.org/mozilla-central/rev/5f74cfa4af59

	Julien Cristau [:jcristau] (back April 22)
	Comment 24 • 7 years ago

Per the intent to ship on dev-platform the target for this is 54, marking 52/53 wontfix.

	Chris Mills [:cmills]
	Comment 25 • 7 years ago

I've documented this:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope
https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/origin

And added a note to the Fx54 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/54#Web_workers_and_Service_Workers

Let me know if these look OK. Thanks!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 7 years ago

Fixed some parts of the docs that weren't quite right.

	Chris Mills [:cmills]
	Comment 27 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #26)
> Fixed some parts of the docs that weren't quite right.

Thanks Boris!