The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla14

Status

()

Toolkit
General
--
trivial
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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"
(Assignee)

Comment 2

5 years ago
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

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
So should I remove the code in the (!gPrefs) branch if any is found?

Comment 5

5 years ago
There isn't any (!gPrefs) branch in viewSource.js is there?
(Assignee)

Comment 6

5 years ago
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?

Comment 7

5 years ago
> 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.
(Assignee)

Comment 8

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

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
So why did anybody put the trys there in the first place?

And what about .set*Pref ? When does that fail?

Comment 11

5 years ago
> 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.
(Assignee)

Comment 12

5 years ago
Created attachment 608995 [details] [diff] [review]
patch
Attachment #608995 - Flags: feedback?(philip.chee)
(Assignee)

Comment 13

5 years ago
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 14

5 years ago
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-
(Assignee)

Comment 15

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

Comment 16

5 years ago
You can probably ask how to go about testing your patch in a comment in the bug or ask in #developers or both.
(Assignee)

Comment 17

5 years ago
If the error could be found just by opening View source then I must have somehow forgot to test that file.
(Assignee)

Comment 18

5 years ago
Created attachment 609384 [details] [diff] [review]
patch v2
Attachment #608995 - Attachment is obsolete: true
Attachment #609384 - Flags: feedback?(philip.chee)

Comment 19

5 years ago
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+
(Assignee)

Comment 20

5 years ago
I usually do not touch variable names, but yes, this one is only used once so is not that important to be descriptive :)
(Assignee)

Updated

5 years ago
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)

Comment 22

5 years ago
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.
(Assignee)

Comment 24

5 years ago
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).
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Comment 27

5 years ago
Created attachment 609824 [details] [diff] [review]
patch v3
Attachment #609384 - Attachment is obsolete: true
Attachment #609824 - Flags: review?(gavin.sharp)
Attachment #609824 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 30

5 years ago
Created attachment 610224 [details] [diff] [review]
patch v4
[Wrong patch]

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

Comment 32

5 years ago
Created attachment 610237 [details] [diff] [review]
patch v5
[Checked in: Comment 46]

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

Comment 34

5 years ago
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!
(Assignee)

Comment 36

5 years ago
Please do.
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=a4ef3e986f78
https://hg.mozilla.org/mozilla-central/rev/110452fddc2a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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 → ---
(Assignee)

Comment 40

5 years ago
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]
(Assignee)

Comment 44

5 years ago
What does it mean now? Is the patch OK, just that Ryan should try to push it again?
http://hg.mozilla.org/integration/mozilla-inbound/rev/76d72796ceee
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76d72796ceee
(Assignee)

Comment 47

5 years ago
So has this landed and did it stick? Forgot to mark RESOLVED?
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #610237 - Attachment description: patch v5 → patch v5 [Checked in: Comment 46]

Comment 50

5 years ago
> 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?

Comment 52

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 754573
(Assignee)

Comment 54

5 years ago
(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.