1319409 - Rename GetSelectedDirectory() function to getSelectedDirURI()

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #196135 +++

I'd like to clean up a bit of messy code in abCommon.js.

Can we please rename the following function:
> GetSelectedDirectory()
to become
> getSelectedDirURI()
because that's what it actually does, contrary to what the current name of this function suggests.

Here's why:

This is quite confusing:
GetSelectedDirectory() function does NOT get the directory object as one would expect, but the URI property of the directory object.

Variables created from GetSelectedDirectory() function also share the same confusion, some named selectedDir, others named selectedAbUri etc.

This is totally inconsistent with the general naming convention in AB Code, where the term "directory" distinctly refers to the directory *object*.
Ironically, proof thereof is even found in GetSelectedDirectory() function itself:
>    return gDirectoryTreeView.getDirectoryAtIndex(gDirTree.currentIndex).URI;

Another example for the correct use of "directory" for the object is this function:
> GetDirectoryFromURI(aUri)

Clearly, getDirectory must be understood to return a directory *object*, and any other properties returned should have the property name in the function name.

You'll find that we even have inefficient and nonsensical reversing code like the following:
GetDirectoryFromURI(GetSelectedDirectory())
Not only is that extremely hard to read (getDirectory of getDirectory!?), but it's also inefficient because first we get the directory object, then we return its URI, then we reverse that to get the directory object again. Duh.

So after the above renaming, we should then add another, new function:
> getSelectedDir() to return the directory *object* straight away.
That's what I also need for bug 196135 where I'm coming from.

There are only 5 TB files and 2 Suite files using this function, so that's an average of only 5 occurences per file for a total of 38 occurences. Not many people are touching AB code this days, so I think that's a simple find and replace exercise which will assist daredevils like me who stubbornly believe that we owe it to our users to improve things in the old system because the new system has been promised forever but so far never materialized.

Comment 1

[Thomas D. (:thomas8)]

https://dxr.mozilla.org/comm-central/search?q=getselecteddirectory&redirect=false

Jörg, I'd be willing to do this if I get someone's blessing.
rsx11m, obviously this makes sense for SeaMonkey, too, to be consistent even though the files are branched.

Thanks.

Comment 2

[Thomas D. (:thomas8)]

Can this affect addons? Can someone find out if it does?

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #1)
> Jörg, I'd be willing to do this if I get someone's blessing.
Frankly: No. I don't think I have time for refactoring or clean-up unless there is a benefit for the user. Too many real bugs to fix. Maybe Aceman is interested. It's really just a straight textual replace. Can't you it yourself?

One other thing: We're currently at TB 53 busy with uplifts to TB 52. The more refactoring we do now, the more uplift conflicts we will get.

BTW, you're aware that the TB address book is on the "Abschussliste", that means, ear-marked for complete replacement/rewrite.

