WorkerNavigator does not implement NavigatorLanguage

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: nsm, Assigned: baku, Mentored)

Tracking

({dev-doc-complete})

unspecified
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Blocks: 925846
Whiteboard: [good first bug][mentor=nsm]

Comment 1

6 years ago
How is the language set when it isn't "en".

Comment 2

6 years ago
Hi Nikhil


Can I take this one too?
I tried something and here is the report:


After adding the 'implements' to WorkerNavigator.webidl

===============================================================================================
diff --git a/dom/webidl/WorkerNavigator.webidl b/dom/webidl/WorkerNavigator.webidl
--- a/dom/webidl/WorkerNavigator.webidl
+++ b/dom/webidl/WorkerNavigator.webidl
@@ -7,8 +7,9 @@ interface WorkerNavigator {
   readonly attribute DOMString appName;
   [Constant]
   readonly attribute DOMString appVersion;
   [Constant]
   readonly attribute DOMString platform;
   [Constant]
   readonly attribute DOMString userAgent;
 };
+WorkerNavigator implements NavigatorLanguage;
===============================================================================================

WebIDL binding requires a GetLanguage() function in dom/worker/Navigator.[h|cpp]

There is already a GetLanguage() implementation here(http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#351) which seems reasonable

So I did a little violent hack in dom/worker/RuntimeService.cpp to pass language string from dom/base/Navigator.cpp to dom/worker/Navigator.cpp

Finally I got a successful build.

Desperately need your further instructions.
Comment on attachment 817263 [details] [diff] [review]
Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage-draft.patch

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

Almost there. Also update the tests.

::: dom/webidl/WorkerNavigator.webidl
@@ +10,5 @@
>    [Constant]
>    readonly attribute DOMString platform;
>    [Constant]
>    readonly attribute DOMString userAgent;
>  };

Nit: Newline

::: dom/workers/Navigator.cpp
@@ +46,5 @@
> +
> +  const RuntimeService::NavigatorStrings& strings =
> +    rts->GetNavigatorStrings();
> +
> +  aLanguage = strings.mLanguage;

Make mLanguage a member of WorkerNavigator and set it in the constructor from WorkerNavigator::Create() just like the other properties. Then you can inline this.

::: dom/workers/Navigator.h
@@ +67,5 @@
>    void GetUserAgent(nsString& aUserAgent) const
>    {
>      aUserAgent = mUserAgent;
>    }
> +  void GetLanguage(nsAString& aLanguage) const;

Convention in this file is nsString&
Attachment #817263 - Flags: review?(nsm.nikhil) → review-

Comment 5

6 years ago
Attachment #817263 - Attachment is obsolete: true
Attachment #817680 - Flags: review?(nsm.nikhil)
Comment on attachment 817680 [details] [diff] [review]
Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage.patch

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

Everything looks good except the IDL change. r=me with that fixed.
Please ask for sr? from a DOM peer.

::: dom/webidl/WorkerNavigator.webidl
@@ +12,5 @@
>    readonly attribute DOMString platform;
>    [Constant]
>    readonly attribute DOMString userAgent;
> +  [Constant]
> +  readonly attribute DOMString language;

You were correct earlier with the 'implements NavigatorLanguage'. Is there a reason the IDL was changed in this update? I only meant to update the implementation itself.
Attachment #817680 - Flags: review?(nsm.nikhil) → review-

Comment 7

6 years ago
Comment on attachment 817680 [details] [diff] [review]
Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage.patch

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

::: dom/webidl/WorkerNavigator.webidl
@@ +12,5 @@
>    readonly attribute DOMString platform;
>    [Constant]
>    readonly attribute DOMString userAgent;
> +  [Constant]
> +  readonly attribute DOMString language;

Also note that language cannot be [Constant] since its value may change throughout the lifetime of a page/worker.
Ah... In that case, it can't be just a string in navigator strings. We'll need to ensure thread-safe calls to NS_GetNavigatorLanguage() from the WorkerNavigator. The easiest way out is for RuntimeService to hold the current language as a member, update it on the main thread in a thread-safe manner when it changes. Have WorkerNavigator read from RuntimeService on worker threads.

