Closed Bug 830456 Opened 11 years ago Closed 11 years ago

Code cleanup in /chat/: Use new String methods like startsWith, endsWith, contains, remaining Services.jsm switches and querySelector use instead of NodeList calls

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 21.0

People

(Reporter: aryx, Assigned: aryx)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch string methods for /chat/ (obsolete) — Splinter Review
This bug is similar to bug 824150 which is for /mail/ and /mailnews/.

Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=0a77fd710b43 - the usual oranges, the Linux debug also failed for a different Try run for me today, feel free to restart it (earlier this succeeded) together with the Linux64 debug Mozmill tests (I'm not touching plugin stuff).
Attachment #701914 - Flags: review?(florian)
Comment on attachment 701914 [details] [diff] [review]
string methods for /chat/

Review of attachment 701914 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/imXPCOMUtils.jsm
@@ +85,5 @@
>    // to avoid cycles with the pref service.
>    let logLevels = {
>      observe: function(aSubject, aTopic, aData) {
> +      // aData begins with "purple.debug.loglevel"
> +      let module = "level" + aData.substr(21);

I find this much less readable...and not that the regular expression is doing real validation, but this code will silently hide other prefixes, where the old code would at least choke and die on it. I don't like this change.
Comment on attachment 701914 [details] [diff] [review]
string methods for /chat/

Review of attachment 701914 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks like a nice code cleanup, thanks for looking at it!

Not a finished review, but I assume getting some feedback early is appreciated, so here are the comments I have after looking quickly:

::: chat/modules/imXPCOMUtils.jsm
@@ +85,5 @@
>    // to avoid cycles with the pref service.
>    let logLevels = {
>      observe: function(aSubject, aTopic, aData) {
> +      // aData begins with "purple.debug.loglevel"
> +      let module = "level" + aData.substr(21);

I agree with clokep that this changes the behavior. I don't remember very well what that code is supposed to do, so I would need to look more carefully to see if this is fine, or suggest what to do here.

@@ +101,5 @@
>  
>    // Initialize with existing log level prefs.
>    for each (let pref in Services.prefs.getChildList("purple.debug.loglevel")) {
>      if (Services.prefs.getPrefType(pref) == Services.prefs.PREF_INT)
> +      logLevels["level" + pref.substr(21)] = Services.prefs.getIntPref(pref);

I'm not a fan of the hardcoded 21 constant. Maybe move purple.debug.loglevel to const kLogLevelPref and use kLogLevelPref.length?

::: chat/protocols/irc/irc.js
@@ +1212,5 @@
>        //  1. If the last parameter contains a space.
>        //  2. If the first character of the last parameter is a colon.
>        //  3. If the last parameter is an empty string.
>        let trailing = params.slice(-1)[0];
> +      if (!trailing.length || trailing.contains(" ") || trailing[0] == ":")

Did you mean to replace trailing[0] == ":" with trailing.startsWith(":")?
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Comment on attachment 701914 [details] [diff] [review]

> ::: chat/modules/imXPCOMUtils.jsm
> @@ +85,5 @@
> >    // to avoid cycles with the pref service.
> >    let logLevels = {
> >      observe: function(aSubject, aTopic, aData) {
> > +      // aData begins with "purple.debug.loglevel"
> > +      let module = "level" + aData.substr(21);
> 
> I agree with clokep that this changes the behavior. I don't remember very
> well what that code is supposed to do, so I would need to look more
> carefully to see if this is fine, or suggest what to do here.

I think Mook wrote this code, CC'ing him.
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> ::: chat/protocols/irc/irc.js
> @@ +1212,5 @@
> >        //  1. If the last parameter contains a space.
> >        //  2. If the first character of the last parameter is a colon.
> >        //  3. If the last parameter is an empty string.
> >        let trailing = params.slice(-1)[0];
> > +      if (!trailing.length || trailing.contains(" ") || trailing[0] == ":")
> 
> Did you mean to replace trailing[0] == ":" with trailing.startsWith(":")?
Added that, had only switched the second condition from indexOf to contains before.

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> ::: chat/modules/imXPCOMUtils.jsm
> @@ +85,5 @@
> >    // to avoid cycles with the pref service.
> >    let logLevels = {
> >      observe: function(aSubject, aTopic, aData) {
> > +      // aData begins with "purple.debug.loglevel"
> > +      let module = "level" + aData.substr(21);
> 
> I agree with clokep that this changes the behavior. I don't remember very
> well what that code is supposed to do, so I would need to look more
> carefully to see if this is fine, or suggest what to do here.
It's no problem to remove these changes. The code here gets the name of the preference and uses the non-generic part for logging, this can be nothing or the module for which the logging has been defined, see http://mxr.mozilla.org/comm-central/source/chat/modules/imXPCOMUtils.jsm#108

Waiting for Mook before uploading the updated patch.
Yep, that change is fine (aData is the name of the pref, which should start with "purple.debug.loglevel" due to line 100-ish).  Though I do think the hard coded numerical constant is harder to read.

And now I feel bad for writing code hard to read enough that I should have a look over when being changed... sorry :(
Comment on attachment 701914 [details] [diff] [review]
string methods for /chat/

Review of attachment 701914 [details] [diff] [review]:
-----------------------------------------------------------------

Per Mook's comment 5, the changes to chat/modules/imXPCOMUtils.jsm are OK (please follow my suggestion in comment 2 of moving "purple.debug.loglevel" to a constant to avoid hardcoding the 21 value).

With these changes, this should be ready to go :).
Attachment #701914 - Flags: review?(florian) → review-
Attached patch string methods for /chat/, v2 (obsolete) — Splinter Review
Attachment #701914 - Attachment is obsolete: true
Attachment #707759 - Flags: review?(florian)
Attachment #707759 - Attachment description: 701914: string methods for /chat/, v2 → string methods for /chat/, v2
I've looked over the changes in attachment #707759 [details] [diff] [review] and they seem fine by me. :)
Comment on attachment 707759 [details] [diff] [review]
string methods for /chat/, v2

Review of attachment 707759 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/imXPCOMUtils.jsm
@@ +24,5 @@
>  const DEBUG_INFO = 2; // Verbose (= 'LOG')
>  const DEBUG_WARNING = 3;
>  const DEBUG_ERROR = 4;
>  
> +const LOG_LEVEL_STARTPOS = "purple.debug.loglevel".length;

I'm afraid I didn't explain myself clearly in the previous comments, sorry :-/.

What I expected here was:
const kLogLevelPref = "purple.debug.loglevel";

@@ +86,5 @@
>    // is necessary to make sure the observe is left alive while being a weak ref
>    // to avoid cycles with the pref service.
>    let logLevels = {
>      observe: function(aSubject, aTopic, aData) {
> +      let module = "level" + aData.substr(LOG_LEVEL_STARTPOS);

Here, kLogLevelPref.length

@@ +97,5 @@
>                                             Ci.nsISupportsWeakReference]),
>    };
>  
>    // Add weak pref observer to see log level pref changes.
>    Services.prefs.addObserver("purple.debug.loglevel", logLevels, true /* weak */);

Use the constant here...

@@ +100,5 @@
>    // Add weak pref observer to see log level pref changes.
>    Services.prefs.addObserver("purple.debug.loglevel", logLevels, true /* weak */);
>  
>    // Initialize with existing log level prefs.
>    for each (let pref in Services.prefs.getChildList("purple.debug.loglevel")) {

and here (and in other places if they exist).

@@ +102,5 @@
>  
>    // Initialize with existing log level prefs.
>    for each (let pref in Services.prefs.getChildList("purple.debug.loglevel")) {
>      if (Services.prefs.getPrefType(pref) == Services.prefs.PREF_INT)
> +      logLevels["level" + pref.substr(LOG_LEVEL_STARTPOS)] = Services.prefs.getIntPref(pref);

and kLogLevelPref.length here.
Attachment #707759 - Flags: review?(florian) → review-
Attached patch string methods for /chat/, v3 (obsolete) — Splinter Review
Attachment #707759 - Attachment is obsolete: true
Attachment #709222 - Flags: review?(florian)
Comment on attachment 709222 [details] [diff] [review]
string methods for /chat/, v3

Thanks!
Sorry for the delay in r+'ing this trivial update to the previous patch. I had completely forgotten about it and just found it in Bugzilla's weekly "Your Outstanding Requests" email.
Attachment #709222 - Flags: review?(florian) → review+
https://hg.mozilla.org/comm-central/rev/61c46ff57276
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
Blocks: 1110166
You need to log in before you can comment on or make changes to this bug.