No good reason not to put this in TB 38. Straightforward port, easy to review ;)

(In reply to aleth [:aleth] from comment #1) > Straightforward port, easy to review ;) By which I mean: the only changes compared to conversation.xml are the commented-out bits.

Comment on attachment 8566848 [details] [diff] [review] tbtabcomplete.diff Review of attachment 8566848 [details] [diff] [review]: ----------------------------------------------------------------- I left some silly nits, but while we're looking at this code, we should fix them. ::: mail/components/im/content/imconversation.xml @@ +485,5 @@ > + text.substring(pos - suffix.length, pos) == suffix) { > + let completions = [b for (b of this.buddies.keys())]; > + // Check if the preceding words are a sequence of nick completions. > + let preceding = text.substring(0, pos - suffix.length).split(", "); > + if (preceding.every(function(n) completions.indexOf(n) != -1)) { Nit: This could use contains. @@ +596,5 @@ > + if (wordStart > 2) { > + let separator = text.substring(wordStart - 2, wordStart); > + if (separator == ": " || separator == ", ") { > + let preceding = text.substring(0, wordStart - 2).split(", "); > + if (preceding.every(function(n) completions.indexOf(n) != -1)) { This could use contains. @@ +601,5 @@ > + secondNick = true; > + isFirstWord = true; > + // Remove preceding completions from possible completions. > + completions = completions.filter(function(c) > + preceding.indexOf(c) == -1); And here. @@ +611,5 @@ > // Keep only the completions that share |word| as a prefix. > // Be case insensitive only if |word| is entirely lower case. > let condition; > if (word.toLocaleLowerCase() == word) > + condition = function(c) c.toLocaleLowerCase().indexOf(word) == 0; We probably want to port this change in the other direction. @@ +616,2 @@ > else > + condition = function(c) c.indexOf(word) == 0; And this should be ported to IB.

That's entirely unrelated, but if you insist. By the way, contains() no longer exists.

Comment on attachment 8567026 [details] [diff] [review] tbtabcomplete.diff v2 Review of attachment 8567026 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to aleth [:aleth] from comment #4) > By the way, contains() no longer exists. Yeah, I realized this morning that it's includes, not contains. Sorry! I'd still really like this code to be modularized and tests added, but that's not a good reason to port this to TB.

https://hg.mozilla.org/comm-central/rev/56cf0d1ed0a0 https://hg.mozilla.org/comm-central/rev/471a92fcd94f

So what happens with this post bug 1136359? Do you plan to reopen and fix the regression?

(In reply to Kent James (:rkent) from comment #8) > So what happens with this post bug 1136359? Do you plan to reopen and fix > the regression? There is nothing wrong with the code here other than that *.includes() is only supported in nightlies, which we were not aware of when this landed. Therefore there is a regression on comm-aurora that does not exist in comm-central. I decided to open a fresh bug for this (bug 1136359) as that backout will be reversed when m-c decides to ship. *.includes().

(In reply to aleth [:aleth] from comment #9) > (In reply to Kent James (:rkent) from comment #8) > > So what happens with this post bug 1136359? Do you plan to reopen and fix > > the regression? > > There is nothing wrong with the code here other than that *.includes() is > only supported in nightlies, which we were not aware of when this landed. > Therefore there is a regression on comm-aurora that does not exist in > comm-central. I decided to open a fresh bug for this (bug 1136359) as that > backout will be reversed when m-c decides to ship. *.includes(). I suppose the real questions here are: 1) Do you plan to continue doing any work in this bug (if not, I will remove the tracking flags). 2) Is there still an issue that needs tracking? If so, what is the bug for that?

(In reply to Kent James (:rkent) from comment #10) > 1) Do you plan to continue doing any work in this bug (if not, I will remove > the tracking flags). > 2) Is there still an issue that needs tracking? If so, what is the bug for > that? Correct me if I'm wrong, but I thought the tracking flags indicate an intention to fix the bug for that particular release. Are the tracking flags removed again when the bug is fixed?

(In reply to aleth [:aleth] from comment #11) > (In reply to Kent James (:rkent) from comment #10) > > 1) Do you plan to continue doing any work in this bug (if not, I will remove > > the tracking flags). No, this bug is resolved fixed and the feature has landed.

We've gone back and forth on what exactly the flags mean. But I think we all agree that they are primarily there to be a tool for release managers. As I am taking on more of the role these days of release manager, I'm going to increasingly interpret the flags as I see fit, trying of course to stay somewhat consistent with overall Mozilla practice. So I view the flags as a way that release managers (in the case mostly me) can run Bugzilla queries to see what issues they should be monitoring that will affect a release. Once all decisions about the issue have been made and implemented, there is no longer any need to monitor the issue, hence no need for the flag. So I will tend to remove it once the issue is resolved. There are other flags for release notes, when fixed, etc. For tracking-thunderbird_esr38 that will be used once we have a separate esr38 repo. For now, the only possible meaning of that would be "we won't be able to land this in Thunderbird 38 before we create esr38, but after we do then we should keep watching this bug" I doubt that is what you mean in this case.

(In reply to Kent James (:rkent) from comment #13) OK, thanks for the explanation!

(In reply to Kent James (:rkent) from comment #13) > [...] trying of course to stay > somewhat consistent with overall Mozilla practice. > > [...] Once all decisions about the issue have been made and > implemented, there is no longer any need to monitor the issue, hence no need > for the flag. So I will tend to remove it once the issue is resolved. The practice of release managers outside of c-c is to keep the tracking flag (or add it and set it to '+') when a bug is fixed and they cared about it, so that if the patch ever gets backed out, the bug appears in bugzilla queries again. I assume the Firefox release managers search bugzilla for tracking+ and status not 'fixed'.

(In reply to Florian Quèze [:florian] [:flo] (Away until March 11) from comment #15) > (In reply to Kent James (:rkent) from comment #13) > > [...] trying of course to stay > > somewhat consistent with overall Mozilla practice. > > > > [...] Once all decisions about the issue have been made and > > implemented, there is no longer any need to monitor the issue, hence no need > > for the flag. So I will tend to remove it once the issue is resolved. > > The practice of release managers outside of c-c is to keep the tracking flag > (or add it and set it to '+') when a bug is fixed and they cared about it, > so that if the patch ever gets backed out, the bug appears in bugzilla > queries again. > > I assume the Firefox release managers search bugzilla for tracking+ and > status not 'fixed'. Not a bad idea. Perhaps if I can get some queries setup to do that better, I can try that.