Closed Bug 738568 Opened 8 years ago Closed 8 years ago

remove Services.prefs.getBranch(null) occurrences in Toolkit

Categories

(Toolkit :: General, defect)

defect
Not set
trivial

Tracking

()

VERIFIED FIXED
mozilla14

People

(Reporter: aceman, Assigned: aceman)

References

()

Details

Attachments

(1 file, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #714606 +++

Philip Chee 2012-03-23 06:27:39 CET

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc16cd4bda12#l1.31
> -const gPrefBranch = gPrefService.getBranch(null);
> +const gPrefBranch = Services.prefs.getBranch(null);
Is getBranch(null) here really necessary? Nothing here sets a branch is there?
If not then gPrefBranch can be directly replaced by Services.prefs.
You could probably check and do all Toolkit:
http://mxr.mozilla.org/mozilla-central/search?string=getBranch(null)&case=1&find=%2Ftoolkit%2F
"Found 4 matching lines in 4 files"
OK, I'll do, when it is only so few occurrences.
Summary: remove getbranch(null) in toolkit/components/viewconfig/content/config.js → remove .getbranch(null) in Toolkit
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js#75
Might as well get rid of the try/catch block here. I don't think we can build toolkit without the preference service these days.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js#75
All subsequent tests for if |(gPrefs)| become unnecessary.
So should I remove the code in the (!gPrefs) branch if any is found?
There isn't any (!gPrefs) branch in viewSource.js is there?
There is one e.g. at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js#222 .

Also, is it necessary to keep try {} around .getBoolPref , .setBoolPref and the like? Can that throw?
> There is one e.g. at
Yeah you can get rid of that since that code will never be reached.

> Also, is it necessary to keep try {} around .getBoolPref , .setBoolPref and the like?
Depends. See below:

> Can that throw?
If the pref doesn't exist get*Pref() will throw.

So you check if defaults exist for those prefs:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#331
According to this, defaults exist so get*Pref() on these won't throw. So you can remove the try/catch block.

For Firefox you'd also need to check:
/browser/app/profile/firefox.js
and possibly other places.

In general it's best is to MXR the preference to see if the default is set anywhere.
OK, thanks. So I'll rather keep the try{}s as I think the  defaults from all.js could be accidentally removed.

I would think that if a pref doesn't exist it would return some value like false, "" or 0. Thanks for explaining this is not the case.
No you *want* things to fail spectacularly and non-silently if someone accidentally removes them from all.js to tell that person that his patch is full of FAIL.
So why did anybody put the trys there in the first place?

And what about .set*Pref ? When does that fail?
> So why did anybody put the trys there in the first place?
If you do some CVS archaeology you'll find that these bits of code date back to 2002/2003. Back then things were very wild and they might not have shaken all the bugs out of the preference service yet. And then newer coders start cargo culting bits of badly written code years later.

> And what about .set*Pref ? When does that fail?
That should never fail. These days that is.
Attached patch patch (obsolete) — Splinter Review
Attachment #608995 - Flags: feedback?(philip.chee)
I did not touch browser.xml because I am not familiar enough with those xml files.
Status: NEW → ASSIGNED
Summary: remove .getbranch(null) in Toolkit → remove Services.prefs.getBranch(null) occurrences in Toolkit
Comment on attachment 608995 [details] [diff] [review]
patch

Tested config.js by opening about:config.
OK.

Tested viewSource.js by opening view source (CTRL-U) while browsing to http://www.mozilla.org/
Not OK.

Error: syntax error
Source file: chrome://global/content/viewSource.js
Line: 38
Source code:
}

https://bugzilla.mozilla.org/attachment.cgi?id=608995&action=diff#a/toolkit/components/viewsource/content/viewSource.js_sec2

Stray closing parenthesis on line 80.

Tested alert.js using the following code run in the chrome context.
OK.

Components.classes["@mozilla.org/alerts-service;1"]
          .getService(Components.interfaces.nsIAlertsService)
          .showAlertNotification(null, "Notification test",
                                 "Surprise! I'm here to test notifications!",
                                 false, "foobarcookie", null);

