925849 - WorkerNavigator does not implement NavigatorLanguage

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#navigatorlanguage

Comment 1

[Tareq Khandaker]

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

Comment 2

[i]

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 3

[i]

Attachment: Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage-draft.patch

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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&

Comment 5

[i]

Attachment: Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage.patch

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 7

[(no longer active)]

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.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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?

Comment 9

[(no longer active)]

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.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Please make sure to get review from a workers peer before landing this.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Bug 785656 does something similar for dump()

Comment 12

[i]

(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

[i]

(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

[i]

Attachment: Bug-925849-WorkerNavigator-does-not-implement-NavigatorLanguage.patch

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 16

[Mike Hoye [:mhoye]]

(In reply to i from comment #14)
> Created attachment 827357 [details] [diff] [review]

Hi, i - are you still working on this?

Comment 17

[i]

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

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: language.patch

Comment 21

[Andrea Marchesini [:baku]]

Attachment: language.patch

https://tbpl.mozilla.org/?tree=Try&rev=dced842cdfb6
Waiting for a green result...

Comment 22

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e85ba6ebfc

Comment 23

[James Graham [:jgraham]]

Attachment: Update web-platform-tests metadata for changes to WorkerNavigator

Comment 24

[James Graham [:jgraham]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b0aa288a8c

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/61e85ba6ebfc
https://hg.mozilla.org/mozilla-central/rev/b2b0aa288a8c

Comment 26

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/35
https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage.language
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage.languages