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

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[:aceman]

+++ 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.

Comment 1

[Serge Gautherie (:sgautherie)]

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"

Comment 2

[:aceman]

OK, I'll do, when it is only so few occurrences.

Comment 3

[Philip Chee]

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.

Comment 4

[:aceman]

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

Comment 5

[Philip Chee]

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

Comment 6

[:aceman]

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

[Philip Chee]

> 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.

Comment 8

[:aceman]

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

[Philip Chee]

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.

Comment 10

[:aceman]

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

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

Comment 11

[Philip Chee]

> 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.

Comment 12

[:aceman]

Attachment: patch

Comment 13

[:aceman]

I did not touch browser.xml because I am not familiar enough with those xml files.

Comment 14

[Philip Chee]

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.

Comment 15

[:aceman]

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

[Philip Chee]

You can probably ask how to go about testing your patch in a comment in the bug or ask in #developers or both.

Comment 17

[:aceman]

If the error could be found just by opening View source then I must have somehow forgot to test that file.

Comment 18

[:aceman]

Attachment: patch v2

Comment 19

[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");

Comment 20

[:aceman]

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

Comment 21

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 22

[Philip Chee]

Services.prefs is a lazy service getter. I suppose we could initialize gPrefBranch in onConfigLoad() (just before |var showWarning = gPrefBranch...|)

Comment 23

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 24

[:aceman]

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...

Comment 25

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 26

[:aceman]

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.

Comment 27

[:aceman]

Attachment: patch v3

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/110452fddc2a

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Backed out, since you forgot to update viewPartialSource.js (which also uses gPrefs).

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba0c586cd88

Comment 30

[:aceman]

Attachment: patch v4 [Wrong patch]

Thanks for the catch, hopefully fixed now.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 610224 [details] [diff] [review]
patch v4
[Wrong patch]

this looks like the wrong patch!

Comment 32

[:aceman]

Attachment: 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.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 610237 [details] [diff] [review]
patch v5
[Checked in: Comment 46]

Have you tested all of the functionality that this patch touches?

Comment 34

[:aceman]

I have tried to do so manually. I can't run Firefox/toolkit tests so far. Can you do that?

Comment 35

[:Gavin Sharp [email: [e-mail]]]

I meant manually, yeah. I can push to try!

Comment 36

[:aceman]

Please do.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=a4ef3e986f78

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/110452fddc2a

Comment 39

[Ed Morley [:emorley]]

Whoops, was backed out by:
https://hg.mozilla.org/mozilla-central/rev/6ba0c586cd88

Comment 40

[:aceman]

Wait, that backout is old, see comment 29.

This is a new patch. But see if the Try frm comment 37 succeeded.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Looks good on Try, and I say that existing tests cover this bug pretty well :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb85ff0f764

Comment 43

[Phil Ringnalda (:philor)]

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.

Comment 44

[:aceman]

What does it mean now? Is the patch OK, just that Ryan should try to push it again?

Comment 45

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/76d72796ceee

Comment 46

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/76d72796ceee

Comment 47

[:aceman]

So has this landed and did it stick? Forgot to mark RESOLVED?

Comment 48

[Serge Gautherie (:sgautherie)]

(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.

Comment 49

[:Gavin Sharp [email: [e-mail]]]

Please file a new bug for additional work.

Comment 50

[Philip Chee]

> 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.

Comment 51

[Serge Gautherie (:sgautherie)]

(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

[Philip Chee]

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.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 54

[:aceman]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 55

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR search.