Geoff Lankow (:darktrojan) Assignee
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: 

From bug 458299 comment 1:
> Perhaps we can make the service's methods polymorphically
> accept either a URI or a string representing the site.

myk, would the correct way to do this be to change the interface from nsIURI to nsIVariant, then throwing an exception if the argument is not string, nsIURI or null?

Reproducible: Always

	Myk Melez [:myk] [@mykmelez]
	Comment 1 • 15 years ago

(In reply to comment #0)
> From bug 458299 comment 1:
> > Perhaps we can make the service's methods polymorphically
> > accept either a URI or a string representing the site.
> 
> myk, would the correct way to do this be to change the interface from nsIURI to
> nsIVariant, then throwing an exception if the argument is not string, nsIURI or
> null?

That sounds reasonable to me, but cc:ing mconnor for confirmation on this API change and dolske to keep him apprised as it was his suggestion originally in bug 458299.

	Geoff Lankow (:darktrojan) Assignee
	Comment 2 • 15 years ago

Now that I've got the previous patch off my tree, here's what I've been working on.

	Myk Melez [:myk] [@mykmelez]
	Comment 3 • 15 years ago

Comment on attachment 396331 [details] [diff] [review]
patch v1

>diff --git a/toolkit/components/contentprefs/public/nsIContentPrefService.idl b/toolkit/components/contentprefs/public/nsIContentPrefService.idl

This IDL file should document that a string argument for an aURI parameter represents the group (i.e. site) to which the pref is restricted, while an nsIURI argument for that parameter represents the URI from which the service should derive the group.

In fact, we should probably change the name of these parameters to reflect this expanded use of them.  Techhnically, this value is the "group", and this parameter should be called "aGroup", even though the content pref service only derives site values from URIs.  Perhaps we can call it "aGroup" but document that it typically represents the site/hostname to which a preference applies.


>diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js

>+    else if (aURI.QueryInterface && aURI.QueryInterface(Ci.nsIURI)) {

Use |aURI instanceof Ci.nsIURI| instead.  It's not only simpler, but it also doesn't throw, whereas aURI.QueryInterface(Ci.nsIURI) will throw NS_ERROR_NO_INTERFACE (preventing your code from throwing NS_ERROR_ILLEGAL_VALUE afterwards) if aURI doesn't implement that interface.


>diff --git a/toolkit/components/contentprefs/tests/unit/test_stringURIs.js b/toolkit/components/contentprefs/tests/unit/test_stringURIs.js

I'm not sure how important this is, but nsIVariant supports a number of string types, and I wonder if they all result in string primitives whose typeof == "string" when passed through an XPCOM interface.  I also wonder if passing a JavaScript String object (whose typeof == "object") results in a primitive string on the other side of an XPCOM interface.  It might be worth testing both these things.

Otherwise, this looks great!

	Geoff Lankow (:darktrojan) Assignee
	Comment 4 • 15 years ago

(In reply to comment #3)
> >diff --git a/toolkit/components/contentprefs/tests/unit/test_stringURIs.js b/toolkit/components/contentprefs/tests/unit/test_stringURIs.js
> 
> I'm not sure how important this is, but nsIVariant supports a number of string
> types, and I wonder if they all result in string primitives whose typeof ==
> "string" when passed through an XPCOM interface.  I also wonder if passing a
> JavaScript String object (whose typeof == "object") results in a primitive
> string on the other side of an XPCOM interface.  It might be worth testing both
> these things.

You're right, it's still an object on the other side. Any ideas on how to check for that? I couldn't find one that worked.
Maybe I should give up on this approach and have an nsIURI version and an AString version of each method.

	Myk Melez [:myk] [@mykmelez]
	Comment 5 • 15 years ago

(In reply to comment #4)
> You're right, it's still an object on the other side. Any ideas on how to check
> for that?

You should be able to check for aURI.constructor.name == "String".  This works for primitive strings too, since JS will transparently convert them to String objects before evaluating the conditional.

Note: you can't test for aURI.constructor == String because the String constructor in one JS context isn't the same as the String constructor from another one.


> Maybe I should give up on this approach and have an nsIURI version and an
> AString version of each method.

That's a significantly more complex API; I'd prefer sticking with your current approach.

	Geoff Lankow (:darktrojan) Assignee
	Comment 6 • 15 years ago

Fixed up and tested String objects and renamed aURI to aGroup.

	Myk Melez [:myk] [@mykmelez]
	Comment 7 • 15 years ago

Comment on attachment 396913 [details] [diff] [review]
patch v2

>diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js
>+      return this._selectPref(aGroup.toString(), aName);

Nit: toString() is unnecessary, as JavaScript String objects can be used exactly the same way as string primitives (except for in "typeof" expressions).


>diff --git a/toolkit/components/contentprefs/tests/unit/test_stringURIs.js b/toolkit/components/contentprefs/tests/unit/test_stringURIs.js

Nit: this should be called test_stringGroups.js.

Otherwise this looks great and works fine in my testing.  r=myk and requesting sr from mconnor.

	Mike Connor [:mconnor]
	Comment 8 • 15 years ago

Comment on attachment 396913 [details] [diff] [review]
patch v2


> 
>-    if (aURI) {
>-      var group = this.grouper.group(aURI);
>+    if (aGroup == null) {
>+      return this._selectGlobalPref(aName);
>+    }
>+    else if (aGroup.constructor.name == "String") {
>+      return this._selectPref(aGroup.toString(), aName);
>+    }
>+    else if (aGroup instanceof Ci.nsIURI) {
>+      var group = this.grouper.group(aGroup);
>       return this._selectPref(group, aName);
>     }

else after return is wrong/useless, these should just be successive if statements, also, no braces around single lines please.

looks good otherwise.

	Geoff Lankow (:darktrojan) Assignee
	Comment 9 • 15 years ago

(In reply to comment #7)
> (From update of attachment 396913 [details] [diff] [review])
> >diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js
> >+      return this._selectPref(aGroup.toString(), aName);
> 
> Nit: toString() is unnecessary, as JavaScript String objects can be used
> exactly the same way as string primitives (except for in "typeof" expressions).

Without it, NS_ERROR_UNEXPECTED is thrown trying to assign an object as a statement parameter. I always thought the Storage API handled that okay, but apparently not.

	Geoff Lankow (:darktrojan) Assignee
	Comment 10 • 15 years ago

Now with less nits to pick.

	Myk Melez [:myk] [@mykmelez]
	Comment 11 • 15 years ago

Comment on attachment 397139 [details] [diff] [review]
patch v3

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 396913 [details] [diff] [review] [details])
> > >diff --git a/toolkit/components/contentprefs/src/nsContentPrefService.js b/toolkit/components/contentprefs/src/nsContentPrefService.js
> > >+      return this._selectPref(aGroup.toString(), aName);
> > 
> > Nit: toString() is unnecessary, as JavaScript String objects can be used
> > exactly the same way as string primitives (except for in "typeof" expressions).
> 
> Without it, NS_ERROR_UNEXPECTED is thrown trying to assign an object as a
> statement parameter. I always thought the Storage API handled that okay, but
> apparently not.

Ah, my bad. How bizarre. :-!

Note to committer: this is the patch you should be committing; mconnor's sr carries forward from the previous patch.

	Dão Gottwald [:dao]
	Comment 12 • 15 years ago

Landed (and forgot to update this bug), and backed out because of this:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1251490034.1251491325.29668.gz

	Geoff Lankow (:darktrojan) Assignee
	Comment 13 • 15 years ago

Found a hack in the private browsing code which is no longer necessary due to this patch anyway...

	Geoff Lankow (:darktrojan) Assignee
	Comment 14 • 15 years ago

Comment on attachment 397384 [details] [diff] [review]
patch v4

Ah sod, left some debug stuff in there.

	Geoff Lankow (:darktrojan) Assignee
	Comment 15 • 15 years ago

Sorry about the bugmail spam!

	Dão Gottwald [:dao]
	Comment 16 • 15 years ago

Comment on attachment 397385 [details] [diff] [review]
patch v4 again

>         // The service only cares about the host of the URI, so we don't need a
>         // full nsIURI object here.
>-        let uri = { host: names[i]};
>+        let uri = names[i];

The comment also needs to be removed or adjusted.

	Geoff Lankow (:darktrojan) Assignee
	Comment 17 • 15 years ago

So it does.

	Reed Loden [:reed]
	Comment 18 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/d5a29930742a

	Eric Shepherd [:sheppy]
	Comment 19 • 14 years ago

Documentation updated:

https://developer.mozilla.org/en/nsIContentPrefService