Bug 1345294 introduced nsPrefBranch::{get,set}StringPref(), which allowed the getting of utf8 strings from prefs, which previously required using nsISupportsString with {get,set}ComplexValue. That bug also converted most uses. This patch finishes the job. - It removes the nsISupportsString support. - It converts existing code that relied on the nsISupportsString. - It removes the lint that was set up to detect such uses of nsISupportsString.

Florian, I'm asking you to review because you did bug 1345294. Please let me know if there are any parts of this patch you feel uncomfortable reviewing.

Comment on attachment 8924768 [details] [diff] [review] Remove support for nsISupportsString values in nsPrefBranch::{get,set}ComplexValue() Review of attachment 8924768 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup :-) ::: netwerk/dns/nsIDNService.cpp @@ +103,5 @@ > > if (!pref || NS_LITERAL_STRING(NS_NET_PREF_IDNBLACKLIST).Equals(pref)) { > + nsAutoCString blacklist; > + nsresult rv = > + prefBranch->GetStringPref(NS_NET_PREF_IDNBLACKLIST, EmptyCString(), 0, blacklist); Could this use Preferences::GetString()? I didn't make these last 2 parameters optional for C++ callers because I was told in bug 1345294 comment 2 that C++ already had a different nicer API. ::: testing/marionette/client/marionette_driver/marionette.py @@ +867,5 @@ > Components.utils.import("resource://gre/modules/Preferences.jsm"); > Preferences.reset(arguments[0]); > """, script_args=(pref,)) > > + def get_pref(self, pref, default_branch=False, value_type="unspecified"): This results in a Components.interfaces["unspecified"] call, I didn't know this quietly returns undefined, nice :-). @@ +876,5 @@ > from the default branch. Otherwise the user-defined > value if set is returned. Defaults to `False`. > :param value_type: Optional, XPCOM interface of the pref's complex value. > + Possible values are: `nsIFile`, > + `nsIPrefLocalizedString`, `nsIRelativeFilePref`. I would not mention nsIRelativeFilePref here either. I also wonder if this is ever used with nsIFile or nsIPrefLocalizedString, given that the associated set_pref method doesn't support specifying a type. ::: toolkit/modules/Preferences.jsm @@ +59,5 @@ > case Ci.nsIPrefBranch.PREF_STRING: > + if (valueType && > + (valueType.toString() === "nsIFile" || > + valueType.toString() === "nsIPrefLocalizedString" || > + valueType.toString() === "nsIRelativeFilePref")) { I would prefer calling valueType.toString() only once with something like: ["nsIFoo1", "nsIFoo2", ...].includes(valueType.toString()) If I was writing this code, I would have used valueType.name rather than valueType.toString(). Do you know which of the two is better? AFAIK nsIRelativeFilePref is only used in mailnews/base/util/nsMsgIncomingServer.cpp and mailnews/base/util/nsMsgUtils.cpp in comm-central. I would prefer if we avoided introducing references to it from JS code shipping in Firefox, and I think eventually we should drop support for it in m-c and move the implementation to mailnews/. I'm actually not sure Preferences.jsm's get method has a single caller for the nsIFile or nsIPrefLocalizedString types either. It doesn't support setComplexValue, so I doubt the getComplexValue part was ever used for something that isn't nsISupportsString. Maybe we could just throw if valueType isn't null and see if there's any thing that breaks on try? Eventually I would like to remove Preferences.jsm, for now we only banned it from the startup path in bug 1357517. Also, are you considering eventually removing getComplexValue completely?

> > + nsresult rv = > > + prefBranch->GetStringPref(NS_NET_PREF_IDNBLACKLIST, EmptyCString(), 0, blacklist); > > Could this use Preferences::GetString()? No, unfortunately. That function gets prefs relative to the pref branch specified by prefBranch. Preferences::GetString() gets prefs relative to the root branch. > > :param value_type: Optional, XPCOM interface of the pref's complex value. > > + Possible values are: `nsIFile`, > > + `nsIPrefLocalizedString`, `nsIRelativeFilePref`. > > I would not mention nsIRelativeFilePref here either. Ok, I will remove it and in the other places. > I also wonder if this > is ever used with nsIFile or nsIPrefLocalizedString, given that the > associated set_pref method doesn't support specifying a type. I found one nsIPrefLocalizedString use here: http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/window.py#54-55 > > + (valueType.toString() === "nsIFile" || > > + valueType.toString() === "nsIPrefLocalizedString" || > > + valueType.toString() === "nsIRelativeFilePref")) { > > I would prefer calling valueType.toString() only once with something like: > ["nsIFoo1", "nsIFoo2", ...].includes(valueType.toString()) Ok. > If I was writing this code, I would have used valueType.name rather than > valueType.toString(). Do you know which of the two is better? I don't know, but I am far from an expert JS programmer, especially for Firefox chrome code, so I'm happy to switch to `name`. > I'm actually not sure Preferences.jsm's get method has a single caller for > the nsIFile or nsIPrefLocalizedString types either. It doesn't support > setComplexValue, so I doubt the getComplexValue part was ever used for > something that isn't nsISupportsString. Maybe we could just throw if > valueType isn't null and see if there's any thing that breaks on try? As mentioned above, there is that one nsIPrefLocalizedString case. > Also, are you considering eventually removing getComplexValue completely? It would be nice, but there are plenty of uses of it with nsIFile and nsIPrefLocalizedString so it seems like a lot of work.

Bug 1345294 introduced nsPrefBranch::{get,set}StringPref(), which allowed the getting of utf8 strings from prefs, which previously required using nsISupportsString with {get,set}ComplexValue. That bug also converted most uses. This patch finishes the job. - It removes the nsISupportsString support. - It converts existing code that relied on the nsISupportsString. - It removes the lint that was set up to detect such uses of nsISupportsString.

Comment on attachment 8925422 [details] [diff] [review] Remove support for nsISupportsString values in nsPrefBranch::{get,set}ComplexValue() Review of attachment 8925422 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/dns/nsIDNService.cpp @@ +103,5 @@ > > if (!pref || NS_LITERAL_STRING(NS_NET_PREF_IDNBLACKLIST).Equals(pref)) { > + nsAutoCString blacklist; > + nsresult rv = > + prefBranch->GetStringPref(NS_NET_PREF_IDNBLACKLIST, EmptyCString(), 0, blacklist); Unless I'm reading this C++ code wrong, it seems 'prefBranch' is always the root branch here, so I still think Preferences::GetString() would look nicer (and if converting the rest of the method too, then the 'prefBranch' parameter can be removed) ::: toolkit/modules/Preferences.jsm @@ +56,5 @@ > > Preferences._get = function(prefName, defaultValue, valueType) { > switch (this._prefBranch.getPrefType(prefName)) { > case Ci.nsIPrefBranch.PREF_STRING: > + if (valueType I would be surprised if this didn't cause a parse error. @@ +58,5 @@ > switch (this._prefBranch.getPrefType(prefName)) { > case Ci.nsIPrefBranch.PREF_STRING: > + if (valueType > + let ifaces = > + ["nsIFile", "nsIPrefLocalizedString", "nsIRelativeFilePref"]; Didn't you say you were removing nsIReleaveFilePref here? Or did you upload the wrong version of the patch?

(In reply to Nicholas Nethercote [:njn] from comment #3) > > I also wonder if this > > is ever used with nsIFile or nsIPrefLocalizedString, given that the > > associated set_pref method doesn't support specifying a type. > > I found one nsIPrefLocalizedString use here: > http://searchfox.org/mozilla-central/rev/ > 5a60492a53667fc61a62af1847d005a210b7a4f6/testing/marionette/puppeteer/ > firefox/firefox_puppeteer/ui/browser/window.py#54-55 Unfortunate to have to keep support only for a test file, but fair enough, changing this would be far out of the scope of this bug. > > If I was writing this code, I would have used valueType.name rather than > > valueType.toString(). Do you know which of the two is better? > > I don't know, but I am far from an expert JS programmer, especially for > Firefox chrome code, so I'm happy to switch to `name`. Thinking about this some more, I think the advantage of .toString() was that it would work both for interface objects and interface names provided as a string. I can't find any caller providing the interface as a string so I think .name is fine. > > Also, are you considering eventually removing getComplexValue completely? > > It would be nice, but there are plenty of uses of it with nsIFile and > nsIPrefLocalizedString so it seems like a lot of work. Right, it's still used, but like nsISupportsString, we could replace them with a script if we can put in place a better replacement API.

> Unless I'm reading this C++ code wrong, it seems 'prefBranch' is always the > root branch here, so I still think Preferences::GetString() would look nicer > (and if converting the rest of the method too, then the 'prefBranch' > parameter can be removed) prefBranch is a parameter of nsIDNService::prefsChanged(), which is called from two places. One of these two places is nsIDNService::Observe(), where the prefBranch is derived from the aSubject parameter when the aTopic is NS_PREFBRANCH_PREFCHANGE_TOPIC_ID. NS_PREFBRANCH_PREFCHANGE_TOPIC_ID occurs in lots of places, enough that I would prefer to be conservative and just assume that it might not be a root branch. > I would be surprised if this didn't cause a parse error. Indeed, my try push was very orange! :) > Didn't you say you were removing nsIReleaveFilePref here? Or did you upload > the wrong version of the patch? I thought you just wanted to remove it from the comments. But I can remove it here too.

This version fixes the syntax error and removes nsIRelativeFilePref support from Preferences.jsm. It leaves the GetStringPref() in nsIDNService.cpp unchanged because I am not convinced it's safe to change it.

(In reply to Nicholas Nethercote [:njn] from comment #7) > > Unless I'm reading this C++ code wrong, it seems 'prefBranch' is always the > > root branch here, so I still think Preferences::GetString() would look nicer > > (and if converting the rest of the method too, then the 'prefBranch' > > parameter can be removed) > > prefBranch is a parameter of nsIDNService::prefsChanged(), which is called > from two places. One of these two places is nsIDNService::Observe(), where > the prefBranch is derived from the aSubject parameter when the aTopic is > NS_PREFBRANCH_PREFCHANGE_TOPIC_ID. NS_PREFBRANCH_PREFCHANGE_TOPIC_ID occurs > in lots of places, enough that I would prefer to be conservative and just > assume that it might not be a root branch. I believe it's safe because: - that prefsChanged method calls Get*Pref with constants defined at the top of the file that seem to be relative to the root branch. - I think aSubject in a pref observer is the pref branch on which the observer was added. In this case the observer is added on a branch obtained by: nsCOMPtr<nsIPrefService> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); nsCOMPtr<nsIPrefBranch> prefInternal(do_QueryInterface(prefs));

> I believe it's safe You may well be right. But the existing patch is (more) obviously correct. I would prefer to be conservative, and not let this minor part of the patch block the rest of it. With that in mind, do you think the existing patch is acceptable?

(In reply to Nicholas Nethercote [:njn] from comment #10) > > I believe it's safe > > You may well be right. But the existing patch is (more) obviously correct. I > would prefer to be conservative, and not let this minor part of the patch > block the rest of it. With that in mind, do you think the existing patch is > acceptable? Yes, I wrote comment 9 and expected to finish the review a couple minutes later, but got very busy for the rest of the afternoon and didn't get time to come back to this review until now. Sorry if with comment 9 it looked like I was waiting for a change on the patch.

Great, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e843de356b7e5f3cb01f114bc101f4a71092550b Bug 1414096 - Remove support for nsISupportsString values in nsPrefBranch::{get,set}ComplexValue(). r=florian.

Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b69cb7c76bcd Backed out 1 changesets for dt1 failures "devtools/client/commandline/test/browser_cmd_pref3.js" r=backout on a CLOSED TREE

Where by "dt1" he meant "the Win7 debug non-e10s devtools tests that are the only place devtools commandline tests run since they're only enabled for !e10s which we only test on Win7 debug."

(In reply to Phil Ringnalda (:philor) from comment #16) > Where by "dt1" he meant "the Win7 debug non-e10s devtools tests that are the > only place devtools commandline tests run since they're only enabled for > !e10s which we only test on Win7 debug." Thank you for the info. I was about to ask why this hadn't shown up on my "[x64]" try push.

So I just missed a couple of cases under devtools/. They're easy fixes and I've confirmed locally that it fixes the failing tests.

https://hg.mozilla.org/integration/mozilla-inbound/rev/54791453291c21807e995e63807b965708419ff3 Bug 1414096 (attempt 2) - Remove support for nsISupportsString values in nsPrefBranch::{get,set}ComplexValue(). r=florian.

Now that the bug is resolved, can someone advise how to replace the following (Add-on) code: service.setComplexValue("QuickFolders.folders", Components.interfaces.nsISupportsString, str); service.getComplexValue("QuickFolders.folders", Components.interfaces.nsISupportsString).data;

(In reply to Axel Grude [:realRaven] from comment #22) > Now that the bug is resolved, can someone advise how to replace the > following (Add-on) code: > > service.setComplexValue("QuickFolders.folders", > Components.interfaces.nsISupportsString, str); > service.getComplexValue("QuickFolders.folders", > Components.interfaces.nsISupportsString).data; Sorry forgot to mention this question is for a Thunderbird (XPCOM XUL based) legacy extension.

(In reply to Axel Grude [:realRaven] from comment #22) > service.setComplexValue("QuickFolders.folders", > Components.interfaces.nsISupportsString, str); > service.getComplexValue("QuickFolders.folders", > Components.interfaces.nsISupportsString).data; There are heaps of examples in bug 1415567 comment #46, for example: https://hg.mozilla.org/comm-central/rev/3a0c2f6597ef#l1.12 We port all this stuff, TB is in a way a giant Firefox add-on ;-)

thanks. I understand I cannot use it with Ci.nsISupportsString, what about Ci.nsIPrefLocalizedString? Same remedy for now, replace with getStringPref / setStringPref?

nsIPrefLocalizedString is still supported and so you don't need to change the cases that use it.