1646857 - Refactor/simplify nested conditionals code in LoadIdentity()

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

LoadIdentity() has several layers of nested conditionals, which can be refactored and simplified to achieve a more readable code flow with early returns.

Comment 1

[Thomas D. (:thomas8)]

.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1646857_loadIdentityConditionalsRefactor.diff

Simplify nested conditionals with early returns for a more readable code flow with less indentation levels.
This rewrite should not change the current code logic or behaviour in any way.

Please note that apart from touching a few lines to convert a few conditions into early returns, and the resulting changes in whitespace/indentation levels, I have not changed any other code in between. So it's a diff artifact which creates a false impression as if the whole code had changed.

It's now much clearer that the first section is shared and will always run, whereas the entire second section will only run for startup = false.

Do we even need to check if identityElement (#msgIdentity) is available?
I am not aware of any situation where that could be missing, because if it is, composition would be seriously broken. I'd guess that even addons would rather keep and manipulate the existing element.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9157792 [details] [diff] [review]
1646857_loadIdentityConditionalsRefactor.diff

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

Looks good I think.
Sent off a try run now (with identityElement not checked) - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=072319b83d15c4ef74e8ad75798967b60bd3a27d

::: mail/components/compose/content/MsgComposeCommands.js
@@ +7011,5 @@
>  function LoadIdentity(startup) {
>    let identityElement = document.getElementById("msgIdentity");
>    let prevIdentity = gCurrentIdentity;
>  
> +  if (!identityElement) {

I think it's always set. Would have to do a try run to make sure.

Comment 4

[Thomas D. (:thomas8)]

(In reply to Magnus Melin [:mkmelin] from comment #3)

• if (!identityElement) {

I think it's always set. Would have to do a try run to make sure.

From failing tests, I don't see anything which relates to this bug.
Some tests on Linux and MAC didn't run because of TBPL FAILURE/Exception, whatever that is.
So can this land?

Comment 5

[Magnus Melin [:mkmelin]]

Too busted try to say anything, sent another one: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7ec6dd796cc3e1ab51d18549ad1a119a578e7a7f

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/16ad4c7f7a26
Refactor nested conditionals code in LoadIdentity(). r=mkmelin

Comment 7

[Thomas D. (:thomas8)]

Attachment: 1646857_loadIdentityConditionalsRefactor.diff

[Approval Request Comment]
Regression caused by (bug #): no regression. code rewrite, substantial indentation changes.
User impact if declined: Developer impact if declined: Potential uplift hassles for > 1 year. It's quite likely that we may want to fix more bugs in this corner and uplift them, e.g. bug 1647050, so this should be uplifted so that release and daily code layout are in sync, avoiding uplift hassles later.
Testing completed (on c-c, etc.): yes, done by Magnus
Risk to taking this patch (and alternatives if risky): near zero. conditionals rewritten without changing code logic. clear and limited scope as we're only changing the internal code layout of one function.

Comment 8

[Thomas D. (:thomas8)]

Comment on attachment 9159215 [details] [diff] [review]
1646857_loadIdentityConditionalsRefactor.diff

Patch as landed on trunk in Comment 6, so this has review+ from mkmelin.

Comment 9

[Thomas D. (:thomas8)]

Comment on attachment 9157792 [details] [diff] [review]
1646857_loadIdentityConditionalsRefactor.diff

Old patch: minor change before landing (don't check for #msgIdentity), hence obsolete. Replaced by attachment 9159215 [details] [diff] [review].

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9159215 [details] [diff] [review]
1646857_loadIdentityConditionalsRefactor.diff

AIUI we will be OK if this hits 78.something, so let's not uplift to beta 78

Comment 11

[Alessandro Castellani [:aleca]]

Comment on attachment 9159215 [details] [diff] [review]
1646857_loadIdentityConditionalsRefactor.diff

[Approval Request Comment]
As reported originally on comment 7, not having this on 78 is causing uplift issues when dealing with regressions and fixes.
I have this issue with bug 1647050, where the patch doesn't apply on 78.

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9159215 [details] [diff] [review]
1646857_loadIdentityConditionalsRefactor.diff

Approved for esr78 (beta hasn't been out, but this has been on trunk 20+ days, so we have that)

Comment 13

[Rob Lemley [:rjl]]

Thunderbird 78.0.1:
https://hg.mozilla.org/releases/comm-esr78/rev/c8c306eb7eee

Comment 14

[Rob Lemley [:rjl]]

Thunderbird 78.0.1: (Prettier fix)
https://hg.mozilla.org/releases/comm-esr78/rev/ad2575a339c0dec10f90796dc498c3568502fbfa

Comment 15

[Jorg K (CEST = GMT+2)]

What has happened here? The trunk and the ESR patches look totally different:
https://hg.mozilla.org/comm-central/rev/16ad4c7f7a26
https://hg.mozilla.org/releases/comm-esr78/rev/c8c306eb7eee

And then there was a second patch for ESR?? - https://hg.mozilla.org/releases/comm-esr78/rev/ad2575a339c0dec10f90796dc498c3568502fbfa

That doesn't fill me with a lot of confidence that the correct code was landed. Also, the patch review said:
WINDOWS PATCH Attachment 9159215 [details] [diff]: 1646857_loadIdentityConditionalsRefactor.diff - Thomas D. <[e-mail]> - 6/25/2020

If there are a lot of white-space changes, it's good practise to submit a patch created with diff -w or hg (q)diff -w. I can't really see what was changed. Is https://hg.mozilla.org/releases/comm-esr78/rev/c8c306eb7eee the net change?

Comment 16

[Rob Lemley [:rjl]]

The short version is that when I grafted c-c:16ad4c7f7a26 on top of c-esr78:e3afee511 there was a merge conflict due to changes from bug 1644345 having also made changes to LoadIdentity().

I suspect the extra white space showed up because I had set my merge utility to ignore leading white space to make it easier to spot differences. Somehow when it sent the results of the merge back to Mercurial the white space were ignored there as well. In hindsight it would have made more sense to backout my bad uplift and reapply.

Comment 17

[Rob Lemley [:rjl]]

Attachment: esr78-uplift-take2.patch

This diff is from uplifting this bug again with Meld not ignoring leading whitespace. When I compared this with what landed on c-c:16ad4c7f7a260b94b68df9b81eb1f3891cb055d1. There is one line that is different around line 7096 (of the resulting file) which is due to bug 1644345.

•    goUpdateCommand("cmd_toggleReturnReceipt");
  

vs

•    ToggleReturnReceipt(msgCompFields.returnReceipt);
  

Then, compare the results of the two uplifts. The files should be identical (and are).

Comment 18

[Rob Lemley [:rjl]]

Attachment: patch

(In reply to Jorg K (CEST = GMT+2) from comment #15)

If there are a lot of white-space changes, it's good practise to submit a patch created with diff -w or hg (q)diff -w. I can't really see what was changed. Is https://hg.mozilla.org/releases/comm-esr78/rev/c8c306eb7eee the net change?

Essentially, yes. The indents are different.

Comment 19

[Thomas D. (:thomas8)]

(Midair collision with Rob's last 3 comments, but I'll file this comment anyway because it wasn't easy to figure this out. Some extra infos here).

(In reply to Jorg K (CEST = GMT+2) from comment #15)

What has happened here? The trunk and the ESR patches look totally different:

Original patch: attachment 9157792 [details] [diff] [review]. Reviewed, correct and fully functional.
I suggested another minor change (omitting leading early return check for identityElement), which Magnus kindly sent to try in comment 3.

https://hg.mozilla.org/comm-central/rev/16ad4c7f7a26

^^ Comment 6: Correctly landed on trunk, now including the minor change.
I then copied the raw changeset from Mercurial into a blank text file (and overlooked Windows-CR-LF, sorry) to create patch attachment 9159215 [details] [diff] [review] and requested approval-comm-beta in comment 7, which unfortunately was refused in comment 10, in spite of my pointing out future uplift problems if not uplifted now. In comment 11, Alex was affected by the predicted uplift problems and hence requested approval-comm-esr78.

https://hg.mozilla.org/releases/comm-esr78/rev/c8c306eb7eee

^^ Comment 13 (maybe because of the erroneous CR-LF) apparently applied diff -w to see the the net change and then accidentally landed that on Thunderbird 78.0.1 (omitting all whitespace changes).

And then there was a second patch for ESR?? - https://hg.mozilla.org/releases/comm-esr78/rev/ad2575a339c0dec10f90796dc498c3568502fbfa

^^ Yes, comment 14 (somewhat euphemistically titled "Prettier fix") had to land the accidentally omitted whitespace changes as a followup patch on Thunderbird 78.0.1. With that, the code changes are now complete and correct, i.e. matching attachment 9159215 [details] [diff] [review].

Unfortunately, there's what I'd consider an awful bug in the diff system somewhere which irritatingly and "randomly" fails to display whitespace-only changes side by side for some, but not all chunks, instead stupidly showing a single lost bracket in totally different lines as an unchanged line and creating diff chaos around that. I can't believe this is a technical necessity, so if someone can point me where to file this as a bug, I gladly will.
https://hg.mozilla.org/releases/comm-esr78/comparison/ad2575a339c0dec10f90796dc498c3568502fbfa/mail/components/compose/content/MsgComposeCommands.js

That doesn't fill me with a lot of confidence that the correct code was landed.

Looks all correct to me.

Also, the patch review said:
WINDOWS PATCH Attachment 9159215 [details] [diff]: 1646857_loadIdentityConditionalsRefactor.diff - Thomas D. <[e-mail]> - 6/25/2020

Explained and addressed, see above.

If there are a lot of white-space changes, it's good practise to submit a patch created with diff -w or hg (q)diff -w. I can't really see what was changed. Is https://hg.mozilla.org/releases/comm-esr78/rev/c8c306eb7eee the net change?

Yes. The good news is that after the hassles of this bug, the code logic is now significantly easier to understand.

(In reply to Thomas D. from comment #2)

Simplify nested conditionals with early returns for a more readable code flow with less indentation levels.
This rewrite should not change the current code logic or behaviour in any way.

Comment 20

[Jorg K (CEST = GMT+2)]

In my time, I never bothered much with fixing merge conflicts (since this is dangerous, one mistake and the thing falls apart) but always uplifted what had gotten into the way as well. So I'd back the ESR 78 stuff out here and reapply it after bug 1644345 (and fortunately I didn't have to ask anyone for permission ;-)) ().
EDIT: Bug 1647050 most likely got in the way too now.
EDIT 2: (), or maybe to late now, just something to keep in mind for next time.