Remove support for nsISupportsString values in nsPrefBranch::{get,set}ComplexValue()

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

2 years ago
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.
Assignee

Comment 1

2 years ago
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.
Attachment #8924768 - Flags: review?(florian)
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?
Attachment #8924768 - Flags: review?(florian) → feedback+
Assignee

Comment 3

2 years ago
> > +    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.
Assignee

Comment 4

2 years ago
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.
Attachment #8925422 - Flags: review?(florian)
Assignee

Updated

2 years ago
Attachment #8924768 - Attachment is obsolete: true
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.
Assignee

Updated

2 years ago
Blocks: 1408580
Assignee

Comment 7

2 years ago
> 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.
Assignee

Comment 8

2 years ago
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.
Attachment #8925837 - Flags: review?(florian)
Assignee

Updated

2 years ago
Attachment #8925422 - Attachment is obsolete: true
Attachment #8925422 - Flags: review?(florian)
(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));
Assignee

Comment 10

2 years ago
> 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?
Flags: needinfo?(florian)
Attachment #8925837 - Flags: review?(florian) → review+
(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.
Flags: needinfo?(florian)
Assignee

Comment 12

2 years ago
Great, thanks!
Assignee

Comment 13

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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e843de356b7e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1415567

Comment 15

2 years ago
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."
Status: RESOLVED → REOPENED
Flags: needinfo?(n.nethercote)
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Assignee

Comment 17

2 years ago
(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.
Flags: needinfo?(n.nethercote)
Assignee

Comment 18

2 years ago
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.
Duplicate of this bug: 1415376
Assignee

Comment 20

2 years ago
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.

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/54791453291c
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.

Comment 24

2 years ago
(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?
Assignee

Comment 26

2 years ago
nsIPrefLocalizedString is still supported and so you don't need to change the cases that use it.
You need to log in before you can comment on or make changes to this bug.