Last Comment Bug 830456 - Code cleanup in /chat/: Use new String methods like startsWith, endsWith, contains, remaining Services.jsm switches and querySelector use instead of NodeList calls
: Code cleanup in /chat/: Use new String methods like startsWith, endsWith, con...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: Sebastian H. [:aryx][:archaeopteryx]
:
Mentors:
Depends on:
Blocks: 1110166
  Show dependency treegraph
 
Reported: 2013-01-14 12:08 PST by Sebastian H. [:aryx][:archaeopteryx]
Modified: 2014-12-11 07:13 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
string methods for /chat/ (17.67 KB, patch)
2013-01-14 12:08 PST, Sebastian H. [:aryx][:archaeopteryx]
florian: review-
Details | Diff | Review
string methods for /chat/, v2 (18.43 KB, patch)
2013-01-29 11:59 PST, Sebastian H. [:aryx][:archaeopteryx]
florian: review-
Details | Diff | Review
string methods for /chat/, v3 (18.57 KB, patch)
2013-02-01 13:34 PST, Sebastian H. [:aryx][:archaeopteryx]
florian: review+
Details | Diff | Review
string methods for /chat/, v4, r=florian (18.58 KB, patch)
2013-02-10 13:37 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Review

Description Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 12:08:11 PST
Created attachment 701914 [details] [diff] [review]
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] 2013-01-14 12:15:56 PST
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] [:flo] 2013-01-15 04:14:13 PST
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] [:flo] 2013-01-15 04:16:02 PST
(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 H. [:aryx][:archaeopteryx] 2013-01-15 11:18:17 PST
(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 2013-01-15 18:45:18 PST
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] [:flo] 2013-01-28 02:59:37 PST
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 H. [:aryx][:archaeopteryx] 2013-01-29 11:59:06 PST
Created attachment 707759 [details] [diff] [review]
string methods for /chat/, v2
Comment 8 Patrick Cloke [:clokep] 2013-01-29 12:54:24 PST
I've looked over the changes in attachment #707759 [details] [diff] [review] and they seem fine by me. :)
Comment 9 Florian Quèze [:florian] [:flo] 2013-01-29 13:32:04 PST
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 H. [:aryx][:archaeopteryx] 2013-02-01 13:34:08 PST
Created attachment 709222 [details] [diff] [review]
string methods for /chat/, v3
Comment 11 Florian Quèze [:florian] [:flo] 2013-02-10 07:48:39 PST
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 H. [:aryx][:archaeopteryx] 2013-02-10 13:37:06 PST
Created attachment 712285 [details] [diff] [review]
string methods for /chat/, v4, r=florian
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-02-11 06:12:31 PST
https://hg.mozilla.org/comm-central/rev/61c46ff57276
Comment 14 Patrick Cloke [:clokep] 2013-02-17 16:17:37 PST
http://hg.instantbird.org/instantbird/rev/4fb10d2f5b31

Note You need to log in before you can comment on or make changes to this bug.