Please fix the syntax error in viewSource.js. Also please test your patches before submitting them. Thanks.
Attachment #608995 - Flags: feedback?(philip.chee) → feedback-
Thanks.
Of course I test patches before submitting but sometimes I can't as I do not know what the code does and how to reach it. (So maybe I should do such kind of patches :() Unfortunately the Javascript errors are found only when code flow reaches the buggy lines. There should be some syntax checker/offline compiler for JS as there is for C.
You can probably ask how to go about testing your patch in a comment in the bug or ask in #developers or both.
If the error could be found just by opening View source then I must have somehow forgot to test that file.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #608995 - Attachment is obsolete: true
Attachment #609384 - Flags: feedback?(philip.chee)
Comment on attachment 609384 [details] [diff] [review]
patch v2

> +  var wraplonglinesPrefValue = Services.prefs
> +    .getBoolPref("view_source.wrap_long_lines");

If you are just trying to avoid lines over 80 chars you could just do:

  var wrap = Services.prefs
                     .getBoolPref("view_source.wrap_long_lines");
  if (wrap)
    document.getElementById("menu_wrapLongLines").setAttribute("checked", "true");
Attachment #609384 - Flags: feedback?(philip.chee) → feedback+
I usually do not touch variable names, but yes, this one is only used once so is not that important to be descriptive :)
Attachment #609384 - Flags: review?(gavin.sharp)
Comment on attachment 609384 [details] [diff] [review]
patch v2

>diff --git a/toolkit/components/viewconfig/content/config.js b/toolkit/components/viewconfig/content/config.js

>-const gPrefBranch = Services.prefs.getBranch(null);

Can you just change this to:

const gPrefBranch = Services.prefs;

and leave the rest of the file as-is? There isn't much benefit to getting rid of gPrefBranch, and it makes the patch much easier to review.

The rest of the changes look fine.
Attachment #609384 - Flags: review?(gavin.sharp)
Services.prefs is a lazy service getter. I suppose we could initialize gPrefBranch in onConfigLoad() (just before |var showWarning = gPrefBranch...|)
The prefs service is used very early in gecko startup (by Gecko C++ callers, by JS startup components, and then also on load of the main browser chrome window). The Services.jsm getter is guaranteed to already have been called by the time this code runs, and even if it hadn't, the cost of a JS getService call for an already-instantiated service is negligible, and not something that needs to be optimized.
Now this is interesting. In TB they always want me to remove the global variable and inline Services.*. even when it would be nicer to preserve it.

The whole point of this bug, as filed, was to remove the variable.
So this isn't much fun...
It's a judgement call. The difference in terms of performance or legibility of using a local variable vs. just always referring to Services.foo directly are negligible. When there are few references and no potential to significantly hurt add-on compatibility (as in alert.js in your patch), it makes sense to just always use Services.foo directly. When there are many changes that are annoying to review, you might as well just minimize the patch size and make the minimal change (as I asked you to do in config.js).
Yes, and it also annoys me to do changes that are then rejected just due to conflicting styles among Moz components. I will need to stick to one component where I can remember the passable style correctly ;)

So let's finish this, I'll send the patch shortly.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #609384 - Attachment is obsolete: true
Attachment #609824 - Flags: review?(gavin.sharp)
Attachment #609824 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/110452fddc2a
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Backed out, since you forgot to update viewPartialSource.js (which also uses gPrefs).

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba0c586cd88
Attached patch patch v4 [Wrong patch] (obsolete) — Splinter Review
Thanks for the catch, hopefully fixed now.
Attachment #609824 - Attachment is obsolete: true
Attachment #610224 - Flags: review?(gavin.sharp)
Comment on attachment 610224 [details] [diff] [review]
patch v4
[Wrong patch]

this looks like the wrong patch!
Attachment #610224 - Attachment is obsolete: true
Attachment #610224 - Flags: review?(gavin.sharp)
Really, what's with me today...

