1573670 - Wrap all custom element code in curly bracket blocks

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Paul Morris [:pmorris]]

Most of our custom elements (classes etc.) are wrapped in curly bracket blocks to prevent leaking to window scope, but not all of them are. Wrap the remaining ones in blocks as well, and do this before the auto-formatting of bug 1572047 since it will affect indentation.

Comment 1

[Paul Morris [:pmorris]]

Attachment: part1-calendar-custom-elements-curly-blocks-0.patch

Part 1 of 3. Wrap all calendar CEs in curly brace blocks. Put "[skip-blame]" in the commit message to skip this change when doing hg blame since it's mostly re-indentation.

Comment 2

[Paul Morris [:pmorris]]

Attachment: part2-tb-custom-elements-curly-blocks-0.patch

Part 2 of 3. Wrap all TB CEs in curly brace blocks. Put "[skip-blame]" in the commit message to skip this change when doing hg blame since it's mostly re-indentation.

Comment 3

[Paul Morris [:pmorris]]

Attachment: part3-redundant-menulist-code-0.patch

Part 3 of 3. I found some redundant code at the bottom and the top of this file.

Try server run looks fine. Test failures seem to be unrelated:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=efa72012c8e6bbbd50308fc689ec65d662517b86

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9085447 [details] [diff] [review]
part1-calendar-custom-elements-curly-blocks-0.patch

This looks fine based on a diff without white-space changes which I'm sure you did perfectly. ;-)

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9085447 [details] [diff] [review]
part1-calendar-custom-elements-curly-blocks-0.patch

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

Looks alright, though I thought the idea was to add the brackets and ignore the linting errors that would come from that, then check it in in the same go as the prettier run, so that prettier would take care of the re-indention where needed. Basically, the just a diff -uw of the changes here.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9085448 [details] [diff] [review]
part2-tb-custom-elements-curly-blocks-0.patch

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

Same thing for this I gather. It would also bitrot in no time as is.

Comment 7

[Jorg K (CEST = GMT+2)]

Hmm, I saw [skip-blame] in one of the patches. We need to research what the right thing is, for the C++ formatting I used # ignore-this-changeset. I think some of the source indexes ignore the latter under some circumstances.

Comment 8

[Paul Morris [:pmorris]]

mkmelin: Hm, sounds like I misunderstood. I was thinking it was an incremental step that could be done beforehand, to simplify the introduction of prettier. What are the reasons for waiting on this until the prettier run? (One benefit would be not having to do the indentation manually, but I guess I already did that...) I guess it complicates uplifting unless you apply it to the uplift targets, which creates more work.

Anyway, it sounds like you are saying I should redo these patches (using 'diff -uw') to remove the indentation changes, and then just apply them as part of the prettier run when that happens on Aug 29.

Comment 9

[Paul Morris [:pmorris]]

(In reply to Jorg K (GMT+2) from comment #7)

Hmm, I saw [skip-blame] in one of the patches. We need to research what the right thing is, for the C++ formatting I used # ignore-this-changeset. I think some of the source indexes ignore the latter under some circumstances.

I asked on IRC and we should indeed be using 'ignore-this-changeset', as described here (thanks Fallen):
https://lists.mozilla.org/pipermail/dev-platform/2019-January/023284.html

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #8)

Anyway, it sounds like you are saying I should redo these patches (using 'diff -uw') to remove the indentation changes, and then just apply them as part of the prettier run when that happens on Aug 29.

Yeah that would be what I suggest. Then there's no duplicate reformatting (if it's only indention).

Comment 11

[Paul Morris [:pmorris]]

Attachment: part1-calendar-custom-elements-curly-blocks-1.patch

Part 1/3 - Now without indentation/whitespace changes and rebased on current trunk.
Previous version of this had darktrojan and mkmelin's r+.

Comment 12

[Paul Morris [:pmorris]]

Attachment: part2-tb-custom-elements-curly-blocks-1.patch

Part 2/3 - Now without indentation/whitespace changes and rebased on current trunk.
Part 3/3 - Hasn't changed from the original part3 patch.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9088497 [details] [diff] [review]
part2-tb-custom-elements-curly-blocks-1.patch

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

Looks ok, though I think for the imports, you actually should use const ;)

Comment 14

[Paul Morris [:pmorris]]

Attachment: part1-calendar-custom-elements-curly-blocks-2.patch

1/3 Now with const instead of let for imports.

As you know, I'll gladly use const anywhere and anytime I can. :)

("If a variable is never reassigned, using the const declaration is better. const declaration tells readers, “this variable is never reassigned,” reducing cognitive load and improving maintainability." -- https://eslint.org/docs/rules/prefer-const )

Comment 15

[Paul Morris [:pmorris]]

Attachment: part2-tb-custom-elements-curly-blocks-2.patch

2/3 Now with const instead of let for imports.

Comment 16

[Paul Morris [:pmorris]]

Attachment: part3-redundant-menulist-code-2.patch

3/3

Comment 17

[Paul Morris [:pmorris]]

Attachment: part1-calendar-custom-elements-curly-blocks-3.patch

Rebased.

Comment 18

[Paul Morris [:pmorris]]

Attachment: part2-tb-custom-elements-curly-blocks-3.patch

Rebased.

Comment 19

[Paul Morris [:pmorris]]

Attachment: part3-redundant-menulist-code-3.patch

Rebased.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1660b26d3a84
Wrap all calendar custom elements in curly bracket blocks. r=darktrojan,mkmelin

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fda41b757b2e
Wrap all Thunderbird custom elements in curly bracket blocks. r=mkmelin
https://hg.mozilla.org/comm-central/rev/6727b142f50d
Remove redundant menulist creation code. r=mkmelin