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

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

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

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

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

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

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

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

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

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

Needinfo from bz per the above.

Needinfo from imelven as well.

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

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

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

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.

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

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

(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

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

(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"?

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

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

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

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

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

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

Things look green. Mike, thank you for the patch!

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

Then change it, instead of mentioning it here.

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

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

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