I've removed one temp variable in viewPartialSource.js, please check if the logic is OK.
Attachment #610237 - Flags: review?(gavin.sharp)
Comment on attachment 610237 [details] [diff] [review]
patch v5
[Checked in: Comment 46]

Have you tested all of the functionality that this patch touches?
Attachment #610237 - Flags: review?(gavin.sharp) → review+
I have tried to do so manually. I can't run Firefox/toolkit tests so far. Can you do that?
I meant manually, yeah. I can push to try!
Please do.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/110452fddc2a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whoops, was backed out by:
https://hg.mozilla.org/mozilla-central/rev/6ba0c586cd88
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla14 → ---
Wait, that backout is old, see comment 29.

This is a new patch. But see if the Try frm comment 37 succeeded.
Ed is just noting the inbound->central merge - since it was checked in and then backed out of inbound, the recent merge of inbound to central means that it looked like it was checked in and then backed out of central. He just didn't notice the backout on inbound before commenting - feel free to ignore comments 38/39.

It's ready to be checked back in, via inbound or otherwise, once we get the full results from try.
Looks good on Try, and I say that existing tests cover this bug pretty well :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb85ff0f764
Flags: in-testsuite- → in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Alas, you got caught up in a bustage sweep, because yours was about the only thing in that too-many-unrelated-things-at-once push which had a clear clean recent try.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb679e5939b9, better luck and better push-neighbors next time.
Keywords: checkin-needed
Attachment #610224 - Attachment description: patch v4 → patch v4 [Wrong patch]
What does it mean now? Is the patch OK, just that Ryan should try to push it again?
So has this landed and did it stick? Forgot to mark RESOLVED?
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(In reply to :aceman from comment #13)
> I did not touch browser.xml because I am not familiar enough with those xml
> files.

mPrefs is used within browser.xml only and doesn't deal with pref branches.
You should be able to do something like
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2618
just using Services.prefs, maybe even getting rid of the field.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please file a new bug for additional work.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Attachment #610237 - Attachment description: patch v5 → patch v5 [Checked in: Comment 46]
> just using Services.prefs, maybe even getting rid of the field.
How do we know that all consumers of <browser> also import Services into the scope? I know that lots of extensions use <browser> elements in their own UI/dialogs/windows. If you introduce a dependency on Services.jsm, then a dev-doc will definitely be needed.

OTOH. You could import Services into the scope of the bound element in the constructor. This will be transparent to consumers but a bit of overkill.
(In reply to Philip Chee from comment #50)
> > just using Services.prefs, maybe even getting rid of the field.
> 
> OTOH. You could import Services into the scope of the bound element in the
> constructor. This will be transparent to consumers but a bit of overkill.

Right, this file doesn't use Services.jsm.

459       <field name="mPrefs" readonly="true">
460         Components.classes['@mozilla.org/preferences-service;1']
461                   .getService(Components.interfaces.nsIPrefService)
462                   .getBranch(null);
463       </field>

Then just get rid of the ".getBranch(null)" part.

***

254       <property name="preferences"
255                 onget="return Components.classes['@mozilla.org/preferences-service;1'].getService(Components.interfaces.nsIPrefService);"
256                 readonly="true"/>

I'm not sure why there are 2 nsIPrefService services in this file.
Maybe one could be removed or at least use the other one?
Both came from Hewitt's initial checkin for Bug 165955 - Check in Satchel (the successor to wallet). It's probably an accidental (manual) merge between Phoenix and the XPFE browser bindings. According to MXR the preferences property isn't used anywhere.
(In reply to Serge Gautherie (:sgautherie) from comment #51)
> 459       <field name="mPrefs" readonly="true">
> 460         Components.classes['@mozilla.org/preferences-service;1']
> 461                   .getService(Components.interfaces.nsIPrefService)
> 462                   .getBranch(null);
> 
> Then just get rid of the ".getBranch(null)" part.

You also need to change the nsIPrefService to nsIPrefBranch.
Blocks: 754573
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #49)
> Please file a new bug for additional work.

Filed bug 754573 for the work according to the useful comments since comment 48.
No longer blocks: 754573
Depends on: 754573
V.Fixed, per MXR search.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.