828261 - Implement `window.location.origin`.

Status: RESOLVED FIXED

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

Description

[Mike West]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.22 (KHTML, like Gecko) Chrome/25.0.1364.5 Safari/537.22

Steps to reproduce:

I tried to validate a `message` event's `source` attribute against `window.location.origin` in the following commit: https://github.com/mikewest/www.html5rocks.com/commit/884a4c24c0d84258e208c8c88f68e8ac227b99a4#L3R88


Actual results:

`window.location.origin` is undefined.


Expected results:

`window.location.origin` should have been populated correctly with the origin of the current document, as specified at http://url.spec.whatwg.org/#dom-url-origin

Comment 1

[Mike West]

Attachment: WIP

With the caveat that I Have No Idea What I'm Doing™, and I can't quite get mozilla-central compiling locally, the attached patch seems like a reasonable start at this. :)

Comment 2

[Mike West]

Attachment: WIP: Needs tests.

Needs tests. I'll see if I can track someone down to help me figure that out.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Probably better to use nsContentUtils::GetUTFOrigin instead of reinventing that wheel, assuming this is the same origin as webapps defines.

How stable is the spec for this at this point?  Last I had checked people were arguing about whether to expose this at all....

Comment 4

[Mike West]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Probably better to use nsContentUtils::GetUTFOrigin instead of reinventing
> that wheel, assuming this is the same origin as webapps defines.

I figured the logic already existed somewhere. Thanks for the tip.

> How stable is the spec for this at this point?  Last I had checked people
> were arguing about whether to expose this at all....

It's been in WebKit since 2010: https://bugs.webkit.org/show_bug.cgi?id=46558

Given that it's the best way of checking a MessageEvent's origin, I do think it's worth implementing the same API here.

Comment 5

[Ian Melven :imelven]

(In reply to Mike West from comment #2)
> Created attachment 699867 [details] [diff] [review]
> WIP: Needs tests.
> 
> Needs tests. I'll see if I can track someone down to help me figure that out.

Hi Mike, 

it seems like you can test this all in content, so https://developer.mozilla.org/en-US/docs/Mochitest is a good place to start. You can run a single mochitest with "mach" using ./mach mochitest-plain <path to mochitest> . http://mxr.mozilla.org/mozilla-central/source/content/base/test/ has a lot of tests you can check out for examples.

./mach build is a good thing to try wrt getting your build going if you aren't using that already, as well.

Feel free to ping me on irc.mozilla.org if you like as well.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Bobby, what's the right place to add tests for new location properties?

Comment 7

[Mike West]

Attachment: nsContentUtils::GetUTFOrigin + Mochitest.

I've switched to using the nsContentUtils::GetUTFOrigin method, and have added a single Mochitest that enumerates the interesting properties of `window.location`. It's fairly basic; I'll flesh it out once you let me know where the test should live. :)

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Bobby, what's the right place to add tests for new location properties?

Well, there's mochitest/dom-level0/test_location.html. They could go there, I guess, or somewhere else in dom/. Probably not in content/ though.

Comment 9

[Mike West]

Attachment: Moar tests.

Alright. I've moved the test, and added two more. May I add one of you fine folks as a reviewer for this patch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 700044 [details] [diff] [review]
Moar tests.

Bobby, want to review?  If not, please punt to sicking or me as needed, I guess.

Jonas, tagging you for the sr.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 700044 [details] [diff] [review]
Moar tests.

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

Looks good in general, though I have a couple of questions here, so cancelling review for now.

Nice tests btw ;-)

::: dom/base/nsLocation.cpp
@@ +588,5 @@
> +
> +  aOrigin.Truncate();
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult result;