Comment 4

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #3)
> (In reply to Thomas D. (needinfo?me) from comment #1)
> > Jörg, I'd be willing to do this if I get someone's blessing.
> Frankly: No. I don't think I have time for refactoring or clean-up unless
> there is a benefit for the user. Too many real bugs to fix. Maybe Aceman is
> interested. It's really just a straight textual replace. Can't you it
> yourself?

Jörg, fast response is nice, but it gets tricky when you don't even read the question...
I've *not* asked *you* to do this, but I offered to fix this *myself*.
But I asked you if you agree to the proposed change.
Plus, (per comment 2), if it can affect addons. I think Aceman can answer the addon part.

> One other thing: We're currently at TB 53 busy with uplifts to TB 52. The
> more refactoring we do now, the more uplift conflicts we will get.

I'd think there's probably nobody except myself who's fixing things in AB.
Applying that "straight textual replace" to TB52 if needed should also be straightforward and simple.
The downside of not doing this now is that with any further patches, I'll continue using and expanding use of the flawed naming.

> BTW, you're aware that the TB address book is on the "Abschussliste", that
> means, ear-marked for complete replacement/rewrite.

Yes, it has been earmarked for replacement for the last 5 years or more, so I've stopped expecting. Meanwhile, our users continue to suffer from all the shortcomings, some of which are easy to fix.
I explicitly comment on that in my comment 0.

(In reply to Thomas D. (needinfo?me) from comment #0)
> Not many people are touching AB code this days, so I think that's a simple find
> and replace exercise which will assist daredevils like me who stubbornly
> believe that we owe it to our users to improve things in the old system
> because the new system has been promised forever but so far never
> materialized.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #4)
> Jörg, fast response is nice, but it gets tricky when you don't even read the
> question...
Damn right. Sorry. I could swear I read "would you be willing to take this on", but clearly I was hallucinating.

> But I asked you if you agree to the proposed change.
You've got my blessing ;-) This is quite ugly:
function GetSelectedDirectory()
{
  if (abList)
    return abList.value;
  else {
    if (gDirTree.currentIndex < 0)
      return null;
    return gDirectoryTreeView.getDirectoryAtIndex(gDirTree.currentIndex).URI; <===
  }
}

Why not just have two functions:
getSelectedDirectory() and getSelectedDirectoryURI() where the latter is just
  return getSelectedDirectory().URI;
?

> Plus, (per comment 2), if it can affect addons.
I have access there, too. It's just a simple search: https://dxr.mozilla.org/addons
Heaps of add-ons use this function, so just do:

function GetSelectedDirectory() { return getSelectedDirectoryURI(); }
function getSelectedDirectoryURI() { return getSelectedDirectory().URI; {
function getSelectedDirectory() { ... the code we have minus the .URI }

Sorry again for not reading it well enough.

Comment 6

[Jorg K (CEST = GMT+2)]

> Heaps of add-ons use this function.
To be precise: 73 references found.

Comment 7

[:aceman]

(In reply to Jorg K (GMT+1) from comment #5)
> Why not just have two functions:
> getSelectedDirectory() and getSelectedDirectoryURI() where the latter is just
>   return getSelectedDirectory().URI;
> ?
> 
> function GetSelectedDirectory() { return getSelectedDirectoryURI(); }
> function getSelectedDirectoryURI() { return getSelectedDirectory().URI; {
> function getSelectedDirectory() { ... the code we have minus the .URI }

Yes, I understand this was part of the proposal/bug description. If getSelectedDirectory is used instead of GetDirectoryFromURI(GetSelectedDirectory()) where needed, this will be an improvement, not just a rename of a function.

I'm fine with the proposal and can review it when done.

Although I do not understand why some functions start lowercase in that file and some are uppercase. What is the preferred style now or is there some difference depending no semantics of the function?

Comment 8

[Thomas D. (:thomas8)]

Attachment: Patch V.1 - rename function, and various related code cleanup

Grrrrr. This turned out to be much more work than expected, especially because some related code was so messy that there was no way to resist cleaning up whilst we are here. Happy churning!

rsx11m, if you could kindly review the SeaMonkey parts or set another reviewer for that. I aligned some nits here and there between TB and SM and copied over some of my code improvements, so that we don't look different where we aren't.

Aceman, I realized the code simplification improvements which you mentioned in your comment 8, and some of them were quite hard to spot so it's definitely wise to do that here and now. You volunteered to review this, so you'll get what you asked for *eg* ;)

Some remarks on details:

1) I followed most, but not all of Jörg's proposals, for performance reasons (see my next comment).

2) The directory tree is single-select and nobody is going to change that for the old AB, plus our function also tests for the single selection via currentIndex, so I conflated/removed some extra tests for single selection and tested for our new !getSelectedDirectory or !getSelectedDirectoryURI instead.

3) Some mind-warping nested if conditionals have been radically simplified without changing their meaning. Let's try to have short and concise if conditions especially when they involve "return". It's quite confusing to have a massive negated if-condition-block on top and what it really does (return false for isEnabled for deletion of special ABs) some 30 lines down, completely disconnected.

4) I replaced some dozens of var with let for variables which are clearly block scope, mostly in those corners which we churned anyway for the new function.
I'd like to know from Jörg, Magnus, and Aceman what is our official style for variables with function scope: let or var. I generally prefer let, but we could also use var as a hint for function scope if we are more serious to use let for block scope. Truly global variables should have a "g" in the variable name, like gDirTree. I'm quite failing to see how abList (without leading "g") passed the reviews, it's clearly a global variable like all others in that corner. But there are too many occurences and maybe addons are using that, so I desisted from changing that.
In switch statements, we should definitely use "var" for all variables in the main threads of the cases, to avoid he fallacy that let would block-scope inside each case, which it doesn't. So "let" can break those switches quite fast and should be avoided there.

5) Added and improved comments.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8813856 [details] [diff] [review]
Patch V.1 - rename function, and various related code cleanup

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

Nice. I'm nit sure what's going on re. let/var, I think it looks a little inconsistent.

::: mail/components/addrbook/content/abCommon.js
@@ +91,1 @@
>              return false;

Indentation.

@@ +115,5 @@
>        case "cmd_properties":
> +        var labelAttr = "valueGeneric";
> +        var accKeyAttr = "valueGenericAccessKey";
> +        var tooltipTextAttr = "valueGenericTooltipText";
> +        var isMailList;

Why change let to var. Normally we want to use let, so it would make more sense to change var to let.

@@ -197,4 @@
>  
>  function AbEditSelectedDirectory()
>  {
> -  if (gDirTree.view.selection.count == 1) {

Why can that test go?

@@ +208,5 @@
> +
> +  if (selectedDir.isMailList) {
> +    goEditListDialog(null, selectedDir.URI);
> +  }
> +  else {

} else { on one line.

@@ +392,4 @@
>          else
>            composeFields.to = GetSelectedAddressesFromDirTree();
>        }
> +      else {

Same line.

::: mail/components/addrbook/content/addressbook.js
@@ +336,1 @@
>      return;

I don't like this change. Why get the directory if the numSelected==0.

@@ +349,5 @@
> +    var printCardUrl = CreatePrintCardUrl(card);
> +    if (printCardUrl)
> +    {
> +       selectionArray[totalCard++] = printCardUrl;
> +    }

What's changed here? White-space. If you're already changing it, you could use 'let'.

::: mailnews/addrbook/content/abDragDrop.js
@@ +138,4 @@
>  
>      var targetURI = gDirectoryTreeView.getDirectoryAtIndex(index).URI;
>  
> +    var srcURI = getSelectedDirectoryURI();

let.

@@ +229,4 @@
>      }
>  
>      var targetURI = gDirectoryTreeView.getDirectoryAtIndex(index).URI;
> +    var srcURI = getSelectedDirectoryURI();

let.

::: suite/mailnews/addrbook/abCommon.js
@@ +54,5 @@
>          return (gAbView != null);
>        case "cmd_delete":
>        case "button_delete":
> +        var selectedDir = getSelectedDirectory();
> +        var selectedDirURI = selectedDir.URI;

let.

@@ -156,4 @@
>  
>  function AbEditSelectedDirectory()
>  {
> -  if (gDirectoryTreeView.selection.count == 1) {

See above. OK to remove test?

@@ +318,4 @@
>          else
>            composeFields.to = GetSelectedAddressesFromDirTree();
>        }
> +      else {

One line.

@@ +381,1 @@
>        for (var i = 0; i < listCardsCount; ++i)

let.

::: suite/mailnews/addrbook/addressbook.js
@@ +189,5 @@
>  {
>    var selectedItems = GetSelectedAbCards();
>    var numSelected = selectedItems.length;
> +  var uri = getSelectedDirectoryURI();
> +  

trailing space.

@@ +190,5 @@
>    var selectedItems = GetSelectedAbCards();
>    var numSelected = selectedItems.length;
> +  var uri = getSelectedDirectoryURI();
> +  
> +  if (!numSelected || !uri)

Same comment as above.

@@ +208,5 @@
> +    var printCardUrl = CreatePrintCardUrl(card);
> +    if (printCardUrl)
> +    {
> +       selectionArray[totalCard++] = printCardUrl;
> +    }

Not sure what's changed here. Could use let.

@@ +238,4 @@
>  
>  function AbPrintAddressBookInternal(doPrintPreview, msgType)
>  {
> +  var uri = getSelectedDirectoryURI();

let.

@@ +371,4 @@
>  
>  function onAdvancedAbSearch()
>  {
> +  var selectedABURI = getSelectedDirectoryURI();

let.

@@ +391,4 @@
>      // Get model query from pref, without preceding "?", so we need to add it again
>      gQueryURIFormat = "?" + getModelQuery("mail.addr_book.quicksearchquery.format");
>    }
> +  var searchURI = getSelectedDirectoryURI();

let.

Comment 10

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #5)
> (In reply to Thomas D. (needinfo?me) from comment #4)
> > Jörg, fast response is nice, but it gets tricky when you don't even read the
> > question...
> Damn right. Sorry. I could swear I read "would you be willing to take this
> on", but clearly I was hallucinating.

Maybe it was the same type of hallucination like when you claimed that I always keep getting my name wrong on patches whilst in 95% of the cases, it was just right... But with new patches coming, there's a good chance to get it wrong again so that you can come back at me ;)

> > But I asked you if you agree to the proposed change.
> You've got my blessing ;-) This is quite ugly:
> function GetSelectedDirectory()
> {
>   if (abList)
>     return abList.value;
>   else {
>     if (gDirTree.currentIndex < 0)
>       return null;
>     return
> gDirectoryTreeView.getDirectoryAtIndex(gDirTree.currentIndex).URI; <===
>   }
> }
> 
> Why not just have two functions:
> getSelectedDirectory() and getSelectedDirectoryURI() where the latter is just
>   return getSelectedDirectory().URI;
> ?

Yes, I created two functions which was extremely helpful for simplifying and clarifying related code.
As for the proposed nesting of our two new functions, that's tempting and aesthetically nice, but probably less performant. The two parts inside the function use opposed constructions, so that when it's more performant for the bottom part, it's less performant at the top, and vice versa. So I figured it's ultimately better to not nest the functions into each other:

> function getSelectedDirectory()
> {
>  if (abList)
>    return MailServices.ab.getDirectory(abList.value);

With nesting...
> function getSelectedDirectoryURI() { return getSelectedDirectory().URI }

...we'd then actually run this:
>  if (abList)
>    return MailServices.ab.getDirectory(abList.value).URI;

...which doesn't make sense because
> MailServices.ab.getDirectory(abList.value).URI == abList.value

...so it's better to just use that (abList.value) and avoid the recursive nesting.

> > Plus, (per comment 2), if it can affect addons.
> I have access there, too. It's just a simple search:
> https://dxr.mozilla.org/addons
> Heaps of add-ons use this function, so just do:
> 
> function GetSelectedDirectory() { return getSelectedDirectoryURI(); }

Done, thanks. Any way of telling addons that this function is now deprecated?
We don't want old stuff to hang around forever, and this one is so confusing, it should really be eliminated in the long run.

Comment 11

[:aceman]

(In reply to Thomas D. (needinfo?me) from comment #8)
> Grrrrr. This turned out to be much more work than expected, especially
> because some related code was so messy that there was no way to resist
> cleaning up whilst we are here. Happy churning!

But then you have broken the promise of keeping this bug to only renaming the single function so that uplifts are easy to do (just one function has to be renamed in patches for branches).
With this amount of changes, you've killed possibility of uplifts in many functions.

Or you do the patch changes needed for uplifts :)
 
> 4) I replaced some dozens of var with let for variables which are clearly
> block scope, mostly in those corners which we churned anyway for the new
> function.

Aren't you too much outside the scope of thus bug?

> I'd like to know from Jörg, Magnus, and Aceman what is our official style
> for variables with function scope: let or var. I generally prefer let, but
> we could also use var as a hint for function scope if we are more serious to
> use let for block scope.

'var' is used for global variables on top of the file outside of functions.
'var' is used for top level variables in functions.
'let' is used for local block scope variables.

> Truly global variables should have a "g" in the
> variable name, like gDirTree. I'm quite failing to see how abList (without
> leading "g") passed the reviews, it's clearly a global variable like all
> others in that corner.

Yes, either reviewer didn't notice or it is from times where these rules didn't apply ;)

> But there are too many occurences and maybe addons
> are using that, so I desisted from changing that.

There seems to be only 1 addon using it.

> In switch statements, we should definitely use "var" for all variables in
> the main threads of the cases, to avoid he fallacy that let would
> block-scope inside each case, which it doesn't. So "let" can break those
> switches quite fast and should be avoided there.

Or you can use different variable names in each case. Or if they are the same, you can define the variable before the switch(). Changing to 'var' makes the variable 'leak' to the whole function, to a larger scope than just the switch().

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #11)
> There seems to be only 1 addon using it.
Pardon me? I searched at https://dxr.mozilla.org/addons and found 73 references to GetSelectedDirectory().

Comment 13

[:aceman]

(In reply to Jorg K (GMT+1) from comment #12)
> (In reply to :aceman from comment #11)
> > There seems to be only 1 addon using it.
> Pardon me? I searched at https://dxr.mozilla.org/addons and found 73
> references to GetSelectedDirectory().

Talking of 'abList'...

Comment 14

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #13)
> (In reply to Jorg K (GMT+1) from comment #12)
> > (In reply to :aceman from comment #11)
> > > There seems to be only 1 addon using it.
> > Pardon me? I searched at https://dxr.mozilla.org/addons and found 73
> > references to GetSelectedDirectory().
> 
> Talking of 'abList'...

I'm not sure if we should slow Jörg down or speed him up...
Slow down is said to be good against hallucinations, but perhaps if we speed him up he'll do all the uplifts in no time... :p

Comment 15

[Thomas D. (:thomas8)]

Comment on attachment 8813856 [details] [diff] [review]
Patch V.1 - rename function, and various related code cleanup

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

Cool. So this big patch is quite good if practically the only nits we can find is let vs. var which is not the explicit goal of this patch, but just collateral updating of code, so you found some more spots where it can be updated.

Other concerns addressed and explained in this review.

::: mail/components/addrbook/content/abCommon.js
@@ +115,5 @@
>        case "cmd_properties":
> +        var labelAttr = "valueGeneric";
> +        var accKeyAttr = "valueGenericAccessKey";
> +        var tooltipTextAttr = "valueGenericTooltipText";
> +        var isMailList;

I explained that with the patch, and it's not as trivial as it looks. Aceman showed a better understanding which answers my concerns and correctly describes our options, where just having let here is not the best option imo. Let's say if I continue on my dynamic menus crusade, there's a good chance to have another command which also needs dynamic labels. Using let for same variable on both case statements will fail, using different variable names for essentially the same thing is odd, so we remain with moving the declarations above the switch statements so as to share them between the cases. Which brings them to function level where according to aceman and myself we should use var... :|

@@ -197,4 @@
>  
>  function AbEditSelectedDirectory()
>  {
> -  if (gDirTree.view.selection.count == 1) {

The Test can go because gDirTree only allows single selections. The check for that should be before this function (command should be disabled), but even if we end up here by mistake, testing for !selectedDir also requires a single selection and will exit if there's none, because selectedDir will return null.

@@ +208,5 @@
> +
> +  if (selectedDir.isMailList) {
> +    goEditListDialog(null, selectedDir.URI);
> +  }
> +  else {

Yeah, I saw that and I only went half the way, all styles are found on code but I also think one-line is good style and more room-efficient ;) Note how I fixed the same issue in other spots to get rid of some useless empty lines with a single bracket...

::: mail/components/addrbook/content/addressbook.js
@@ +336,1 @@
>      return;

Right, but I wonder why we need the check for selected directory at all. There's no way you can select a contact without prior selection of a directory, so this can getSelectedDirectory can never fail. So I think we can just drop the check for selected directory, uri isn't otherwise used in this function.
Even the check for selected cards does not belong here; ironically, they never bothered to disable the menuitems/command so you can still click print contact or print contact preview even without a contact selected, which will silently fail on our condition here.

::: suite/mailnews/addrbook/abCommon.js
@@ -156,4 @@
>  
>  function AbEditSelectedDirectory()
>  {
> -  if (gDirectoryTreeView.selection.count == 1) {

Yep, see above.

::: suite/mailnews/addrbook/addressbook.js
@@ +208,5 @@
> +    var printCardUrl = CreatePrintCardUrl(card);
> +    if (printCardUrl)
> +    {
> +       selectionArray[totalCard++] = printCardUrl;
> +    }

Whitespace/Indentation corrected from 5 to 4 spaces.

Comment 16

[Thomas D. (:thomas8)]

Please use the main page of splinter review to see comment 15 in context. The digest to comments is very poor...

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #16)
> Please use the main page of splinter review to see comment 15 in context.
I did. I still don't agree with changing 'let' to 'var' in the switch. I'm looking at the existing code and the 'let' is perfectly fine since the variables are only used in |case "cmd_properties":|. No need to change that. If one day you'll add another case, we'll revisit it. However, I'm glad that Aceman showed a better understanding.

Comment 18

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #17)
> (In reply to Thomas D. (needinfo?me) from comment #16)
> > Please use the main page of splinter review to see comment 15 in context.
> I did. I still don't agree with changing 'let' to 'var' in the switch. I'm
> looking at the existing code and the 'let' is perfectly fine since the
> variables are only used in |case "cmd_properties":|. No need to change that.
> If one day you'll add another case, we'll revisit it.

Sorry, I wasn't trying to insist on var, only that none of the solutions offered so far were nice/practical AND sustainable. I prefer to write sustainable code which does not need revisiting...
More so because the case syntax can easily be misunderstood to to case-scoped, which it is not.

The good news being, I found a solution which can make everyone happy, nice AND sustainable:
We can just add the missing block scope for case block, then we're free to declare any variables where we really need them, even if the same variables are used in other cases...:

switch (foo) {
  case 1: {
    let x=1
    break;
  }
  case 2: {
    let x=2
    break;
  }
}

A simple pair of {} as scope indicators - problem solved!

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #18)
> A simple pair of {} as scope indicators - problem solved!
We'll cross this bridge when we come to it ;-)
I'm really surprised about this discussion since in your recent work you used 'let':
https://hg.mozilla.org/comm-central/rev/71581aa2080102ab14918586874cdf0285ac1ba0#l1.13
https://hg.mozilla.org/comm-central/rev/71581aa2080102ab14918586874cdf0285ac1ba0#l6.12

Comment 20

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #19)
> (In reply to Thomas D. (needinfo?me) from comment #18)
> > A simple pair of {} as scope indicators - problem solved!
> We'll cross this bridge when we come to it ;-)

We're already on the bridge, because...

> I'm really surprised about this discussion since in your recent work you
> used 'let':
> https://hg.mozilla.org/comm-central/rev/
> 71581aa2080102ab14918586874cdf0285ac1ba0#l1.13

In my recent work which you cited I already started to use "var" as a workaround for exactly this problem that switch cases are not scoped by default:

https://hg.mozilla.org/comm-central/rev/71581aa2080102ab14918586874cdf0285ac1ba0#l1.17

> case "cmd_properties":
> ...
> +        var selectedDir = GetSelectedDirectory();

That's because a few lines further up, there's this (at least after the patch proposed here):

> case "button_delete":
> ...
> let selectedDir = getSelectedDirectory();

But to avoid misunderstandings, I'm not at all against using let, and I'll use let here as you proposed.

I'll also follow Aceman's coding style information that at function scope level (variables which are physically or logically at the top of function), we should use 'var', which obliterates some of the nits mentioned in your review.

Comment 21

[Thomas D. (:thomas8)]

Attachment: Patch V.2 - Nitfixes. Rename/add function, and various related code cleanup

Alright, so here's a new patch which addresses Jörg's review of Comment 9.

Switch-Case statements have let, and variables are declared and initiated close-to-use in their own cases, plus consistent with similar cases, so all is well ;)

I preserved var for function-scope variables, per Aceman's styleguide info of comment 11:

> 'var' is used for global variables on top of the file outside of functions.
> 'var' is used for top level variables in functions.
> 'let' is used for local block scope variables.

I also dropped one or two unnecessary/duplicated tests as explained in my comment 15.

Comment 22

[Thomas D. (:thomas8)]

Comment on attachment 8814561 [details] [diff] [review]
Patch V.2 - Nitfixes. Rename/add function, and various related code cleanup

Let me fix a couple of nits first which I saw when looking at the patch diff...

Btw, I did not change abList to gAbList, that can wait for another bug...

Comment 23

[Thomas D. (:thomas8)]

Attachment: Patch V.2.1 - (More Nitfixes) Rename/add function, and various related code cleanup

Functions inside controllers etc. are also functions, so they should use var for function scope variables.

And our main renamed/added functions, getSelectedDirectory() and getSelectedDirectoryURI(), just got better with sanitized if/else structure and improved annotations.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814568 [details] [diff] [review]
Patch V.2.1 - (More Nitfixes) Rename/add function, and various related code cleanup

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

Quite a few nits still. Please do what I suggested: Look for | $| in Notepad++ and any trailing spaces found them before submitting the patch.

::: mail/components/addrbook/content/abCommon.js
@@ +219,5 @@
>  }
>  
>  function AbDeleteSelectedDirectory()
>  {
> +  var selectedABURI = getSelectedDirectoryURI();

let.

@@ +428,4 @@
>  
> +  if (!selectedDir)
> +    return "";
> +  

Trailing spaces.

@@ +430,5 @@
> +    return "";
> +  
> +  if (!selectedDir.isMailList)
> +    return "";
> +  

Trailing spaces.

@@ +602,4 @@
>      }
>  
>      // this approach is necessary to support generic columns that get overlayed.
> +    var elements = document.querySelectorAll('[name="sortas"]');

let

@@ +662,5 @@
>  }
>  
> +/**
> + * Get the selected directory object.
> + * 

Trailing space

@@ +679,5 @@
> +}
> +
> +/**
> + * Get the URI of the selected directory.
> + * 

Trailing space.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +177,1 @@
>    var searchInput = document.getElementById("peopleSearchInput");

let twice.

::: mail/components/addrbook/content/addressbook.js
@@ +334,4 @@
>    if (!numSelected)
>      return;
>  
> +  var statusFeedback;

let

@@ +340,2 @@
>  
> +  var selectionArray = new Array(numSelected);

let

@@ +342,2 @@
>  
> +  var totalCard = 0;

let

@@ +378,4 @@
>  
>  function AbPrintAddressBookInternal(doPrintPreview, msgType)
>  {
> +  var uri = getSelectedDirectoryURI();

let

@@ +519,4 @@
>  
>  function onAdvancedAbSearch()
>  {
> +  var selectedABURI = getSelectedDirectoryURI();

let

@@ +540,4 @@
>      gQueryURIFormat = getModelQuery("mail.addr_book.quicksearchquery.format");
>    }
>  
> +  var searchURI = getSelectedDirectoryURI();

let

::: mailnews/addrbook/content/abDragDrop.js
@@ +56,4 @@
>        aXferData.data.addDataForFlavour("text/x-moz-address", selectedAddresses);
>        aXferData.data.addDataForFlavour("text/unicode", selectedAddresses);
>  
> +      var srcDirectory = getSelectedDirectory();

let

@@ +138,4 @@
>  
>      var targetURI = gDirectoryTreeView.getDirectoryAtIndex(index).URI;
>  
> +    var srcURI = getSelectedDirectoryURI();

let x2

@@ +229,4 @@
>      }
>  
>      var targetURI = gDirectoryTreeView.getDirectoryAtIndex(index).URI;
> +    var srcURI = getSelectedDirectoryURI();

let x2

::: suite/mailnews/addrbook/abCommon.js
@@ +373,4 @@
>  
> +  if (!selectedDir)
> +    return "";
> +  

trailing spaces.

@@ +373,5 @@
>  
> +  if (!selectedDir)
> +    return "";
> +  
> +  if (!selectedDir.isMailList)

Combine with preceding if.

@@ +375,5 @@
> +    return "";
> +  
> +  if (!selectedDir.isMailList)
> +    return "";
> +  

trailing spaces.

@@ +377,5 @@
> +  if (!selectedDir.isMailList)
> +    return "";
> +  
> +  var listCardsCount = selectedDir.addressLists.length;
> +  var cards = new Array(listCardsCount);

let x2

@@ +606,5 @@
>  }
>  
> +/**
> + * Get the selected directory object.
> + * 

trailing space.

@@ +623,5 @@
> +}
> +
> +/**
> + * Get the URI of the selected directory.
> + * 

Trailing space.

::: suite/mailnews/addrbook/addressbook.js
@@ +199,2 @@
>  
> +  var selectionArray = new Array(numSelected);

let

@@ +201,2 @@
>  
> +  var totalCard = 0;

let

@@ +275,4 @@
>   * Export the currently selected addressbook.
>   */
>  function AbExportSelection() {
> +  var selectedABURI = getSelectedDirectoryURI();

let

@@ +370,4 @@
>  
>  function onAdvancedAbSearch()
>  {
> +  var selectedABURI = getSelectedDirectoryURI();

let

@@ +390,4 @@
>      // Get model query from pref, without preceding "?", so we need to add it again
>      gQueryURIFormat = "?" + getModelQuery("mail.addr_book.quicksearchquery.format");
>    }
> +  var searchURI = getSelectedDirectoryURI();

let

Comment 25

[:aceman]

(In reply to Jorg K (GMT+1) from comment #24)
> ::: mail/components/addrbook/content/abCommon.js
> @@ +219,5 @@
> >  }
> >  
> >  function AbDeleteSelectedDirectory()
> >  {
> > +  var selectedABURI = getSelectedDirectoryURI();
> 
> let.

Well, these variables at function level are according to my spec. I'll see if I can find references if that is the right style.

Comment 26

[Jorg K (CEST = GMT+2)]

Yes, but since when do we use 'var' here? Take any "modern" source. New code by Magnus:
https://hg.mozilla.org/comm-central/rev/c8666b9f31bc868950a698b2cc28896d45a09030#l1.18
https://hg.mozilla.org/comm-central/rev/c8666b9f31bc868950a698b2cc28896d45a09030#l2.14

Or:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1616

> 'var' is used for global variables on top of the file outside of functions.
Yes.

> 'var' is used for top level variables in functions.
Umm, no.

> 'let' is used for local block scope variables.
Yes.

Comment 27

[:aceman]

(In reply to Jorg K (GMT+1) from comment #26)
> > 'var' is used for top level variables in functions.
> Umm, no.

OK, I could also only find reviews saying let also on function level, e.g. at bug 804004 comment 5.

Maybe I got it mixed up from all the years of review comments sometimes rejecting needless changes, e.g. because var and let on function level behave the same and Seamonkey is not as enthusiastic of let conversions than TB :)

Comment 28

[Jorg K (CEST = GMT+2)]

Well, how about you watch https://www.youtube.com/watch?v=sjyJBL5fkp8 and if you then still want to use 'var', we talk again. I actually never paid a lot of attention to it, but I saw all the horrible things you can do with 'var', so I now joined the 'let' fanatics ;-)

You are right, we don't go around changing 'var' to 'let', but 'let' needs to be used in new code and if you already in the vicinity, you clean it up as you go. Certainly never any conversion from 'let' to 'var'.

Comment 29

[Thomas D. (:thomas8)]

Attachment: Patch V.2.2 - (let let rule) Rename/add function, and various related code cleanup

Let let rule! As Aceman said, the last patch was 100% correct according to the style suggested by him, and I would have appreciated a more explicit reference to that without which your review might seem to suggest that I'm not able or willing to get it right... ;)

Thanks about the hint on searching whitespace with Notepad++, that's helpful and works really well (after finding that putting "|  $|" returned 38000 matches which appeared too much so "  $" worked better ;).

I won't tell anyone that this patch was received by cardinal sin of hacking patch directly which worked perfectly except for that minor glitch where I shot myself in the foot by touching Ctrl+D (duplicate line) when I wanted Ctrl+S (save) which blew up patch application... and I couldn't find from TortoiseHg where exactly it blew... gggrrrr. However, it helped to re-appreciate the phantastic usability of WinMerge for efficient manual keyboard-only diff of files... Alt+Down, Alt+Down, Alt+Down, (Alt+Left/Right if you want to apply the current difference between the two files) until falling asleep ;)

My idea is to land this first and then take my other candidates from there... I'm a bit worried about time, both my personal time and the stringfreeze... It's taking far longer than expected... Jörg and Aceman, thank you for working on this!

Comment 30

[Thomas D. (:thomas8)]

Comment on attachment 8814638 [details] [diff] [review]
Patch V.2.2 - (let let rule) Rename/add function, and various related code cleanup

Hmmm, sorry, looks like I've missed one small round of var-conversion due to physical tiredness and the irritation of failing to re-apply patch...
But then, if we continue like this, we end up doing var-conversion for all of addressbook, which I think is unwanted at this time considering the code churn... After all, code works with var and we can always do a systematic var-cleanup later at any time, so what if we drop our perfectionist hat for now and just try to get stuff landed... More so given that current AB code won't live forever even though - mark my words - it's gonna live much longer than we can ever imagine...

Comment 31

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #28)
> Well, how about you watch https://www.youtube.com/watch?v=sjyJBL5fkp8
Nice!

Comment 32

[Thomas D. (:thomas8)]

Attachment: Patch V.2.3 - (more lets) Rename/add function, and various related code cleanup

(In reply to Thomas D. (needinfo?me) from comment #30)
> Comment on attachment 8814638 [details] [diff] [review]
> Patch V.2.2 - (let let rule) Rename/add function, and various related code
> cleanup
> 
> Hmmm, sorry, looks like I've missed one small round of var-conversion...
> so what if we drop our perfectionist hat for
> now and just try to get stuff landed...

I picked up my perfectionist hat but this is much work for little gain. When we are done with this round of things, we should just replace all of them which would be much simpler because true vars are very rare.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #29)
> Thanks about the hint on searching whitespace with Notepad++, that's helpful
> and works really well (after finding that putting "|  $|" returned 38000
> matches which appeared too much so "  $" worked better ;).
I said in comment #24 to use a regular expression of a *single* space followed by a dollar to find spaces at the end of the line (since $ matches the end of the line). Everywhere on BMO when people use |xyz|, they are referring to the text between the two vertical bars, *excluding* of course the vertical bars.

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814651 [details] [diff] [review]
Patch V.2.3 - (more lets) Rename/add function, and various related code cleanup

OK, let's go with this.

I went through the changes visually and they seem OK. I mildly tested it and saw no problems.

I'm really very reluctant to uplift a heap of changes like this to Aurora, it would have been much better to do a minimal change here to change the function names. If Kent were doing the uplifts, I could guarantee you that he wouldn't take it.

Some other notes:
hg qimport bz:1319409 gives me a heap of errors due to the ANSI (and not UTF-8) encoding of the patch and the "ü". So someone will need to take very special care when landing this.

There are still trailing spaces in the patch. Do try what I suggested to find them before submitting a patch ;-) You can even edit the patch to remove them. In this case, no new patch is needed, I will remove them before landing them.

Since you touch suite/ code, you need a SM review. I have bypassed them before, but I will only make myself unpopular if I have to ;-) Ratty has been around yesterday, so let's see what he says. I don't want to mention that you smashed SM in bug 1310442/bug 1318852.

Comment 35

[Thomas D. (:thomas8)]

Attachment: Patch V.2.4 - (yet more lets) Rename/add function, and various related code cleanup

In the previous patch, I accidentally changed one let to var, so before Jörg finds out about this evil deed, let's fix it.

Philip, there are other bugs with string changes depending on this, and string freeze is fast approaching (first days of December). So it would be great if you can find time to review the Seamonkey parts of this, which are practically identical to the Thunderbird changes; all changes have already been reviewed by Jörg who has very sharp eyes 8)

Which triggered another round of hunting for vars which are very close to the context of our changes. I'm hoping that this is the last round of var-let changes. I'm also wondering, even for uplifting, if it's not easier if we change ALL var's in those files to let (unless they are at the top of the file, where I haven't seen many), instead of only a chosen few.

As you can see, following Jörg's stern inspiration I'm really starting to like 'let'... ;) Always willing to learn.

I also fixed one irritatingly misspelled local variable name (2 occurences):
msgComposFormat --> msgComposeFormat. All other related variables follow the same spelling, and I almost nuked my files' integrity because of this.

I'm hoping you can succeed with the uplift, thanks a lot. At this stage, it would also not be easy to rip this apart and start all over again, plus have a poor base for all the other incoming patches in the same area...

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814658 [details] [diff] [review]
Patch V.2.4 - (yet more lets) Rename/add function, and various related code cleanup

How come you didn't kill the trailing spaces while you were there?

(In reply to Thomas D. (needinfo?me) from comment #35)
> ... reviewed by Jörg who has very sharp eyes 8)
Ratty usually sees more than I do and he has more experience.

Comment 37

[Thomas D. (:thomas8)]

Attachment: Patch V.2.5 - (nitfixes) Rename/add function, and various related code cleanup

Sorry, last patch fixed one variable name, but I broke another one instead - fixed here (aSelectedABURI --> aselectedDirURI --> aSelectedDirURI).
I do believe that using uniform variable names for the same thing in different places makes better readable code...

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814661 [details] [diff] [review]
Patch V.2.5 - (nitfixes) Rename/add function, and various related code cleanup

Thomas, I DO NOT APPRECIATE being ignored. The trailing spaces are still there. I've told you repeatedly to remove them and you can easily do it in the patch itself.

Comment 39

[Thomas D. (:thomas8)]

Attachment: Patch V.2.6 - (nitfixes: 5 trailing spaces removed, 8 lines of context) Rename/add function, and various related code cleanup (streamlined function calls and conditionals, var -> let)

For everyone's convience and peace of mind, here's the same patch now with 8 lines of context and *drumrolls...* 5 trailing spaces removed!!!

(In reply to Jorg K (GMT+1) from comment #38)
> Comment on attachment 8814661 [details] [diff] [review]
> Patch V.2.5 - (nitfixes) Rename/add function, and various related code
> cleanup
> 
> Thomas, I DO NOT APPRECIATE being ignored. The trailing spaces are still
> there. I've told you repeatedly to remove them and you can easily do it in
> the patch itself.

Sorry for consecutive patches, but fighting with selected(!) occurences of var vs. let in a 1700 line patch which seeks to clean up the existing mess of misnamed function calls and other inefficiencies of code, it's not easy to do and remember and keep track of everything at once...

So things which are practically irrelevant, like the existence of exactly five(!) trailing spaces in the previous patch, four of which in comments (!), are easy to overlook between all the other hard and real work... So I wasn't ignoring anyone or anything, but: Errare humanum est... :|

So Jörg, just relax, all is well, I know you are more than hardworking, too, but let's not push it too far and SHOUT at me for nothing... Have a beer and instead and play football with your child... ;)

Comment 40

[Thomas D. (:thomas8)]

Furthermore, I voluntarily removed about a dozen or so superfluous leading spaces from misaligned indenting, so even if I add 5 trailing spaces, it's still a net improvement :p
Time to get some fresh air for all coders involved, why are we working on a Sunday without even being paid!? We must be crazy...

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #39)
> So I wasn't ignoring anyone or anything, but: Errare humanum est... :|
Well, we talked about trailing spaces in depth and the | $| regexp pattern, so I expected my comment being addressed. I last mentioned in comment #34 and comment #36, so sorry, I put some EMPHASIS on the issue in comment #38.

> Have a beer and instead and play football with your child... ;)
I don't drink and the little one is six months (!!) old. He doesn't even crawl.

(In reply to Thomas D. (needinfo?me) from comment #40)
> We must be crazy...
My wife calls it a hobby.

Comment 42

[:aceman]

Comment on attachment 8814669 [details] [diff] [review]
Patch V.2.6 - (nitfixes: 5 trailing spaces removed, 8 lines of context) Rename/add function, and various related code cleanup (streamlined function calls and conditionals, var -> let)

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

Thanks, looks good to me.

But I had the impression this was not intended to be uplifted and that the string freeze is already past us?
Is there any news on this front?

::: mail/components/addrbook/content/abCommon.js
@@ +422,5 @@
>  // item selected in the directory pane is not a mailing list,
>  // an empty string is returned.
>  function GetSelectedAddressesFromDirTree()
>  {
> +  let addresses = "";

This variable seems unneeded.

@@ +438,1 @@
>    return addresses;

It seems you can just 'return GetAddressesForCards(cards);' here.

::: mail/components/addrbook/content/addressbook.js
@@ -335,5 @@
>      return;
>  
> -  var uri = GetSelectedDirectory();
> -  if (!uri)
> -    return;

Nice, this was unused? :)

@@ +347,5 @@
> +    let card = selectedItems[i];
> +    let printCardUrl = CreatePrintCardUrl(card);
> +    if (printCardUrl)
> +    {
> +       selectionArray[totalCard++] = printCardUrl;

I would just selectionArray.push(here) and then totalCard is not even needed, but that is for another bug where we can test it properly :)

@@ +579,5 @@
>    var cardViewBox       = GetCardViewBox();
>    var cardViewBoxEmail1 = GetCardViewBoxEmail1();
>    var searchBox         = document.getElementById('search-container');
>    var dirTree           = GetDirTree();
> +  let searchInput       = document.getElementById('peopleSearchInput');

I'm not sure what algorithm made you change searchInput to 'let' but not the others, but please do not reply and do not touch it more in this bug :)

Comment 43

[Thomas D. (:thomas8)]

Jörg, this patch is totally safe for SeaMonkey, because we're not changing any existing functionality, we're just replacing an old function with a new (renamed) twin function, adjusting the function calls accordingly, and rewriting some small portions of code, where the rewrites are 100% functionally identical with the original.
The new functions have been fully implemented for SeaMonkey, too.
Moreover, we are not even dropping the old function, but still offer it as a wrapper, so it's really totally safe.
The full code, inluding the SeaMonkey parts, has undergone and passed technical review.

The code rewrites are like the following:

Original, nested code:

> if (x!=1)
>   return bar
> else
>  return foo
>
> if (y==1)
>  return foo

Rewritten, streamlined code:

> if (x==1 || y==1)
>   return foo
>
> return bar

Which is obviously 100% functionally equivalent, just more readable.
So SeaMonkey can be quite happy that we are improving the quality of their code, too.

So there's no need to delay our landings and wait for the SeaMonkey review.
Could you please land this? Thank you.

Comment 44

[:aceman]

Even in perfectly logical change, there can always be a typo in the SM part which you will not find without running with the patch. Please do not rush big patches like this.
Also, I'm sure Ratty will find something to improve (also for TB) :)

Comment 45

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #44)
> Even in perfectly logical change, there can always be a typo in the SM part
> which you will not find without running with the patch. Please do not rush
> big patches like this.

I'm not known for typos, and I don't see why the hypothetical existence of a typo in the SM part should stop us from landing this and move on in TB. It will actually help us and them to find any typo faster. Plus typos are easy to fix with a one-liner follow-up patch.

On the other hand, not landing this makes it much harder to land other important TB patches which include string changes, with string freeze around the corner on Dec. 5. Without this patch here, we are forced to continue using the outdated function everywhere, so it's a lot of work both for me to downconvert my current patches, and then again later to upgrade the code again. Which is more than just search and replace, because this patch does two things:
Rename GetSelectedDirectory to getSelectedDirectoryURI (which is what the old function really does)
Add a new function:            getSelectedDirectory (which now really gets the *object* as expected)
Not having the new function available means all incoming patches need to work around that and use outdated ways of accessing the current directory *object*.

> Also, I'm sure Ratty will find something to improve (also for TB) :)

Such improvements can only affect some of the details of this patch, but not its core, which is to rename an existing function (and all the callers) because the current function name is grossly misleading. We'll happily receive Ratty's review and ideas for improvements but I see no reason why that should stop this from landing. With this landed, it will probably be *easier* to then land the improvements.

Finally, even though this patch looks big, the potential to break things is near-zero, because the actual code *changes* are very few. Much of this patch just renames the function callers, plus a *lot* of dead-simple rewrites from 'var' to 'let', which is purely cosmetical and can never break anything.
So while I'm looking forward to Ratty's review, I maintain that this is safe to land now, and it should land now so we can move on with other important work including string changes.

Comment 46

[:aceman]

(In reply to Thomas D. (needinfo?me) from comment #45)
> I'm not known for typos, and I don't see why the hypothetical existence of a
> typo in the SM part should stop us from landing this and move on in TB. It
> will actually help us and them to find any typo faster. Plus typos are easy
> to fix with a one-liner follow-up patch.

But why land with typos if they can be prevented easily.
 
> On the other hand, not landing this makes it much harder to land other
> important TB patches which include string changes, with string freeze around

And that's the problem right there. I don't think we can consider these purely cosmetic changes in the abandoned addressbook as *important*.

It's fine that you like to work on the AB, but you can't tie 2 other devs to look at it with highest priority.

> Finally, even though this patch looks big, the potential to break things is
> near-zero, because the actual code *changes* are very few. Much of this
> patch just renames the function callers, plus a *lot* of dead-simple
> rewrites from 'var' to 'let', which is purely cosmetical and can never break
> anything.

Untrue. var and let have different behaviour so you can create problems if you converted incorrectly (e.g. if you blindly convert to 'let' into the 'case' blocks).

> So while I'm looking forward to Ratty's review, I maintain that this is safe
> to land now, and it should land now so we can move on with other important
> work including string changes.

But those are the rules. You need to collect all reviews. If you didn't want to wait for the SM part, you could have made a patch purely for TB, have it landed and made a new bug called "Port bug 1319409 to Seamonkey" that can be reviewed/landed later.
But that is not worth it when the difference of reviews is just several days.
And I appreciate you made the the improvements to SM in one go with TB, so that they are potentially not forgotten.

Comment 47

[Thomas D. (:thomas8)]

Attachment: Patch V. 3 - (updated to current trunk) Rename/add function, and various related code cleanup (streamlined function calls and conditionals, var -> let)

TortoiseHG auto-updated this patch when I pulled incoming changes to trunk.
Some patches like PluralForms were entering my territory ;)

Comment 48

[Thomas D. (:thomas8)]

Comment on attachment 8815605 [details] [diff] [review]
Patch V. 3 - (updated to current trunk) Rename/add function, and various related code cleanup (streamlined function calls and conditionals, var -> let)

Philip, this patch looks big but does little:
* Rename a function and all of its callers, both for TB and SM:
                             GetSelectedDirectory() --> getSelectedDirectoryURI()
* add one new twin function: getSelectedDirectory(), to get the directory *object*
* change loads of 'var's to 'let's
* plus less than a handful of minor code cleanup to simplify if-structures without changing their meaning.

So most of the changes are purely cosmetical, and big chunks are duplicated between TB and SM. We also keep a function wrapper with the old function name for legacy access. Furthermore, the whole patch has already been reviewed for technical correctness. So this might be an easy review, and if you can find the time, we'd much appreciate that, as it will remove our last flimsy excuse for not landing this. Landing this is much needed for other incoming patches which use the new functions, including string changes, with TB's string freeze around the corner on Dec. 5. Tia.

Comment 49

[Jorg K (CEST = GMT+2)]

These address book changes are really giving me a headache, and as Aceman said, I'm tied up with this more than I'd like.

The trouble here is that some of the code lives in mailnews/ and some is in mail/ and suite/. Some files are complete identical copies, like pref-directory-add.dtd and pref-directory-add.dtd , others have subtle differences abMailListDialog.dtd (these three are changed in bug 1308776), others again are quite different, like abCommon.js. We already smashed SM in bug 1310442/bug 1318852 which is not nice.

Sadly there are competing requirements here:
1) For any code change (unless an urgent bustage fix, a comment or totally trivial otherwise)
   we need a review by a module peer.
2) Certain strings are need to be landed by certain dates.
3) SM reviewers are usually less available.

I know that waiting for reviews is totally frustrating and seeing cut-off dates missed because a review didn't come is utterly discouraging.

This discussion is rally about bug 1308776, not this bug here, since only it has string changes. So what can we do? These are the options:
a) land without review
b) land the TB and Mailnews parts only and smash SM for sure
c) land bug 1308776 first
d) only land the strings of bug 1308776. Looks like bug 1308776 will do more damage to SM,
   it does not even attempt to fix SM, FRG has already asked for the strings to be added
   to suite/ as well (see list of affected files in second paragraph in this comment).

I'm still thinking about it, we still have a couple of days. I'm taking a few days off on address book bugs at least until Saturday.

Comment 50

[:aceman]

(In reply to Jorg K (GMT+1) from comment #49)
> c) land bug 1308776 first

I proposed landing the small bugs first (196135 comment 11), some of them needing uplift. Then the bug here would be landed last and not uplifted. Landing it first will cause it to be uplifted too.

Comment 51

[Jorg K (CEST = GMT+2)]

You need to write "bug" for Bugzilla to insert a link: Bug 196135 comment 11)
There this first option you named was "Please fix bug 1319409 [this bug here] first, ..." ;-)

Comment 52

[:aceman]

I think we also have the option here to split the patch and land the /mail part first then /mailnews and /suite in seconds part. I think the parts are would be independent enough. After landing the /mail part the /mailnews part will still call the old function but that one will still exist (we kept it for compatibility). Can you try that out?

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: Patch V. 3.1

Addressed review comments from Aceman's review in comment #42.
This patch is ANSI encoded (for the "ü").

Comment 54

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/acd6a5e56d6666c64ca34a883354fb14a2bf6038

Landed with the correct "ü" in Düllmann; on Windows the patch needs to be ANSI encoded, on Linux and Mac it needs to be UTF-8 encoded. Be careful with other non-ASCII characters like ellipsis ("…")!

Some comments:
This patch has suite/ parts which were landed without SM review and approval for the following reasons:

We need these patches since further bugs which have string changes (bug 1308776, bug 1319493) are based on them. These bugs need to land in the next three days to meet the late string freeze for TB 52 (Aurora) on Dec. 5th, 2016.

We asked for review, but sadly it didn't come on time. I sincerely appreciate Ratty's input, but unfortunately we couldn't wait more.

The suite/ changes are exact ports of changes made in mail/ (abCommon.js and addressbook.js) which have been reviewed by two Thunderbird peers. Since there are also mailnews/ changes involved in this patch, I decided that it made no sense to follow the standard procedure to not land suite/ changes since SM would have been busted.

So good faith and in the best interest of SM, I landed the suite/ parts as well. This decision hasn't been taken lightly, there has been much deliberation in this bug and also in private messages of the people involved.

I am the manager for Thunderbird 52 during the Aurora and Beta cycle and perhaps also during ESR. I didn't want to complicate my life here with multiple patches, multiple landings and multiple uplifts.

If you SM people are not happy with this, please let me know.

Comment 55

[Jorg K (CEST = GMT+2)]

Comment on attachment 8816442 [details] [diff] [review]
Patch V. 3.1

Too quick, ? for now, approval and uplift on Monday, Dec. 5th.

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 8816442 [details] [diff] [review]
Patch V. 3.1

This was required for another bug that didn't get ready on time. The fair amount of changes is risky and the risk is not worth taking if the other bug can't land.

Comment 57

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #56)
> Comment on attachment 8816442 [details] [diff] [review]
> Patch V. 3.1
> 
> This was required for another bug that didn't get ready on time. The fair
> amount of changes is risky and the risk is not worth taking if the other bug
> can't land.

Nonsense. As I've explained many times here, there's nothing risky about this patch.
But it was helpful for better code in a lot of other bugs, so to go back on your original decision of landing this for TB52 is surprising, disappointing, and wrong imo.

Comment 58

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (needinfo?me) from comment #57)
> Nonsense.
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

> As I've explained many times here, there's nothing risky about
> this patch.
The point is, I don't need this patch. You can argue far and wide about risk, usually I would take more risks than Kent, and he already takes more risks than other release manager.

I was surprised that a one line change here:
https://hg.mozilla.org/releases/comm-esr45/rev/d439d002b2ee#l1.43
caused a terrible regression that now every new installation of TB will show the system integration dialogue forever: Bug 1314236 / bug 1321469. Who would have thought. Aceman wrote the patch and I reviewed and we both messed it up. Now this is in an ESR and molesting users. That's risk.

Comment 59

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #42)
> I would just selectionArray.push(here) and then totalCard is not even
> needed, but that is for another bug where we can test it properly :)

Not needed, and I fell for that, grrr:
https://hg.mozilla.org/comm-central/rev/acd6a5e56d66#l3.25
https://hg.mozilla.org/comm-central/rev/acd6a5e56d66#l3.47

To be addressed in bug 1329455.

Comment 60

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/06d0c74bf867ad62fc6c50f4e5724544522013bf (this bug)
https://hg.mozilla.org/releases/comm-aurora/rev/1e40d7501405429e0f88adefc3d096c59464dc56 (regression 1)
https://hg.mozilla.org/releases/comm-aurora/rev/1d819c377a51ebb29c4285930268734a2b3a6b6b (regression 2)

I decided to uplift this to Aurora together with two regression fixes. We will need to address more bugs in the address book during the ESR 52 cycle, for example bug 1329295, and the refactoring here will be forever in our way. At least that's my feeling.

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 8816442 [details] [diff] [review]
Patch V. 3.1

I've changed my mind about the Aurora uplift, see previous comment.