WorkerNavigator does not implement all of NavigatorID

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: nsm, Assigned: tareqakhandaker)

Tracking

({dev-doc-complete})

unspecified
mozilla28
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

appCodename, product, taintEnabled() are missing.
Blocks: 925846
Whiteboard: [good first bug][mentor=nsm]
(Assignee)

Comment 1

6 years ago
Posted patch IncompleteNavigatorID.patch (obsolete) — Splinter Review
I have a few questions:

1. I changed dom/webidl/WorkerNavigator.webidl. Does dom/webidl/Navigator.webidl need to be changed as well?

2. I didn't know how to create a literal nsString, so I used NS_LITERAL_STRING. Is this the right thing to use?

3. I compiled this patch without any errors. Are there any tests that I need to run too?

Thanks,
Assignee: nobody → tareqakhandaker
Attachment #816428 - Flags: review?(nsm.nikhil)
Comment on attachment 816428 [details] [diff] [review]
IncompleteNavigatorID.patch

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

Looks good. The test you've to modify is dom/workers/test/navigator_worker.js. Please update the patch with the test.

Looking more into this code, it seems the main thread (dom/webidl/Navigator.webidl) code can be modified to reflect the current standard, moving these attributes into NavigatorID, after which WorkerNavigator would just 'implement NavigatorID'. Do you want to try that?