Modern Gecko convention calls this |rv|, and declares it the first time it's used. Sorry some of the code you're looking at here is so crufty :-(

@@ +590,5 @@
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult result;
> +
> +  result = GetURI(getter_AddRefs(uri), true);

I'm not sure if passing true here is appropriate. Please get an explicit answer from someone who knows. Maybe Boris or Sicking?

@@ +592,5 @@
> +  nsresult result;
> +
> +  result = GetURI(getter_AddRefs(uri), true);
> +
> +  if (uri) {

I'd prefer to flip this logic, so that the bailout/return case comes first.

@@ +594,5 @@
> +  result = GetURI(getter_AddRefs(uri), true);
> +
> +  if (uri) {
> +    nsAutoString origin;
> +    result = nsContentUtils::GetUTFOrigin(uri, origin);

Do NS_ENSURE_SUCCESS(rv, rv) to handle the error case. It logs in debug builds and saves a level of indentation.

@@ +601,5 @@
> +      return NS_OK;
> +    }
> +  }
> +
> +  aOrigin.AssignLiteral("null");

Hm, is this what we want to be returning here? I have no idea. Maybe Boris knows.

::: dom/tests/mochitest/dom-level0/test_location_sandboxed.html
@@ +18,5 @@
> +    is(loc.hash, '', 'Unexpected hash.');
> +    is(loc.host, 'mochi.test:8888', 'Unexpected host.');
> +    is(loc.hostname, 'mochi.test', 'Unexpected hostname.');
> +    // Is this correct? It matches WebKit, but it seems wrong:
> +    // https://bugs.webkit.org/show_bug.cgi?id=106488

I'm not sure. I'll CC imelven.

Comment 12

[Bobby Holley (:bholley)]

Needinfo from bz per the above.

Comment 13

[Bobby Holley (:bholley)]

Needinfo from imelven as well.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if passing true here is appropriate.

Passing true is the right thing there.

> Hm, is this what we want to be returning here? 

I don't think we should be ending up there at all.  The cases in which we can't get a URI or nsContentUtils::GetUTFOrigin fails are exceptional cases that should return an error nsresult (basically out of memory or an incredibly broken URI implementation).

Comment 15

[Mike West]

(In reply to Bobby Holley (:bholley) from comment #11)
> Nice tests btw ;-)

I like tests. :)

> Modern Gecko convention calls this |rv|, and declares it the first time it's
> used. Sorry some of the code you're looking at here is so crufty :-(

I'm shocked! Cargo culting never got me in trouble before! :)

So this should instead read as follows?

    nsresult nv = nsresult rv = GetURI(getter_AddRefs(uri), ...);

> I'd prefer to flip this logic, so that the bailout/return case comes first.

Makes sense.

> Do NS_ENSURE_SUCCESS(rv, rv) to handle the error case. It logs in debug
> builds and saves a level of indentation.

Got it.

> 
> @@ +601,5 @@
> > +      return NS_OK;
> > +    }
> > +  }
> > +
> > +  aOrigin.AssignLiteral("null");
> 
> Hm, is this what we want to be returning here? I have no idea. Maybe Boris
> knows.

