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

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: string methods for /chat/

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

Comment 1

[Patrick Cloke [:clokep]]

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 2

[Florian Quèze [:florian]]

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(":")?

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 5

[:Mook]

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 6

[Florian Quèze [:florian]]

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

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: string methods for /chat/, v2

Comment 8

[Patrick Cloke [:clokep]]

I've looked over the changes in attachment #707759 [details] [diff] [review] and they seem fine by me. :)

Comment 9

[Florian Quèze [:florian]]

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.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: string methods for /chat/, v3

Comment 11

[Florian Quèze [: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.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: string methods for /chat/, v4, r=florian

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/61c46ff57276

Comment 14

[Patrick Cloke [:clokep]]

http://hg.instantbird.org/instantbird/rev/4fb10d2f5b31