::: dom/workers/Navigator.h
@@ +53,5 @@
>    }
>  
> +  void GetAppCodeName(nsString& aAppCodeName) const
> +  {
> +    aAppCodeName = NS_LITERAL_STRING("Mozilla");

Use aAppCodeName.AssignLiteral("Mozilla")

@@ +69,5 @@
>      aPlatform = mPlatform;
>    }
> +  void GetProduct(nsString& aProduct) const
> +  {
> +    aProduct = NS_LITERAL_STRING("Gecko");

AssignLiteral()
Attachment #816428 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 3

6 years ago
Posted patch IncompleteNavigatorID.patch (obsolete) — Splinter Review
I changed the patch to reflect your comments.

Running this test ./mach mochitest-plain dom/workers/test/test_navigator.html

results in a failure.
Passed: 6
Failed: 1
Todo: 0
failed | Mismatched navigator string for taintEnabled! - got function taintEnabled() { [native code] }, expected undefined

I looked at the following file, https://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_navigator.html?force=1 and I found the line causing the error, but I don't know what to do about it. From what I understand, the taintEnabled() function gets mangled by JSON.stringify so that undefined is what gets passed as args.value .
Attachment #816428 - Attachment is obsolete: true
Attachment #817514 - Flags: review?(nsm.nikhil)
Comment on attachment 817514 [details] [diff] [review]
IncompleteNavigatorID.patch

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

This patch looks good, but please update the test and ask for review on that.

You'll also need super review from a DOM peer since this exposes more attributes to the web. I'm worried that exposing taintEnabled() on workers when we've wanted to get rid of it for a long time (bug 679971) may not be the best move.

::: dom/webidl/Navigator.webidl
@@ +46,5 @@
>    // Spec has this as a const, but that's wrong because it should not
>    // be on the interface object.
>    //const DOMString product = "Gecko"; // for historical reasons
> +  [Constant]
> +  readonly attribute DOMString product; // constant "Gecko"

Nit: Add newline here.

::: dom/webidl/WorkerNavigator.webidl
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  interface WorkerNavigator {
> +  // objects implementing this interface also implement the interfaces given below

No need for this comment.

::: dom/workers/test/navigator_worker.js
@@ +7,5 @@
>    "appName",
>    "appVersion",
>    "platform",
> +  "product",
> +  "taintEnabled",

Since taintEnabled is a function, you'll have to write custom code in the worker and html page to handle it.
Attachment #817514 - Flags: superreview?(jonas)
Attachment #817514 - Flags: review?(nsm.nikhil)
Attachment #817514 - Flags: review-
(Assignee)

Comment 5

6 years ago
Posted patch IncompleteNavigatorID.patch (obsolete) — Splinter Review
I changed test_navigator.html to check for functions and pass them.
Attachment #817514 - Attachment is obsolete: true
Attachment #817514 - Flags: superreview?(jonas)
Attachment #817571 - Flags: superreview?(jonas)
Attachment #817571 - Flags: review?(nsm.nikhil)
Do we really need to implement these craps only to comply the spec? We couldn't remove them from the main-thead Navigator because it will break the web. But why do we have to bring them to workers?
Could someone ask Hixie why these were added to workers?

I suspect there might be good reason to add these properties to workers too since it could enable running the same script libraries in workers as are used on the main thread. However I could also believe that those libraries don't work in workers anyway since there is no 'window' or 'document' properties available in the global scope.

I.e. most main-thread libraries doesn't work in workers out-of-the-box anyway. So it's work asking if these were added intentionally or by accident.
Comment on attachment 817571 [details] [diff] [review]
IncompleteNavigatorID.patch

Come to think of it, I'm actually fine with doing this.

Keeping these properties on the navigator object is extremely low cost. It doesn't seem worth the trouble of trying to get rid of them from the web platform.

I.e. I'm not sure it's worth our time to investigate if there are libraries that benefit from this patch. Nor am I sure it's worth asking developers to spend time on removing it from their code.

At some point in the future we can try to add warnings whenever these properties are accessed. And slowly phase them out that way. But in the meantime keeping things consistent seems like the simplest thing.

If someone wants to do the work of trying to keep these properties out, I'm fine with that too though.
Attachment #817571 - Flags: superreview?(jonas) → superreview+
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #8)
> Keeping these properties on the navigator object is extremely low cost. It
> doesn't seem worth the trouble of trying to get rid of them from the web
> platform.

I'm not proposing to remove these properties. These properties are not present on WorkerNavigator from the start (at least on our implementation).

> Nor am I sure it's worth asking developers to
> spend time on removing it from their code.

No developers have to remove it from their code because the properties are not present atm.

> At some point in the future we can try to add warnings whenever these
> properties are accessed. And slowly phase them out that way. But in the
> meantime keeping things consistent seems like the simplest thing.
> 
> If someone wants to do the work of trying to keep these properties out, I'm
> fine with that too though.

I bet we can't remove them forever once I added them to WorkerNavigator. So I'm against adding them unless we have strong reason to do so.

> I.e. I'm not sure it's worth our time to investigate if there are libraries that benefit from this patch.

And it looks like we have no strong reason.
What I'm saying is that the lack of these properties might currently be preventing authors from taking existing libraries and running them in workers.

I also don't think it's true that once we add them to WorkerNavigator we can't ever get rid of them. I think these properties will be much easier to get rid of once they are consistently implemented on a majority of browsers, since at that point there is no reason for anyone to use them as they always return the same values.

However if you want to raise with the spec author that these properties should only be exposed in a Window context and not in a Worker context them I definitely support that. But we shouldn't simply silently ignore the spec.
Comment on attachment 817571 [details] [diff] [review]
IncompleteNavigatorID.patch

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

::: dom/workers/test/test_navigator.html
@@ +36,5 @@
>      }
>  
> +    if (typeof args.value == "undefined" && typeof navigator[args.name] ==
> +        "function") {
> +      ok(true, "Function " + args.name + " found.");

taintEnabled() should be called and checked that it returns the same value in both navigators. For this you'll need to modify navigator_worker.js too. As a precaution, also check that it does indeed return false. Since it is the only function (and HTML standards don't change often), just compare args.name === "taintEnabled" rather than making it generic.
Attachment #817571 - Flags: review?(nsm.nikhil) → review-
(Assignee)

Comment 12

6 years ago
Posted patch IncompleteNavigatorID.patch (obsolete) — Splinter Review
Attachment #817571 - Attachment is obsolete: true
Attachment #818108 - Flags: review?(nsm.nikhil)
(Assignee)

Updated

6 years ago
Attachment #818108 - Attachment is patch: true
Attachment #818108 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 818108 [details] [diff] [review]
IncompleteNavigatorID.patch

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

Great job! r=me.

Carrying forward sr=sicking
Attachment #818108 - Flags: superreview+
Attachment #818108 - Flags: review?(nsm.nikhil)
Attachment #818108 - Flags: review+
Attachment #818108 - Flags: checkin+
Comment on attachment 818108 [details] [diff] [review]
IncompleteNavigatorID.patch

On second thought although this is fine, I'm not a Worker peer. Bent, once over please?
Attachment #818108 - Flags: review?(bent.mozilla)
Attachment #818108 - Flags: review+
Attachment #818108 - Flags: checkin+
Comment on attachment 818108 [details] [diff] [review]
IncompleteNavigatorID.patch

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

This looks really good! I found a few things here that will need a little work before this can land, however. Please feel free to ping me if you need any help here.

::: dom/webidl/Navigator.webidl
@@ +32,5 @@
>  
>  [NoInterfaceObject]
>  interface NavigatorID {
> +  [Constant]
> +  readonly attribute DOMString appCodeName; // constant "Mozilla"

Nit: Why didn't you preserve the comment about which engines support this property? I think you should keep it.

@@ +38,2 @@
>    readonly attribute DOMString appName;
> +  [Constant]

Removing [Throws] changes the signature of the method that the bindings will call. It used to call this one:

  http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.h#156

Now it will be calling this one:

  http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#326

That's ok, I guess, but it may mean that the old methods are now unused. Please check and remove them if so.

And we should file a followup to make the bindings call non-virtual methods for these constants.

@@ +44,5 @@
>    readonly attribute DOMString userAgent;
>  
>    // Spec has this as a const, but that's wrong because it should not
>    // be on the interface object.
>    //const DOMString product = "Gecko"; // for historical reasons

You need to do something with this block. And is that comment right? If this shouldn't be on the interface object then we have problems.

@@ +48,5 @@
>    //const DOMString product = "Gecko"; // for historical reasons
> +
> +  [Constant]
> +  readonly attribute DOMString product; // constant "Gecko"
> +  boolean taintEnabled(); // constant false

Nit: Why didn't you preserve the comment about which engines support this function? I think you should keep it.

::: dom/workers/Navigator.h
@@ +71,5 @@
> +  void GetProduct(nsString& aProduct) const
> +  {
> +    aProduct.AssignLiteral("Gecko");
> +  }
> +  bool TaintEnabled()

This should be a const method too.
Attachment #818108 - Flags: review?(bent.mozilla) → review-
(Assignee)

Comment 16

6 years ago
I will change the [Throws] and [Constants] back to the way they were. Maybe appName, product, and taintEnabled() should be [Throws] as well? I will also put the engine comments back. I am not sure what to do with that comment block besides deleting it. Maybe it is no longer relevant? (http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#client-identification)
appName, product, and taintEnabled() are dummy methods which always return the fixed value. I don't understand why we can't make them [Constant] and removing [Throw] from them. And it is the reason I was against adding them to workers. These properties will not bring any value at all.
(Assignee)

Comment 18

6 years ago
Currently, appName is neither [Throws] or [Constant] (https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#35), and appCodeName is [Throws] (https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#140). I tried putting [Throws, Constant] once before, but that didn't work.
(Assignee)

Comment 19

6 years ago
I forgot to set this earlier.
Flags: needinfo?(bent.mozilla)
(In reply to Tareq Khandaker from comment #16)
> I will change the [Throws] and [Constants] back to the way they were.

Oh, no, sorry. I am fine with them being [Constant] now, I just want to make sure that we aren't leaving unused methods lying around, and I want to eventually make sure we switch to using non-virtual functions (in a followup). If the methods can't throw (and these can't) then we should not use [Throws].

> I am not sure what to do with that comment
> block besides deleting it. Maybe it is no longer relevant?
> (http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.
> html#client-identification)

It looks that way to me too. However, since bz added this comment (and sicking reviewed) in bug 838146 I think it would be wise to get their input here before proceeding.

(In reply to Tareq Khandaker from comment #18)
> Currently, appName is neither [Throws] or [Constant]

It should just be [Constant] now.
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bent.mozilla)
We should not put .product on the interface object. So using

...
const DOMString product = "Gecko";
...


in the webidl would be wrong. But since that's not what we're doing it sounds like we're fine. If the spec still says to use "const DOMString product..." then feel free to leave a comment saying that the spec is wrong and that what we're doing is intentional.
Flags: needinfo?(jonas)
If we can claim the spec is wrong, we shouldn't add these craps to WorkerNavigator in the first place.
Looks like the spec's IDL has gotten fixed.  It now has:

  readonly attribute DOMString product; // constant "Gecko"

and prose that says:

  product
    Must return the string "Gecko".

so the comment about how the spec is wrong can go away, since the spec has been fixed.
Flags: needinfo?(bzbarsky)
Thanks guys!

Tareq, I think we're good to go here once you attach a new patch :)
(In reply to Masatoshi Kimura [:emk] from comment #22)
> If we can claim the spec is wrong, we shouldn't add these craps to
> WorkerNavigator in the first place.

We can *always* push back against a spec that we disagree with. I tried to say in comment 10 that if you want to push back against the spec here please do go ahead.

I just think that in this instance, it'll be more work, both for us and for other browser/spec authors to try to find the absolute minimum set of properties to expose here. And the cost of supporting these properties is very low.

Additionally, given how utterly useless these properties are, I think that with time they will be pretty easy to deprecate in the future. Once almost all users are using browsers which conform to the current spec, there's simply no reason for authors to use these properties. And so with a simple nudge in the form of a warning, we can most likely get rid of them pretty quickly.

That said, you should absolutely feel free to bring this up on the whatwg list yourself. I just don't think it's worth my time to do so. I don't think it's worth your time either, but clearly that is your decision.
(Assignee)

Comment 26

6 years ago
I had to leave in TaintEnabled() in dom/base/Navigator.h or else it would fail to compile.
Attachment #818108 - Attachment is obsolete: true
Attachment #820767 - Flags: superreview?(bent.mozilla)
Attachment #820767 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 27

6 years ago
Are there any tests that I should run for this patch?
Comment on attachment 820767 [details] [diff] [review]
IncompleteNavigatorID.patch

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

Thanks for the patch Tareq. r=me.

A couple of tips for future contributions:
If you haven't made major implementation changes to a patch after somebody gave you a r+, you can carry that r+ forward yourself.
Similarly, after receiving a sr+ for the over-arching story of your patch, you can carry it forward. Of course, in both cases it is better to err on the side of caution :)
For example, for DOM API modifications, if the WebIDL hasn't changed after the sr+ (or has changed keeping in line with what the reviewer recommended), you can carry it forward.

The patch one-line description should state what was fixed, and not what the problem was, so "Bug 925847 - WorkerNavigator implements NavigatorID".
That way, when a bug has multiple patches, each one's intention clearly shows up in |hg log| & co.

Please update your Bugzilla display name to have your IRC handle.

Cheers!
Attachment #820767 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 820767 [details] [diff] [review]
IncompleteNavigatorID.patch

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

r=me
Attachment #820767 - Flags: superreview?(bent.mozilla) → review+
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=nsm]
https://hg.mozilla.org/mozilla-central/rev/bdd0b739b34d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Note that this commit incorrectly dropped the [Throws] annotations from stuff, thus causing it to silently eat exceptions instead of propagating them out....  This was even discussed during the review, but there is no explanation here for _why_ we'd want to make such a behavior change!
> Note that this commit incorrectly dropped the [Throws] annotations from stuff

I will fix that in bug 1431846, I guess....
You need to log in before you can comment on or make changes to this bug.