The spec says that if URL is empty, we should return the empty string (http://url.spec.whatwg.org/#dom-url-origin), so this isn't necessary at all. Thanks.

> ::: dom/tests/mochitest/dom-level0/test_location_sandboxed.html
> @@ +18,5 @@
> > +    is(loc.hash, '', 'Unexpected hash.');
> > +    is(loc.host, 'mochi.test:8888', 'Unexpected host.');
> > +    is(loc.hostname, 'mochi.test', 'Unexpected hostname.');
> > +    // Is this correct? It matches WebKit, but it seems wrong:
> > +    // https://bugs.webkit.org/show_bug.cgi?id=106488
> 
> I'm not sure. I'll CC imelven.

I'm poking abarth@ on the other bug. I suspect this is accidental and I'll poke at WHATWG about it, but probably good enough for the moment.

Comment 16

[Mike West]

Attachment: bholley's feedback.

Here's a pass at cleaning up the bits you noted. Thanks for taking a look!

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 700074 [details] [diff] [review]
bholley's feedback.

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

r=bholley with comments addressed.

::: dom/base/nsLocation.cpp
@@ +596,5 @@
> +  rv = nsContentUtils::GetUTFOrigin(uri, origin);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  aOrigin = origin;
> +  return NS_OK;

I think you can get rid of the nsAutoString and do:

return nsContentUtils::GetUTFOrigin(uri, aOrigin);

::: dom/interfaces/base/nsIDOMLocation.idl
@@ +22,1 @@
>             attribute DOMString        protocol;

You need to rev the iid of this interface when making a change to it.

Comment 18

[Mike West]

(In reply to Bobby Holley (:bholley) from comment #17)
> Comment on attachment 700074 [details] [diff] [review]
> bholley's feedback.
> 
> Review of attachment 700074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=bholley with comments addressed.
> 
> ::: dom/base/nsLocation.cpp
> @@ +596,5 @@
> > +  rv = nsContentUtils::GetUTFOrigin(uri, origin);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  aOrigin = origin;
> > +  return NS_OK;
> 
> I think you can get rid of the nsAutoString and do:
> 
> return nsContentUtils::GetUTFOrigin(uri, aOrigin);

I don't think I can, at least, that code doesn't compile as written. GetUTFOrigin expects a nsString. aOrigin is a nsAString.

Can those be cast back and forth somehow? Sorry, I'm simply not familiar at all with the internals here.

> 
> ::: dom/interfaces/base/nsIDOMLocation.idl
> @@ +22,1 @@
> >             attribute DOMString        protocol;
> 
> You need to rev the iid of this interface when making a change to it.

Happy to... can you point me at a doc? Looking at the history for the file, I'm assuming that there's some sort of script?

Comment 19

[Ian Melven :imelven]

(In reply to Mike West from comment #18)
> 
> Happy to... can you point me at a doc? Looking at the history for the file,
> I'm assuming that there's some sort of script?

https://developer.mozilla.org/en-US/docs/Generating_GUIDs has some online tools etc.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Mike West from comment #18)

> I don't think I can, at least, that code doesn't compile as written.
> GetUTFOrigin expects a nsString. aOrigin is a nsAString.

Oh whoops, my bad. Ignore that comment.

> > You need to rev the iid of this interface when making a change to it.
> 
> Happy to... can you point me at a doc? Looking at the history for the file,
> I'm assuming that there's some sort of script?

http://mozilla.pettay.fi/cgi-bin/mozuuid.pl

Comment 21

[Ian Melven :imelven]

(In reply to Mike West from comment #18)
> 
> I don't think I can, at least, that code doesn't compile as written.
> GetUTFOrigin expects a nsString. aOrigin is a nsAString.
> 
> Can those be cast back and forth somehow? Sorry, I'm simply not familiar at
> all with the internals here.

https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide has some details. nsAString is an abstract class.

Comment 22

[Mike West]

(In reply to Bobby Holley (:bholley) from comment #20)
> > Happy to... can you point me at a doc? Looking at the history for the file,
> > I'm assuming that there's some sort of script?
> 
> http://mozilla.pettay.fi/cgi-bin/mozuuid.pl

I see. There's no rhyme or reason, just uniqueness. :)

A silly process question: when I upload the next iteration of the patch, should I set r+ for you, and sr? for Jonas? Or change the commit message to include "r=bholley"?

Comment 23

[Ian Melven :imelven]

(In reply to Mike West from comment #15)
 
> > ::: dom/tests/mochitest/dom-level0/test_location_sandboxed.html
> > @@ +18,5 @@
> > > +    is(loc.hash, '', 'Unexpected hash.');
> > > +    is(loc.host, 'mochi.test:8888', 'Unexpected host.');
> > > +    is(loc.hostname, 'mochi.test', 'Unexpected hostname.');
> > > +    // Is this correct? It matches WebKit, but it seems wrong:
> > > +    // https://bugs.webkit.org/show_bug.cgi?id=106488
> > 
> > I'm not sure. I'll CC imelven.
> 
> I'm poking abarth@ on the other bug. I suspect this is accidental and I'll
> poke at WHATWG about it, but probably good enough for the moment.

yeah, that seems wrong to me as well, i'd expect to see the unique origin of the sandboxed document here. I'll keep an eye out for the WHATWG discussion.

Comment 24

[Ian Melven :imelven]

(In reply to Mike West from comment #22)
>
> A silly process question: when I upload the next iteration of the patch,
> should I set r+ for you, and sr? for Jonas? Or change the commit message to
> include "r=bholley"?

You should do both of those options, IMO. The r= is needed in the commit message before landing (there's a hook that checks for it).

Comment 25

[Mike West]

Attachment: Reviewed patch.

Carrying over r+ from the last patch, though it doesn't give me a field into which to put bholley's name...

@imelvin: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-January/038603.html

Comment 26

[Mike West]

Attachment: _Actually_ changing the UUID.

Add, then commit, then format-patch. *sigh*

Comment 27

[Bobby Holley (:bholley)]

(In reply to Mike West from comment #25)
> Created attachment 700102 [details] [diff] [review]
> Reviewed patch.
> 
> Carrying over r+ from the last patch, though it doesn't give me a field into
> which to put bholley's name...

You can just self-review it with r=bholley in the name of the patch. Hacky, I know.

I'll r+ it now.

Comment 28

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f53242d94bc

Comment 29

[Ian Melven :imelven]

(In reply to Mike West from comment #25)
> Created attachment 700102 [details] [diff] [review]
> Reviewed patch.
> 
> Carrying over r+ from the last patch, though it doesn't give me a field into
> which to put bholley's name...
> 
> @imelvin:
> http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-January/038603.html

After Anne's clarification, I see what Adam was saying in the Webkit bug now about location.origin being the origin of the location and not the document - so it seems like the test is correct even though it's a little counterintuitive perhaps.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Things look green.

Mike, thank you for the patch!

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5f53242d94bc

Comment 32

[Tom S [:evilpie]]

Mentioned on https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers and
https://developer.mozilla.org/en-US/docs/DOM/window.location.

Comment 33

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to Tom Schuster [:evilpie] from comment #32)
> Mentioned on
> https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers and
> https://developer.mozilla.org/en-US/docs/DOM/window.location.

In MDN is not mentioned that this will only work in FF21+, unless it's planned to be backported.

Comment 34

[Tom S [:evilpie]]

Then change it, instead of mentioning it here.

Comment 35

[David Bruant]

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #33)
> (In reply to Tom Schuster [:evilpie] from comment #32)
> > Mentioned on
> > https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers and
> > https://developer.mozilla.org/en-US/docs/DOM/window.location.
> 
> In MDN is not mentioned that this will only work in FF21+, unless it's
> planned to be backported.
Added browser compatibility table.
https://developer.mozilla.org/en-US/docs/DOM/window.location$compare?to=371021&from=370723
https://developer.mozilla.org/en-US/docs/DOM/window.location#Browser_compatibility


(In reply to Tom Schuster [:evilpie] from comment #34)
> Then change it, instead of mentioning it here.
Indeed. MDN is a wiki. Anyone is allowed to change it. I made the change this time, but please contribute (Hernan) especially for small things like this. Feel free to send me an email or a tweet if you need a review or advice or if you're not sur about something.

Comment 36

[Tom S [:evilpie]]

I just added an extra row for window.location.origin in the compatibility table.

Comment 37

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to David Bruant from comment #35)
> (In reply to Tom Schuster [:evilpie] from comment #34)
> > Then change it, instead of mentioning it here.
> Indeed. MDN is a wiki. Anyone is allowed to change it. I made the change
> this time, but please contribute (Hernan) especially for small things like
> this. Feel free to send me an email or a tweet if you need a review or
> advice or if you're not sur about something.

No need to be harsh here. I know MDN is a wiki, it's just not my area on contributing, thought somebody else could do a better job there. Thanks for adding the compatibility.