Ehsan, any observer topic that gets triggered on language change, or any other way to monitor for the change?
Flags: needinfo?(ehsan)

Comment 9

6 years ago
It's a pref change, so presumably we can listen for the changes on the pref and update the value somewhere that the worker can access it.
Flags: needinfo?(ehsan)
Please make sure to get review from a workers peer before landing this.
Bug 785656 does something similar for dump()

Comment 12

6 years ago
(In reply to Nikhil Marathe [:nsm] from comment #6)
> Comment on attachment 817680 [details] [diff] [review]
> Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage.patch
> 
> Review of attachment 817680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Everything looks good except the IDL change. r=me with that fixed.
> Please ask for sr? from a DOM peer.
> 
> ::: dom/webidl/WorkerNavigator.webidl
> @@ +12,5 @@
> >    readonly attribute DOMString platform;
> >    [Constant]
> >    readonly attribute DOMString userAgent;
> > +  [Constant]
> > +  readonly attribute DOMString language;
> 
> You were correct earlier with the 'implements NavigatorLanguage'. Is there a
> reason the IDL was changed in this update? I only meant to update the
> implementation itself.

Sorry about that, I misunderstood :(

Comment 13

6 years ago
(In reply to Nikhil Marathe [:nsm] from comment #11)
> Bug 785656 does something similar for dump()

OK, I am working on it.
I'll update my patch once your solution to Bug 785656 is confirmed.

Comment 14

6 years ago
Attachment #817680 - Attachment is obsolete: true
Attachment #827357 - Flags: review?(nsm.nikhil)
Comment on attachment 827357 [details] [diff] [review]
Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage.patch

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

This code is great, but if the user were to change languages, running workers should see the change when script yields bu g925437#c6 about the run to completion violation there. You'll want to add a runnable that updates each WorkerNavigator's copy of the language, and in the RuntimeService uses the BROADCAST_ALL_WORKERS macro to send that runnable.

::: dom/webidl/WorkerNavigator.webidl
@@ +11,5 @@
>    [Constant]
>    readonly attribute DOMString platform;
>    [Constant]
>    readonly attribute DOMString userAgent;
>  };

Nit: Newline here.

::: dom/workers/RuntimeService.h
@@ +244,5 @@
>    WorkersDumpEnabled();
>  
> +  const nsString&
> +  GetLanguage();
> +  

Nit: whitespace.
Attachment #827357 - Flags: review?(nsm.nikhil) → review-
(In reply to i from comment #14)
> Created attachment 827357 [details] [diff] [review]

Hi, i - are you still working on this?

Comment 17

5 years ago
(In reply to Mike Hoye [:mhoye] from comment #16)
> (In reply to i from comment #14)
> > Created attachment 827357 [details] [diff] [review]
> 
> Hi, i - are you still working on this?

No, not any more.

Updated

5 years ago
Whiteboard: [good first bug][mentor=nsm] → [good next bug][mentor=nsm]
Mentor: nsm.nikhil
Whiteboard: [good next bug][mentor=nsm] → [good next bug]
(Assignee)

Updated

5 years ago
Assignee: nobody → amarchesini
Whiteboard: [good next bug]
(Assignee)

Comment 18

5 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #827357 - Attachment is obsolete: true
Attachment #8482166 - Flags: review?(nsm.nikhil)
Comment on attachment 8482166 [details] [diff] [review]
patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +1561,5 @@
>      return true;
>    }
>  };
>  
> +class UpdateLanguagesRunnable MOZ_FINAL : public WorkerControlRunnable

Making this a WorkerControlRunnable means it will preempt running JS to run, so the value of navigator.languages could change in the middle of script execution.  That would violate run-to-completion semantics.
Attachment #8482166 - Flags: review?(nsm.nikhil) → review-
(Assignee)

Comment 20

5 years ago
Posted patch language.patch (obsolete) — Splinter Review
Attachment #8482166 - Attachment is obsolete: true
Attachment #8482340 - Flags: review?(khuey)
(Assignee)

Comment 21

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=dced842cdfb6
Waiting for a green result...
Attachment #8482340 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/61e85ba6ebfc
https://hg.mozilla.org/mozilla-central/rev/b2b0aa288a8c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.