Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Description • 11 years ago

appCodename, product, taintEnabled() are missing.

	Tareq Khandaker Assignee
	Comment 1 • 11 years ago

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,

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 2 • 11 years ago

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

	Tareq Khandaker Assignee
	Comment 3 • 11 years ago

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 .

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 4 • 11 years ago

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.

	Tareq Khandaker Assignee
	Comment 5 • 11 years ago

I changed test_navigator.html to check for functions and pass them.

	Masatoshi Kimura [:emk]
	Comment 6 • 11 years ago

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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 11 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 11 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 9 • 11 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 10 • 11 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 11 • 11 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 13 • 11 years ago

Comment on attachment 818108 [details] [diff] [review]
IncompleteNavigatorID.patch

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

Great job! r=me.

Carrying forward sr=sicking

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 14 • 11 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 15 • 11 years ago

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.

	Tareq Khandaker Assignee
	Comment 16 • 11 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)

	Masatoshi Kimura [:emk]
	Comment 17 • 11 years ago

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.

	Tareq Khandaker Assignee
	Comment 18 • 11 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.

	Tareq Khandaker Assignee
	Comment 19 • 11 years ago

I forgot to set this earlier.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 20 • 11 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 21 • 11 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 22 • 11 years ago

If we can claim the spec is wrong, we shouldn't add these craps to WorkerNavigator in the first place.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 11 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 24 • 11 years ago

Thanks guys!

Tareq, I think we're good to go here once you attach a new patch :)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 25 • 11 years ago

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

	Tareq Khandaker Assignee
	Comment 26 • 11 years ago

I had to leave in TaintEnabled() in dom/base/Navigator.h or else it would fail to compile.

	Tareq Khandaker Assignee
	Comment 27 • 11 years ago

Are there any tests that I should run for this patch?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 28 • 11 years ago

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!

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 29 • 11 years ago

Comment on attachment 820767 [details] [diff] [review]
IncompleteNavigatorID.patch

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

r=me

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 31 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/bdd0b739b34d

	Jean-Yves Perrier [:teoli]
	Comment 32 • 10 years ago

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID.taintEnabled
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID.product
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID.appCodeName
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID
https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator
and
https://developer.mozilla.org/en-US/Firefox/Releases/28

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 8 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 6 years ago

> Note that this commit incorrectly dropped the [Throws] annotations from stuff

I will fix that in bug 1431846, I guess....