The default bug view has changed. See this FAQ.

Implement `window.location.origin`.

RESOLVED FIXED in mozilla21

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Mike West, Assigned: Mike West)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla21
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox -)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 699752 [details] [diff] [review]
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. :)
Component: Untriaged → DOM
Product: Firefox → Core
(Assignee)

Comment 2

4 years ago
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.
Attachment #699752 - Attachment is obsolete: true
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....
(Assignee)

Comment 4

4 years ago
(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

4 years ago
(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.

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Bobby, what's the right place to add tests for new location properties?
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 7

4 years ago
Created attachment 700009 [details] [diff] [review]
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. :)
Attachment #699867 - Attachment is obsolete: true
(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.
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 9

4 years ago
Created attachment 700044 [details] [diff] [review]
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?
Attachment #700009 - Attachment is obsolete: true
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.
Attachment #700044 - Attachment is patch: true
Attachment #700044 - Flags: superreview?(jonas)
Attachment #700044 - Flags: review?(bobbyholley+bmo)
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.
Attachment #700044 - Flags: review?(bobbyholley+bmo)
Needinfo from bz per the above.
Flags: needinfo?(bzbarsky)
Needinfo from imelven as well.
Flags: needinfo?(imelven)
> 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).
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
Created attachment 700074 [details] [diff] [review]
bholley's feedback.

Here's a pass at cleaning up the bits you noted. Thanks for taking a look!
Attachment #700044 - Attachment is obsolete: true
Attachment #700044 - Flags: superreview?(jonas)
Attachment #700074 - Flags: review?(bobbyholley+bmo)
Attachment #700044 - Flags: superreview?(jonas)
Attachment #700044 - Flags: review?(bobbyholley+bmo)
Attachment #700074 - Flags: superreview?(jonas)
Attachment #700044 - Flags: superreview?(jonas)
Attachment #700044 - Flags: review?(bobbyholley+bmo)
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.
Attachment #700074 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 18

4 years ago
(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

4 years ago
(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.
Flags: needinfo?(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.

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

4 years ago
(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.
(Assignee)

Comment 22

4 years ago
(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

4 years ago
(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.

Updated

4 years ago
Assignee: nobody → mkwst
Status: NEW → ASSIGNED

Comment 24

4 years ago
(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).
(Assignee)

Comment 25

4 years ago
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
Attachment #700074 - Attachment is obsolete: true
Attachment #700074 - Flags: superreview?(jonas)
Attachment #700102 - Flags: superreview?(jonas)
Attachment #700102 - Flags: review+
(Assignee)

Comment 26

4 years ago
Created attachment 700103 [details] [diff] [review]
_Actually_ changing the UUID.

Add, then commit, then format-patch. *sigh*
Attachment #700103 - Flags: superreview?(jonas)
(Assignee)

Updated

4 years ago
Attachment #700102 - Attachment is obsolete: true
Attachment #700102 - Flags: superreview?(jonas)
Attachment #700102 - Flags: review+
Attachment #700103 - Flags: superreview?(jonas) → superreview+
(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.
Keywords: checkin-needed
Attachment #700103 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f53242d94bc
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla21

Updated

4 years ago
Keywords: dev-doc-needed

Comment 29

4 years ago
(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!
https://hg.mozilla.org/mozilla-central/rev/5f53242d94bc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → -
Mentioned on https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers and
https://developer.mozilla.org/en-US/docs/DOM/window.location.
Keywords: dev-doc-needed → dev-doc-complete
(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.

Comment 35

4 years ago
(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.

Updated

4 years ago
Depends on: 878297
Depends on: 973401
See Also: → bug 996013
You need to log in before you can comment on or make changes to this bug.