location.origin is not available in workers

RESOLVED FIXED in mozilla29

Status

()

Core
DOM: Workers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

({dev-doc-complete})

unspecified
mozilla29
x86
Mac OS X
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c= p=2 s=2014.01.31 u=][systemsfe])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
The location object is available in workers as read-only access. All properties seem to be present except the origin property. Furthermore, the mdn page states,  "origin on all location objects" is supported on version 26+.

Being that origin is missing from location objects in workers, I think we should add it.

Actual:
When accessed from a worker, location.origin is undefined.

Expected:
When accessed from a worker, location.origin should contain the expected protocol and host as found in the location object.
(Assignee)

Updated

4 years ago
Blocks: 920752
(Assignee)

Comment 1

4 years ago
This was just added in the spec: https://github.com/whatwg/url/commit/0027a2ec9d30501081da97b82455e07e67e22e2b
(Assignee)

Comment 2

4 years ago
Created attachment 8366278 [details] [diff] [review]
Patch - Add location.origin to workers
(Assignee)

Updated

4 years ago
Attachment #8366278 - Flags: review?(amarchesini)
Comment on attachment 8366278 [details] [diff] [review]
Patch - Add location.origin to workers

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

Can you add a mochitest?
I would like to review this patch again with these changes.
Thanks.

::: dom/workers/WorkerPrivate.cpp
@@ +3450,5 @@
> +  origin.Append(mLocationInfo.mProtocol);
> +  origin.Append("//");
> +  origin.Append(mLocationInfo.mHost);
> +
> +  mLocationInfo.mOrigin.Assign(origin);

you should use mOrigin to the LocationInfo struct and use

  nsContentUtils::GetUTFNonNullOrigin(aBaseURI, mLocationInfo.mOrigin);

This doesn't work for all the origin. Think about blob:, file: etc etc.
Attachment #8366278 - Flags: review?(amarchesini) → review-
Keywords: dev-doc-needed
(Assignee)

Comment 4

4 years ago
Created attachment 8366362 [details] [diff] [review]
Patch - Add location.origin to workers

Hey Baku - thanks for the quick review. I've updated the patch.

One thing that's a bit new/odd to me is the casting. I've tried to follow examples elsewhere, and this seemed to be the most appropriate way to do it. Let me know if you think there is a better way, thanks!
Attachment #8366362 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Attachment #8366278 - Attachment is obsolete: true
Comment on attachment 8366362 [details] [diff] [review]
Patch - Add location.origin to workers

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

Can you provide a mochitest? There are several examples in dom/workers/test/
I would create a test_origin.html that creates a worker with file_origin.js and here you should check if the origin is == to what you expect it should be.

::: dom/workers/WorkerPrivate.cpp
@@ +3445,5 @@
>    else {
>      mLocationInfo.mHost.Assign(mLocationInfo.mHostname);
>    }
> +
> +  NS_ConvertUTF8toUTF16 origin(mLocationInfo.mOrigin);

remove this and use:

nsCString origin;
nsContentUtils::GetUTFNonNullorigin(aBaseURI, origin);
mLocationInfo.mOrigin = NS_ConvertUTF16toUTF8(origin);
Attachment #8366362 - Flags: review?(amarchesini)
(Assignee)

Comment 6

4 years ago
Created attachment 8366379 [details] [diff] [review]
Patch - Add location.origin to workers

(In reply to Andrea Marchesini (:baku) from comment #5)
> Comment on attachment 8366362 [details] [diff] [review]
> Patch - Add location.origin to workers
> 
> Review of attachment 8366362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you provide a mochitest? There are several examples in dom/workers/test/
> I would create a test_origin.html that creates a worker with file_origin.js
> and here you should check if the origin is == to what you expect it should
> be.
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3445,5 @@
> >    else {
> >      mLocationInfo.mHost.Assign(mLocationInfo.mHostname);
> >    }
> > +
> > +  NS_ConvertUTF8toUTF16 origin(mLocationInfo.mOrigin);
> 
> remove this and use:
> 
> nsCString origin;
> nsContentUtils::GetUTFNonNullorigin(aBaseURI, origin);
> mLocationInfo.mOrigin = NS_ConvertUTF16toUTF8(origin);

Hi Andrea, thanks for the quick review as always. I've had to change it a bit to use nsString origin, instead of nsCString - so please confirm that this is ok. (Get an error otherwise).

I have also made a small modification to the test_location mochitest as it seemed to be an appropriate place to test for origin. I am also going to run this on try to make sure everything passes there.
Attachment #8366379 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Whiteboard: [c= p=2 s= u=]
(Assignee)

Comment 7

4 years ago
On try: https://tbpl.mozilla.org/?tree=Try&rev=24b36ca08ab3
(Assignee)

Updated

4 years ago
Attachment #8366362 - Attachment is obsolete: true
Comment on attachment 8366379 [details] [diff] [review]
Patch - Add location.origin to workers

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

Bent, this patch looks good to me, but can you take a quick look?

::: dom/workers/WorkerPrivate.h
@@ +131,4 @@
>      nsCString mPathname;
>      nsCString mSearch;
>      nsCString mHash;
> +    nsCString mOrigin;

if this is a nsString, you don't hve to convert it twice. Change it to nsString and then do:

nsContentUtils::GetUTFNonNullOrigin(aBase, mLocation.mOrigin);

in WorkerPrivate.cpp.

And in Location.cpp, just use mOrigin instead NS_ConvertUTF8toUTF16(aInfo.mOrigin));
Attachment #8366379 - Flags: review?(bent.mozilla)
Attachment #8366379 - Flags: review?(amarchesini)
Attachment #8366379 - Flags: review+

Updated

4 years ago
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
(Assignee)

Comment 9

4 years ago
(In reply to Andrea Marchesini (:baku) from comment #8)
> Comment on attachment 8366379 [details] [diff] [review]
> Patch - Add location.origin to workers
> 
> Review of attachment 8366379 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> if this is a nsString, you don't hve to convert it twice. Change it to
> nsString and then do:
> 
> nsContentUtils::GetUTFNonNullOrigin(aBase, mLocation.mOrigin);
> 
> in WorkerPrivate.cpp.
> 
> And in Location.cpp, just use mOrigin instead
> NS_ConvertUTF8toUTF16(aInfo.mOrigin));


Sure, will update - thanks! I was trying to follow the existing conventions in WorkerPrivate.h.
(Assignee)

Comment 10

4 years ago
Created attachment 8366711 [details] [diff] [review]
Patch - Add location.origin to workers
Attachment #8366379 - Attachment is obsolete: true
Attachment #8366379 - Flags: review?(bent.mozilla)
Attachment #8366711 - Flags: review?(bent.mozilla)
Comment on attachment 8366711 [details] [diff] [review]
Patch - Add location.origin to workers

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

Looks good, thanks!
Attachment #8366711 - Flags: review?(bent.mozilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/00470cecfc56
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/00470cecfc56
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [c= p=2 s= u=] → [c= p=2 s=2014.01.31 u=]
Documentation updated:
https://developer.mozilla.org/en-US/docs/URLUtilsReadOnly.origin
https://developer.mozilla.org/en-US/docs/Web/API/URLUtilsReadOnly
https://developer.mozilla.org/en-US/docs/Web/API/WorkerLocation
and
https://developer.mozilla.org/en-US/Firefox/Releases/29
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [c= p=2 s=2014.01.31 u=] → [c= p=2 s=2014.01.31 u=][systemsfe]
You need to log in before you can comment on or make changes to this bug.