824150 - Code cleanup in /mail/ and /mailnews/: Use new String methods like startsWith, endsWith, contains, remaining Services.jsm switches and querySelector use instead of NodeList calls

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: new string methods patch v1

This bug is about updating JavaScript code in /mail/ and /mailnews/:
- Use String methods startsWith, endsWith and contains instead of regular expressions, indexOf, lastIndexOf, substr, substring etc.
- Replace patterns like getElementsByFoo(condition)[0] with querySelector(css selector)
- Convert remaining interfaces where possible to Services.jsm, this mainly affects tests.

The patch is work in progress, e.g. keyword detection for missing attachments doesn't work for composing new messages, but seems to work in replies. There are also gloda fails. A recent Thunderbird-Try run is https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=5aa64016a08c
The patch will be split to ease review and unblock work in other bugslike bug 824104, file changes shouldn't affect other files.

Comment 1

[:aceman]

This subtly changes the behaviour, as it now matches also when the string is not as the first word in the class value. But I think this new version is more robust.
-                 ((node.getAttribute("class").split(" ")[0] != "bar") &&
-                  (node.getAttribute("class").split(" ")[0] != "facet-more") &&
-                  (node.getAttribute("class").split(" ")[0] != "facet-content")))
+                 (!node.classList.contains("bar") &&
+                  !node.classList.contains("facet-more") &&
+                  !node.classList.contains("facet-content")))

Does this intentionally check if the start of the class value is "moz-attached-image":
-      if (/^moz-attached-image/.test(target.className)) {
+      if (target.className.startsWith("moz-attached-image")) {

Does it also want to match moz-attached-image* ? If not, you should probably also change it to node.classList.contains. Of course, if you can't answer this, then you can leave your change as is.

Shouldn't this be querySelectorAll?
-    var formNodes = document.getElementById('messagepane').contentDocument.getElementsByTagName("form");
+    var formNodes = document.getElementById('messagepane').contentDocument.querySelector("form[action]");

+++ b/mail/base/test/unit/test_windows_font_migration.js
-var gPrefBranch = Cc["@mozilla.org/preferences-service;1"]
-                    .getService(Ci.nsIPrefBranch);
+Components.utils.import("resource://gre/modules/Services.jsm");

Is gPrefBranch unsused in that file? As you do not remove it anywhere in that file.

+++ b/mail/components/about-support/content/export.js
-    if (className.indexOf(CLASS_DATA_UIONLY) != -1) {
+    if (className.contains(CLASS_DATA_UIONLY)) {
-    else if (aHidePrivateData && className.indexOf(CLASS_DATA_PRIVATE) != -1) {
+    else if (aHidePrivateData && className.contains(CLASS_DATA_PRIVATE)) {
-    else if (!aHidePrivateData && className.indexOf(CLASS_DATA_PUBLIC) != -1) {
+    else if (!aHidePrivateData && className.contains(CLASS_DATA_PUBLIC)) {

Candidates for node.classList.contains ?

+++ b/mail/components/activity/modules/autosync.js
     for (let i = 1; i < this._inQFolderList.length; i++) {
       // do not include already existing account names
-      if (accountList.search(this._inQFolderList[i].server.prettyName) == -1)
+      if (!accountList.contains(this._inQFolderList[i].server.prettyName))

I don't like code like this as it assumes one .server.prettyName is not contained in some other one. Not sure if that is safe (I think prettyName relies on user supplied data). I'd prefer to handle this as an array of prettyNames, not a string.

+++ b/mail/components/compose/content/MsgComposeCommands.js
-    let spans = mailBodyNode.getElementsByTagName("span");
+    let spans = mailBodyNode.querySelector("span[_moz_quote]");
     for (let i = spans.length - 1; i >= 0; i--) {
-      if (spans[i].hasAttribute("_moz_quote"))
-        spans[i].parentNode.removeChild(spans[i]);
+      spans[i].parentNode.removeChild(spans[i]);
.querySelectorAll ?

+++ b/mail/components/compose/content/addressingWidgetOverlay.js
-      var inputID = item.getElementsByTagName(awInputElementName())[0].getAttribute("id").split("#")[1];
-      var popupID = item.getElementsByTagName(awSelectElementName())[0].getAttribute("id").split("#")[1];
+      var inputID = item.querySelector(awInputElementName()).getAttribute("id").split("#")[1];
+      var popupID = item.querySelector(awSelectElementName()).getAttribute("id").split("#")[1];
       if (inputID != i || popupID != i)
item.querySelector(awSelectElementName()).id.startsWith("#" + i) ?

+++ b/mail/components/im/content/imconversation.xml
-              if (/^\/me /.test(text))
+              if (text.startsWith("/me"))
text.startsWith("/me ")) ?

-          modifiers |= (navigator.platform.indexOf("Mac") >= 0) ? masks.META_MASK
-                                                                : masks.CONTROL_MASK;
+          modifiers |= (navigator.platform.contains("Mac") >= 0) ? masks.META_MASK
+                                                                 : masks.CONTROL_MASK;
Surplus '>= 0' ?

More nits in the next comment.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): about:support, v1

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): account creation, v1

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): activity manager, v1

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): add-ons, v1

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): addressbook, v1

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): chat, v1

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): cloudfile, v1

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): compose, v1

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): fakeserver, v1

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): filter, v1

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): gloda, v1

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): imap, v1

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): import, v1

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): internals, v1

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): local (/mailnews/local/), v1

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): message window including content tabs, v1

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): newsblog, v1

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): news, v1

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): preferences, v1

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): test: performance and resources, v1

Comment 22

[:aceman]

-          lastName.lastIndexOf(firstWord, 0) == 0)))
+          lastName.startsWith(firstWord))))

I don't think these are equivalent (your new code does not ensure firstWord does not appear later in the string). [Several occurences.]

+++ b/mailnews/base/content/folderWidgets.xml
-          if (/wrapper-.*/.test(node.id)) {
+          if (node.id.contains("wrapper-")) {

Please make this .startsWith as the "wrapper-" we are looking for must be at the start only.

(I have not looked at all the test files, it was too much for now.)

Great work otherwise, thanks for these global refinements.

Comment 23

[:aceman]

> +++ b/mail/components/compose/content/addressingWidgetOverlay.js
> -      var inputID =
> item.getElementsByTagName(awInputElementName())[0].getAttribute("id").
> split("#")[1];
> -      var popupID =
> item.getElementsByTagName(awSelectElementName())[0].getAttribute("id").
> split("#")[1];
> +      var inputID =
> item.querySelector(awInputElementName()).getAttribute("id").split("#")[1];
> +      var popupID =
> item.querySelector(awSelectElementName()).getAttribute("id").split("#")[1];
>        if (inputID != i || popupID != i)
> item.querySelector(awSelectElementName()).id.startsWith("#" + i) ?

Ok, this nit is not valid as you need value of popupID later in the error message. But the .getAttribute("id") could be changed to .id. ?

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to :aceman from comment #23)
> > +++ b/mail/components/compose/content/addressingWidgetOverlay.js
> > -      var inputID =
> > item.getElementsByTagName(awInputElementName())[0].getAttribute("id").
> > split("#")[1];
> > -      var popupID =
> > item.getElementsByTagName(awSelectElementName())[0].getAttribute("id").
> > split("#")[1];
> > +      var inputID =
> > item.querySelector(awInputElementName()).getAttribute("id").split("#")[1];
> > +      var popupID =
> > item.querySelector(awSelectElementName()).getAttribute("id").split("#")[1];
> >        if (inputID != i || popupID != i)
> > item.querySelector(awSelectElementName()).id.startsWith("#" + i) ?
> 
> Ok, this nit is not valid as you need value of popupID later in the error
> message. But the .getAttribute("id") could be changed to .id. ?

I changed it to
      var popupID = item.querySelector(awSelectElementName()).id.split("#")[1];
There are no performance gains for this, I think this could be done together with for ... of - switching now between patches for something different than fixing bugs is more of a burden.
And startsWith(...) also doesn't work because the id looks like this: id="addressCol1#1"

(In reply to :aceman from comment #22)
> -          lastName.lastIndexOf(firstWord, 0) == 0)))
> +          lastName.startsWith(firstWord))))
> 
> I don't think these are equivalent (your new code does not ensure firstWord
> does not appear later in the string). [Several occurences.]
No, the previous code checks only the start because the search is performed from right to left. See the documentation at https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/lastIndexOf :

Syntax
  string.lastIndexOf(searchValue[, fromIndex])

Parameters
  searchValue
    A string representing the value to search for.
  fromIndex
    The location within the calling string to start the search from, indexed from left to right. It can be any integer between 0 and the length of the string. The default value is the length of the string. 

> +++ b/mailnews/base/content/folderWidgets.xml
> -          if (/wrapper-.*/.test(node.id)) {
> +          if (node.id.contains("wrapper-")) {
> 
> Please make this .startsWith as the "wrapper-" we are looking for must be at
> the start only.
Ok, trusting you on this ;)


(In reply to :aceman from comment #1)
> This subtly changes the behaviour, as it now matches also when the string is
> not as the first word in the class value. But I think this new version is
> more robust.
> -                 ((node.getAttribute("class").split(" ")[0] != "bar") &&
> -                  (node.getAttribute("class").split(" ")[0] !=
> "facet-more") &&
> -                  (node.getAttribute("class").split(" ")[0] !=
> "facet-content")))
> +                 (!node.classList.contains("bar") &&
> +                  !node.classList.contains("facet-more") &&
> +                  !node.classList.contains("facet-content")))
These are all in http://mxr.mozilla.org/comm-central/source/mail/base/content/glodaFacetBindings.xml and it looks robust the new way (it also seems that it's always only one class).

> Does this intentionally check if the start of the class value is
> "moz-attached-image":
> -      if (/^moz-attached-image/.test(target.className)) {
> +      if (target.className.startsWith("moz-attached-image")) {
> 
> Does it also want to match moz-attached-image* ? If not, you should probably
> also change it to node.classList.contains. Of course, if you can't answer
> this, then you can leave your change as is.
Changed it, also seems to be the only class here.

> Shouldn't this be querySelectorAll?
> -    var formNodes =
> document.getElementById('messagepane').contentDocument.
> getElementsByTagName("form");
> +    var formNodes =
> document.getElementById('messagepane').contentDocument.
> querySelector("form[action]");
Thank you for the catch.

> +++ b/mail/base/test/unit/test_windows_font_migration.js
> -var gPrefBranch = Cc["@mozilla.org/preferences-service;1"]
> -                    .getService(Ci.nsIPrefBranch);
> +Components.utils.import("resource://gre/modules/Services.jsm");
> 
> Is gPrefBranch unsused in that file? As you do not remove it anywhere in
> that file.
Uh, seems like a closed the file half-way
> 
> +++ b/mail/components/about-support/content/export.js
> -    if (className.indexOf(CLASS_DATA_UIONLY) != -1) {
> +    if (className.contains(CLASS_DATA_UIONLY)) {
> -    else if (aHidePrivateData && className.indexOf(CLASS_DATA_PRIVATE) !=
> -1) {
> +    else if (aHidePrivateData && className.contains(CLASS_DATA_PRIVATE)) {
> -    else if (!aHidePrivateData && className.indexOf(CLASS_DATA_PUBLIC) !=
> -1) {
> +    else if (!aHidePrivateData && className.contains(CLASS_DATA_PUBLIC)) {
> 
> Candidates for node.classList.contains ?
Done.

> +++ b/mail/components/activity/modules/autosync.js
>      for (let i = 1; i < this._inQFolderList.length; i++) {
>        // do not include already existing account names
> -      if (accountList.search(this._inQFolderList[i].server.prettyName) ==
> -1)
> +      if (!accountList.contains(this._inQFolderList[i].server.prettyName))
> 
> I don't like code like this as it assumes one .server.prettyName is not
> contained in some other one. Not sure if that is safe (I think prettyName
> relies on user supplied data). I'd prefer to handle this as an array of
> prettyNames, not a string.
mxr only finds the definition of this method (getAccountListString), but no calls. The same applies to getFolderListString. This needs some info, maybe from bwinton or Standard8 (or mconley?)
Leaving it unchanged for the moment.

> +++ b/mail/components/compose/content/MsgComposeCommands.js
> -    let spans = mailBodyNode.getElementsByTagName("span");
> +    let spans = mailBodyNode.querySelector("span[_moz_quote]");
>      for (let i = spans.length - 1; i >= 0; i--) {
> -      if (spans[i].hasAttribute("_moz_quote"))
> -        spans[i].parentNode.removeChild(spans[i]);
> +      spans[i].parentNode.removeChild(spans[i]);
> .querySelectorAll ?
Of course, thank you.

> +++ b/mail/components/im/content/imconversation.xml
> -              if (/^\/me /.test(text))
> +              if (text.startsWith("/me"))
> text.startsWith("/me ")) ?
Fixed.

> -          modifiers |= (navigator.platform.indexOf("Mac") >= 0) ?
> masks.META_MASK
> -                                                                :
> masks.CONTROL_MASK;
> +          modifiers |= (navigator.platform.contains("Mac") >= 0) ?
> masks.META_MASK
> +                                                                 :
> masks.CONTROL_MASK;
> Surplus '>= 0' ?
Fixed.

Try run with most of the patches: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3d41e23b4bec

Comment 25

[:aceman]

(In reply to Archaeopteryx [:aryx] from comment #24)
> (In reply to :aceman from comment #22)
> > -          lastName.lastIndexOf(firstWord, 0) == 0)))
> > +          lastName.startsWith(firstWord))))
> > 
> > I don't think these are equivalent (your new code does not ensure firstWord
> > does not appear later in the string). [Several occurences.]
> No, the previous code checks only the start because the search is performed
> from right to left.
OK, I missed the 0 argument :)

> > +++ b/mail/components/activity/modules/autosync.js
> >      for (let i = 1; i < this._inQFolderList.length; i++) {
> >        // do not include already existing account names
> > -      if (accountList.search(this._inQFolderList[i].server.prettyName) ==
> > -1)
> > +      if (!accountList.contains(this._inQFolderList[i].server.prettyName))
> > 
> > I don't like code like this as it assumes one .server.prettyName is not
> > contained in some other one. Not sure if that is safe (I think prettyName
> > relies on user supplied data). I'd prefer to handle this as an array of
> > prettyNames, not a string.
> mxr only finds the definition of this method (getAccountListString), but no
> calls. The same applies to getFolderListString. This needs some info, maybe
> from bwinton or Standard8 (or mconley?)
> Leaving it unchanged for the moment.
Yes, this would be harder to change/fix and you are free to decide this is outside the scope of this bug.

> Try run with most of the patches:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3d41e23b4bec
This run is after the patches were fixed according to my comments? You have not yet attached the new versions?

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): about:support, v2

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): activity manager, v2

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): chat, v2

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): compose, v2

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): internals, v2

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch (work in progress): message window including content tabs, v2

Try run with updated patches: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=90919b3771c6

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: about:support, v3, patch

Successful Thunderbird-Try run with the patches I am now uploading in this bug applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b8efc032eeb1

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: account creation, v2, patch

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: activity manager, v3, patch

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: add-ons, v2, patch

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: addressbook, v2, patch

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat, v3, patch

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: cloudfile, v2, patch

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: filter, v2, patch

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: import, v2, patch

Comment 41

[Florian Quèze [:florian]]

Comment on attachment 698256 [details] [diff] [review]
chat, v3, patch

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

Looks fine, thanks for cleaning up this code!
I just have a tiny nit.

::: mail/components/im/content/imContextMenu.js
@@ +236,5 @@
>  
>      href = this.link.getAttributeNS("http://www.w3.org/1999/xlink",
>                                      "href");
>  
> +    if (!href || (href.trim() == "")) {

It's a bit strange to do the first empty check as a null check, and the second by comparing to "". Maybe use:
if (!href || !href.trim()) {

I think the "!href ||" part was added to avoid the cost of the regexp in some cases, but trim shouldn't be that expensive, so maybe we can even simplify to:
if (!href.trim()) {

(This pattern is repeated 3 other times in the linkText function.)

Comment 42

[:aceman]

Could href be null so that href.trim fails?

Comment 43

[Florian Quèze [:florian]]

(In reply to :aceman from comment #42)
> Could href be null so that href.trim fails?

Good catch, thanks for checking!

* getAttribute and getAttributeNS return null if the attribute isn't set, so please keep the null check for these values.

* gatherTextUnder always returns a string, and that string is already trimmed: http://mxr.mozilla.org/comm-central/source/mail/base/content/utilityOverlay.js#57
So one line can be simplified in the linkText function.

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat, v4, r=florian [checkin: comment 64]

Thank you for the review, addressed your last comment, but kept two (!text || (text.trim() == "")) because I felt uneasy calling NOT on a non boolean value (string of length 0). Alternative would be (!text || !text.trim())

Comment 45

[Kent James (:rkent)]

There has apparently been some controversy concerning .contains() (Bug 789036, and Bug 793781 backs this out in Moz17). Is this change in mail code perhaps a little preliminary? I'm not saying it is, but I'd like to hear from others.

Comment 46

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

As far as I can see in Bug 793781, Firefox 18 will ship tomorrow with the String.prototype.contains function and this patch is for (likely) Gecko 24, so I see no issue here.

Comment 47

[:aceman]

This patch will run on the gecko that is current at the time of landing. Maybe 21, so if they pull it from there comm-central will break. But yes, they decided to NOT remove 'contains' from FF18 and 19.

Comment 48

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: internals, v3, patch

Try run with the same oranges like usual: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=2bc26793024c&noignore=1

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: message window including content tabs, v3

Successful Try run with the common oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4bd3ef3be8dd&noignore=1

Comment 50

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: local (/mailnews/local/), v2

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=add525660cd3

Comment 51

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: newsblog, v2, patch

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=8e2caf9715a1

Comment 52

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: news, v2 [ready for check-in]

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=b16ec7e3ba38

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: preferences, v2

Successful Try run with the known oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=85b44393f82f

Comment 54

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: preferences, v3, patch

Attached wrong file before.

Comment 55

[:aceman]

Aryx, do you plan to check in each patch immediatelly after it gets review? Otherwise somebody will always bitrot you.

Could you find out if .querySelector('*[command]')) is equivalent to .querySelector('[command]')) and if yes use the shorter version (if the JS guys prefer that)?

Comment 56

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: compose, v3, patch

Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=b86eb4432ea9
I fixed the Mozmill fail and ran the failing test file locally (it passed).

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698250 [details] [diff] [review]
account creation, v2, patch

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

Looks good - just a few tiny nits. r=me with those fixed.

::: mailnews/base/prefs/content/AccountWizard.js
@@ +138,5 @@
>      // really cancel if the user hits the "cancel" button
>      if (accountCount < 1) {
>        var confirmMsg = gPrefsBundle.getString("cancelWizard");
>        var confirmTitle = gPrefsBundle.getString("accountWizard");
> +      var result = Services.prompt.confirmEx(window, confirmTitle, confirmMsg,

let instead of var

@@ +139,5 @@
>      if (accountCount < 1) {
>        var confirmMsg = gPrefsBundle.getString("cancelWizard");
>        var confirmTitle = gPrefsBundle.getString("accountWizard");
> +      var result = Services.prompt.confirmEx(window, confirmTitle, confirmMsg,
> +        (Services.prompt.BUTTON_TITLE_IS_STRING*Services.prompt.BUTTON_POS_0)+

spaces on either side of * and +

@@ +140,5 @@
>        var confirmMsg = gPrefsBundle.getString("cancelWizard");
>        var confirmTitle = gPrefsBundle.getString("accountWizard");
> +      var result = Services.prompt.confirmEx(window, confirmTitle, confirmMsg,
> +        (Services.prompt.BUTTON_TITLE_IS_STRING*Services.prompt.BUTTON_POS_0)+
> +        (Services.prompt.BUTTON_TITLE_IS_STRING*Services.prompt.BUTTON_POS_1),

spaces on either side of *

@@ +974,5 @@
>  }
>  
>  // flush the XUL cache - just for debugging purposes - not called
>  function onFlush() {
> +        Services.prefs.setBoolPref("nglayout.debug.disable_xul_cache", true);

While you're here, let's fix the indentation on these two lines.

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +1346,5 @@
>  
>      gEmailWizardLogger.info("creating account in backend");
>      var newAccount = createAccountInBackend(configFilledIn);
>  
> +    var existingAccountManager = Services.wm

let instead of var

::: mailnews/base/prefs/content/accountcreation/fetchConfig.js
@@ +161,5 @@
>    sucAbortable.current = getMX(domain,
>      function(mxHostname) // success
>      {
>        ddump("getmx took " + (Date.now() - time) + "ms");
> +      var sld = Services.eTLD.getBaseDomainFromHost(mxHostname);

let instead of var

::: mailnews/base/prefs/content/accountcreation/guessConfig.js
@@ +617,5 @@
>      if (new RegExp(prefix + "CRAM-MD5").test(line))
>        result.push(Ci.nsMsgAuthMethod.passwordEncrypted);
>      if (new RegExp(prefix + "(NTLM|MSN)").test(line))
>        result.push(Ci.nsMsgAuthMethod.NTLM);
> +    if ( ! (protocol == IMAP && line.contains("LOGINDISABLED")))

We don't need the big spaces on either side of !

::: mailnews/base/prefs/content/accountcreation/util.js
@@ +73,5 @@
>  function readURLasUTF8(uri)
>  {
>    assert(uri instanceof Ci.nsIURI, "uri must be an nsIURI");
>    try {
> +    var chan = Services.io.newChannelFromURI(uri);

let instead of var

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698254 [details] [diff] [review]
addressbook, v2, patch

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

Looks good! Just some nits - also, not sure we need String in some places.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +594,5 @@
>  
>  function CleanUpWebPage(webPage)
>  {
>    // no :// yet so we should add something
> +  if ( webPage.length && !webPage.contains("://") )

We don't need spaces after ( or before )

@@ +599,3 @@
>    {
>      // check for missing / on http://
> +    if ( webPage.startsWith("http:/") )

Same as above.

::: mail/components/addrbook/content/abCommon.js
@@ +525,5 @@
>        sortDirection = gAbView.sortDirection;
>      }
>  
>      // this approach is necessary to support generic columns that get overlayed.
> +    var elements = document.querySelectorAll('*[name="sortas"]');

let instead of var

::: mailnews/addrbook/content/abMailListDialog.js
@@ +263,5 @@
>      if (total)
>      {
>        var listbox = document.getElementById('addressingWidget');
>        var newListBoxNode = listbox.cloneNode(false);
> +      var templateNode = listbox.querySelector("listitem");

let instead of var

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +239,5 @@
>  // disables all the text fields corresponding to the .uri pref.
>  function DisableUriFields(aPrefName)
>  {
>    if (Services.prefs.prefIsLocked(aPrefName)) {
> +    var lockedElements = document.querySelectorAll('*[disableiflocked="true"]');

let instead of var

::: mailnews/addrbook/test/unit/test_db_enumerator.js
@@ +24,5 @@
>          let dir = MailServices.ab.getDirectory(uri);
>  
>          dump("considering: j: " + j + " " + elem.dirName + "\n");
>  
> +        if (j == 1 && String(elem.dirName).startsWith(ab_prefix)) {

shouldn't elem.dirName.startsWith(ab_prefix) be sufficient?

@@ +50,5 @@
>      let elem = enm_dirs.getNext().QueryInterface(Ci.nsIAbDirectory);
>      let uri  = elem.URI;
>      let dir  = MailServices.ab.getDirectory(uri);
>  
> +    if (String(elem.dirName).startsWith(ab_prefix)) {

Are we sure we need String here?

@@ +81,5 @@
>  
>    while (enm_dirs.hasMoreElements()) {
>      let elem = enm_dirs.getNext().QueryInterface(Ci.nsIAbDirectory);
>  
> +    if (String(elem.dirName).startsWith(ab_prefix)) {

Same as above - do we need String? If not, remove it.

Comment 59

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: addressbook, v3, patch

> shouldn't elem.dirName.startsWith(ab_prefix) be sufficient?
nsIAbDirectory.dirName is a string, so I changed String(elem.dirName) to elem.dirName.

Comment 60

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: news, v3. r=jcranmer [checkin: comment 64]

Comment 61

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: account creation, v3, r=mconley [checkin: comment 64]

Comment 62

[Kent James (:rkent)]

Comment on attachment 698258 [details] [diff] [review]
filter, v2, patch

Thanks for the patch!

Comment 63

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: filter, v3 [checkin: comment 64]

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/11dc139c9e53
https://hg.mozilla.org/comm-central/rev/7feccbd27039
https://hg.mozilla.org/comm-central/rev/2968b7dbde32
https://hg.mozilla.org/comm-central/rev/a012dd84980d

Comment 65

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: fakeserver, v2

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=e5d7f51c80e7

Comment 66

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: gloda, v2

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=ab5f667af62f

Comment 67

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: imap, v2

Comment 68

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Try run with the patch applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=9c5d34b1de14

Comment 69

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: test: performance and resources, v2

Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=9c5d34b1de14

Comment 70

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698257 [details] [diff] [review]
cloudfile, v2, patch

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

One nit - otherwise, looks great. Thanks!

::: mail/components/cloudfile/cloudFileAccounts.js
@@ +31,5 @@
>      let accountKeySet = {};
>      let branch = Services.prefs.getBranch(ACCOUNT_ROOT);
>      let children = branch.getChildList("", {});
>      for (let [,child] in Iterator(children)) {
> +      let subbranch = (child.split("."))[0];

Nit - I think we can safely reduce the parentheses noise and just use

child.split(".")[0];

Nice simplification!

Comment 71

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: cloudfile, v3, r=mconley [ready for checkin, r+ in comment #70]

Comment 72

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 701718 [details] [diff] [review]
preferences, v3, patch

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

Just some style nits, but overall this looks good. r=me with these fixed. Thanks!

::: mail/components/preferences/applications.js
@@ +1256,5 @@
>      var lastSelectedType = this._list.getAttribute("lastSelectedType");
>      if (!lastSelectedType)
>        return;
>  
> +    var item = this._list.querySelector('*[type="' + lastSelectedType + '"]');

let, not var

::: mail/components/preferences/cookies.js
@@ +445,5 @@
>    },
>  
>    _makeStrippedHost: function (aHost)
>    {
> +    var formattedHost = aHost.startsWith(".") ? aHost.substring(1, aHost.length) : aHost;

let, not var

@@ +468,5 @@
>  
>    _makeCookieObject: function (aStrippedHost, aCookie)
>    {
>      var host = aCookie.host;
> +    var formattedHost = host.startsWith(".") ? host.substring(1, host.length) : host;

let, not var

::: mail/components/preferences/fonts.js
@@ +73,2 @@
>  
>    readFontSelection: function (aElement)

Ugh, isn't this more or less the same function from display.js? :( Booo...

@@ +77,5 @@
>      // - there is no setting
>      // - the font selected by the user is no longer present (e.g. deleted from
>      //   fonts folder)
>      var preference = document.getElementById(aElement.getAttribute("preference"));
> +    var fontItem;

let, not var

::: mail/components/preferences/permissions.js
@@ +98,5 @@
>        }
>      }
>  
>      if (!exists) {
> +      host = host.startsWith(".") ? host.substring(1,host.length) : host;

nit: space after comma

::: mail/components/preferences/sendoptions.js
@@ +90,5 @@
>    },
>  
>    domainAlreadyPresent: function(aDomainName)
>    {
> +    var matchingDomains = this.mHTMLListBox.querySelectorAll('*[label="' + aDomainName + '"]');

let, not var

::: mail/test/resources/mozmill/test/test_prefs.js
@@ +49,5 @@
>    
>    // // Click on the search box
>    // var node = controller.window.document.getAnonymousElementByAttribute(
> +  //    controller.window.document.getElementById('paneApplications').querySelector(
> +  //     'hbox').querySelector('textbox'), 

Please clean up the trailing whitespace in here.

::: mailnews/base/prefs/content/AccountManager.js
@@ +72,5 @@
>    }
>  }
>  
>  function hideShowControls(serverType) {
> +  var controls = document.querySelectorAll("*[hidefor]");

let, not var

Comment 73

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: cloudfile, v4, r=mconley [checkin: comment 74]

Old patch had wrong checkin comment.

Comment 74

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: preferences, v4, r=mconley [ready for checkin, r+ in comment #72]

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 709788 [details] [diff] [review]
cloudfile, v4, r=mconley [checkin: comment 74]

v3 was already in my checkin queue and I didn't see that a new patch had been posted, so that's what landed :(

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

Comment 76

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: preferences, v5, r=mconley [checkin: comment 77]

Updated patch for preferences, got bitrotted by aceman's http://hg.mozilla.org/comm-central/rev/35d92e1faf46

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 709795 [details] [diff] [review]
preferences, v5, r=mconley [checkin: comment 77]

https://hg.mozilla.org/comm-central/rev/89fa8b687178

Comment 78

[Rain [:rain1] (inactive)]

Comment on attachment 698249 [details] [diff] [review]
about:support, v3, patch

I'm not going to have the time to review this for a while, sorry.

Comment 79

[Florian Quèze [:florian]]

Comment on attachment 698249 [details] [diff] [review]
about:support, v3, patch

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

During today's Thunderbird status meeting, Mike said "feel free to steal reviews from my queue"; I had already looked at this patch this morning when I saw comment 78, so I asked if I could r+ a patch from his queue even though it wasn't in a module I'm a peer of, he said to use my best judgment but that in this case I should likely just "go for it". So r=me with the nit addressed.

::: mail/components/about-support/content/export.js
@@ +105,5 @@
>  
>  function cleanUpText(aElem, aHidePrivateData) {
>    let node = aElem.firstChild;
>    while (node) {
> +    let classList = ("classList" in node && node.classList);

Coding style nit: These parentheses can be removed, right?

Comment 80

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 698251 [details] [diff] [review]
activity manager, v3, patch

rs=me.  Thanks, and thanks to Florian for mentioning the bug.  ;)

Comment 81

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: about:support, v4, r=florian [checkin: comment 85]

Comment 82

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: activity manager, v4, r=bwinton (ready for check-in)

Comment 83

[Florian Quèze [:florian]]

Comment on attachment 710406 [details] [diff] [review]
activity manager, v4, r=bwinton (ready for check-in)

># HG changeset patch
># User Sebastian Hengst <[e-mail]>
># Date 1360106104 -3600
># Node ID 4aa4a6f593acc20b820af483007b2a97beb71f74
># Parent  a83a817f750c2aaf2ec37d08e3c8ef929f967a25
>Bug 824150 - Code cleanup in /editor/: Use new String methods like startsWith, endsWith, contains, remaining Services.jsm switches and querySelector use instead of NodeList calls: activity manager. r=florian

The reviewer for this patch is bwinton, not me ;).

Comment 84

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: activity manager, v5, r=bwinton [checkin: comment 85]

Thank you for the catch

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/bca970870c77
https://hg.mozilla.org/comm-central/rev/cca94582cef4

Comment 86

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: add-ons, v3, r=Standard8 [checkin: comment 88]

Comment 87

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: import, v3, r=Standard8 [checkin: comment 88]

Comment 88

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/cd2d38507626
https://hg.mozilla.org/comm-central/rev/c39b215c927a

Comment 89

[Mark Banner (:standard8)]

Comment on attachment 701707 [details] [diff] [review]
local (/mailnews/local/), v2

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

::: mailnews/local/test/unit/test_streamHeaders.js
@@ +58,5 @@
>    messageService.streamHeaders(uri, createStreamListener(
>      function theString(k) {
>        dump('the string:\n' + k + '\n');
>        // The message contains this header
> +      do_check_true(k.contains("X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0", 1));

I don't understand the addition of ', 1' here, I don't think it is necessary or wanted...

Comment 90

[Mark Banner (:standard8)]

Comment on attachment 704523 [details] [diff] [review]
fakeserver, v2

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

r=me assuming this passes try server.

Comment 91

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: local (/mailnews/local/), v3

Thank you for the review, the only change is that the stream headers test now checks for the header from the first byte and not from the second one.

Comment 92

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 701908 [details] [diff] [review]
compose, v3, patch

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

Hey Sebastian - looks great! Just a few nits. r=me with these fixed.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1942,5 @@
>  // for the url and returns them.
>  function handleMailtoArgs(mailtoUrl)
>  {
>    // see if the string is a mailto url....do this by checking the first 7 characters of the string
> +  if (mailtoUrl.toLowerCase().startsWith("mailto:"))

Why the toLowerCase()? Do we want to be able to handle cases other than just mailto: in all lowercase here? This change seems orthogonal to the stated purpose of this bug.

@@ +2680,5 @@
>      }
>  
>      // Strip trailing spaces and long consecutive WSP sequences from the
>      // subject line to prevent getting only WSP chars on a folded line.
> +    var fixedSubject = subject.replace(/\s{74,}/g, "    ").trimRight();

let, not var

@@ +3212,5 @@
>  {
>    InitLanguageMenu();
>    var spellChecker = gSpellChecker.mInlineSpellChecker.spellChecker;
>    var curLang = spellChecker.GetCurrentDictionary();
> +  var language = aTarget.querySelector('*[value="' + curLang + '"]');

let, not var

@@ +4261,5 @@
>              item.flavour.contentType == "application/x-moz-file")
>          {
>            if (item.flavour.contentType == "application/x-moz-file")
>            {
> +            var fileHandler = Services.io.getProtocolHandler("file")

I think I'd prefer:

let fileHandler = Services.io
                          .getProtocolHandler("file")
                          .QueryInterface(Components.interfaces.nsIFileProtocolHandler);

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +161,5 @@
>      var listbox = document.getElementById('addressingWidget');
>      var newListBoxNode = listbox.cloneNode(false);
>      var listBoxColsClone = listbox.firstChild.cloneNode(true);
>      newListBoxNode.appendChild(listBoxColsClone);
> +    var templateNode = listbox.querySelector("listitem");

let, not var

@@ +364,5 @@
>    {
>      for (var i = 1; i <= listitems.length; i ++)
>      {
>        var item = listitems [i - 1];
> +      var inputID = item.querySelector(awInputElementName()).id.split("#")[1];

let, not var for these two lines.

@@ +920,5 @@
>    var listbox = document.getElementById("addressingWidget");
>    var listboxHeight = listbox.boxObject.height;
>  
>    // remove rows to remove scrollbar
> +  var kids = listbox.querySelectorAll('*[_isDummyRow]');

let, not var

@@ +981,5 @@
>  
>  function awGetNextDummyRow()
>  {
>    // gets the next row from the top down
> +  var aKid = document.querySelector('#addressingWidget > *[_isDummyRow]');

let, not var. Nice clean-up here.

::: mail/test/mozmill/attachment/test-attachment.js
@@ +15,5 @@
>  var elib = {};
>  Cu.import('resource://mozmill/modules/elementslib.js', elib);
>  var EventUtils = {};
>  Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);
>  

Probably don't need the newline here.

::: mail/test/mozmill/composition/test-eml-actions.js
@@ +114,4 @@
>      throw new Error("body text didn't contain the decoded text; message=" +
>                      message + ", bodyText=" + bodyText);
>  
>    close_compose_window(compWin); 

Please trim the whitespace from these two lines while you're here.

@@ +137,4 @@
>      throw new Error("body text didn't contain the decoded text; message=" +
>                      message + ", bodyText=" + bodyText);
>  
>    close_compose_window(compWin); 

And trim the whitespace from these two lines as well, please.

::: mail/test/mozmill/composition/test-forwarded-content.js
@@ +54,1 @@
>      throw new Error("Subject not set correctly in header table: subject=" + 

Please trim the trailing whitespace while you're here.

::: mail/test/mozmill/composition/test-forwarded-eml-actions.js
@@ +98,5 @@
>  
>    plan_for_new_window("msgcompose");
>    msgWin.keypress(null, hotkeyToHit, hotkeyModifiers);
>    let compWin = wait_for_compose_window(msgWin);
>    

Please trim the trailing whitespace while you're here.

::: mail/test/mozmill/composition/test-signature-updating.js
@@ +21,5 @@
>  var jumlib = {};
>  Components.utils.import("resource://mozmill/modules/jum.js", jumlib);
>  var elib = {};
>  Components.utils.import("resource://mozmill/modules/elementslib.js", elib);
>  

No newline necessary here.

Comment 93

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: compose, v4, r=mconley [checkin: comment 97]

(In reply to Mike Conley (:mconley) from comment #92)
> > +  if (mailtoUrl.toLowerCase().startsWith("mailto:"))
> 
> Why the toLowerCase()? Do we want to be able to handle cases other than just
> mailto: in all lowercase here? This change seems orthogonal to the stated
> purpose of this bug.
This was at least a code regression from my patch in bug 794909 and so gets fixed. (Performance of startsWith with toLowerCase is slower depending on the length of the string, without toLowerCase faster.)

> ::: mail/test/mozmill/composition/test-signature-updating.js
> @@ +21,5 @@
> >  var jumlib = {};
> >  Components.utils.import("resource://mozmill/modules/jum.js", jumlib);
> >  var elib = {};
> >  Components.utils.import("resource://mozmill/modules/elementslib.js", elib);
> >  
> 
> No newline necessary here.

Comment 94

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: fakeserver, v3, r=Standard8 [checkin: comment 97]

Patch passes Try server: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=93afab1873d8

Comment 95

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: newsblog, v3, r=Standard8 [checkin: comment 97]

Comment 96

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: test: performance and resources, v3, r=Standard8 [checkin: comment 97]

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/aeb1127152ac
https://hg.mozilla.org/comm-central/rev/9e7319029574
https://hg.mozilla.org/comm-central/rev/59e6970aea76
https://hg.mozilla.org/comm-central/rev/e89c30d54a1e

Comment 98

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 702444 [details] [diff] [review]
addressbook, v3, patch

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

Looks good! r=me with the following changes.

::: mail/components/addrbook/content/abCommon.js
@@ +526,5 @@
>      }
>  
>      // this approach is necessary to support generic columns that get overlayed.
> +    let elements = document.querySelectorAll('[name="sortas"]');
> +    for (let i=0; i<elements.length; i++) {

While you're here, spaces on either side of = and <

@@ +527,5 @@
>  
>      // this approach is necessary to support generic columns that get overlayed.
> +    let elements = document.querySelectorAll('[name="sortas"]');
> +    for (let i=0; i<elements.length; i++) {
> +        let cmd = elements[i].id;

Two space indentation for lines 531-533

::: mailnews/addrbook/content/abMailListDialog.js
@@ +268,3 @@
>  
>        top.MAX_RECIPIENTS = 0;
> +      for (let i = 0;  i < total; i++)

nit - only one space after ;

@@ +426,1 @@
>    {  

Please remove this trailing whitespace while you're here.

@@ +433,5 @@
>      
>      top.MAX_RECIPIENTS++;
>  
>      var input = newNode.getElementsByTagName(awInputElementName());
> +    if (input && input.length == 1)

Please remove the trailing whitespace around these lines while you're here.

@@ +514,5 @@
>    catch (ex) {
>      return;
>    }
>  
> +  for (var i = 0; i < dragSession.numDropItems; ++i)

let, not var

@@ +521,5 @@
>      var dataObj = new Object();
>      var bestFlavor = new Object();
>      var len = new Object();
> +    trans.getAnyTransferData(bestFlavor, dataObj, len);
> +    if (dataObj)  dataObj = dataObj.value.QueryInterface(Components.interfaces.nsISupportsString);

Let's break these up over two lines, like:

if (dataObj)
  dataObj = dataObj.value.QueryInterface(Components.interfaces.nsISupportsString);
else
  continue;

::: mailnews/addrbook/prefs/content/pref-directory-add.js
@@ +240,5 @@
>  function DisableUriFields(aPrefName)
>  {
>    if (Services.prefs.prefIsLocked(aPrefName)) {
> +    let lockedElements = document.querySelectorAll('[disableiflocked="true"]');
> +    for (let i=0; i<lockedElements.length; i++)

Space on either side of =, <

Comment 99

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: addressbook, v4, r=mconley [checkin: comment 101]

Comment 100

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: message window including content tabs, v4

This patch has all calls to the function adding classes to nodes in the multi-message view converted to use an array as parameter which wasn't completed before.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5c10bc43843d

Comment 102

[Mark Banner (:standard8)]

Comment on attachment 701545 [details] [diff] [review]
internals, v3, patch

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

Looks good, there's a bit of bitrot that you'll need to fix.

Comment 103

[Mark Banner (:standard8)]

Comment on attachment 704576 [details] [diff] [review]
gloda, v2

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

Looks good, please make sure this has been run through try server before landing.

Comment 104

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 714796 [details] [diff] [review]
message window including content tabs, v4

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

Hey Sebastian,

This looks great! r=me with the following fixes. Thanks,

-Mike

::: mail/base/content/mailCore.js
@@ +208,5 @@
>    // make sure our toolbar buttons have the correct enabled state restored to them...
>    if (this.UpdateMailToolbar != undefined)
>      UpdateMailToolbar(focus);
>  
> +  var toolbox = document.querySelector('*[doCustomization="true"]');

let, not var.

@@ +216,5 @@
>      // The GetMail button is stuck in a strange state right now, since the
>      // customization wrapping preserves its children, but not its initialized
>      // state. Fix that here.
>      // Fix Bug 565045: Only treat "Get Message Button" if it is in our toolbox
> +    var popup = toolbox.getElementById("button-getMsgPopup");

let, not var

::: mail/base/content/mailWindowOverlay.js
@@ +935,5 @@
>      var newMenuItem = document.createElement("menuitem");
>      SetMessageTagLabel(newMenuItem, i + 1, taginfo.tag);
>      newMenuItem.setAttribute("value", taginfo.key);
>      newMenuItem.setAttribute("type", "checkbox");
> +    var removeKey = (" " + curKeys + " ").contains(" " + taginfo.key + " ");

let, not var

::: mail/base/content/phishingDetector.js
@@ +94,5 @@
>      for (var index = 0; index < linkNodes.length; index++)
>        this.analyzeUrl(linkNodes[index].href, gatherTextUnder(linkNodes[index]));
>  
>      // extract the action urls associated with any form elements in the message and analyze them.
> +    var formNodes = document.getElementById('messagepane').contentDocument.querySelectorAll("form[action]");

let, not var

::: mail/base/content/selectionsummaries.js
@@ +43,5 @@
>   * the equivalent of jQuery's addClass.  Avoids duplicates, nothing fancy.
>   *
>   * @param node
>   *        any old DOM node
>   * @param classname

This documentation needs to be updated for classArray.

@@ +47,5 @@
>   * @param classname
>   *        a string, which will be added as a CSS class
>   */
> +function _mm_addClass(node, classArray) {
> +  for (let i=0; i<classArray.length; i++)

Spaces on either side of =, <

::: mail/extensions/mailviews/content/msgViewPickerOverlay.js
@@ +166,5 @@
>  {
>    // mark default views if selected
>    let currentViewValue = ViewPickerBinding.currentViewValue;
>  
> +  var viewAll = aViewPopup.querySelector('*[value="' + kViewItemAll + '"]');

let, not var

::: mail/test/mozmill/content-policy/test-dns-prefetch.js
@@ -167,5 @@
>      '</head><body>test dns prefetch</body></html>';
>  
>    let newTab = open_content_tab_with_url(dataurl);
>  
> -  // XXX this should be a check for DNS prefetch being enabled, but bug 545407

I love it when stuff like this gets fixed. :)

::: mail/test/mozmill/folder-display/test-message-commands.js
@@ +82,5 @@
>                "abled when it shouldn't be!");
>  }
>  
>  function enable_archiving(enabled) {
> + Services.prefs.setBoolPref("mail.identity.default.archive_enabled", enabled);

Nit - 2 spaces indent for this line.

::: mail/test/mozmill/folder-display/test-summarization.js
@@ +321,5 @@
>  }
>  
>  function check_address_name(name) {
>    let htmlframe = mc.e('multimessage');
> +  let aMatch = htmlframe.contentDocument.querySelector('.sender');

Why "aMatch"? The "a" prefix is usually reserved for arguments passed to functions. I think this should stay "matches".

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +1322,3 @@
>  
>    for (let key in nsIDOMKeyEvent) {
>      

Trim this trailing whitespace while you're here.

::: mailnews/base/content/folderProps.js
@@ +288,5 @@
>  }
>  
>  function hideShowControls(serverType)
>  {
> +  var controls = document.querySelectorAll("*[hidefor]");

let, not var

::: mailnews/base/content/folderWidgets.xml
@@ +24,5 @@
>          // won't have proper sizing.
>          let inWrapper = false;
>          let node = this;
>          while (node instanceof XULElement) {
> +          if (node.id.startsWith("wrapper-")) {

The old rule is contains, not startsWith. We should probably use contains, unless there's a good reason not to.

Comment 105

[:aceman]

(In reply to Mike Conley (:mconley) from comment #104)
> ::: mailnews/base/content/folderWidgets.xml
> @@ +24,5 @@
> >          // won't have proper sizing.
> >          let inWrapper = false;
> >          let node = this;
> >          while (node instanceof XULElement) {
> > +          if (node.id.startsWith("wrapper-")) {
> 
> The old rule is contains, not startsWith. We should probably use contains,
> unless there's a good reason not to.

I asked him to change it do startsWith as I think that is what the code actually intended, see comment 22. That one should trigger when elements are in the customize palette, where their ids are prefixed with "wrapper-". But we can ask Neil if he knows what is right here.

Comment 106

[[e-mail]]

The id appears to be just a convenience for themes; other code seems to check for a localName of "toolbarpaletteitem". See search.xml's constructor for a related example.

Comment 107

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: gloda, v3, r=Standard8 [checkin: comment 112]

Comment 108

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: imap, v3, r=Standard8 [checkin: comment 112]

Comment 109

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: internals, v4, r=Standard8 [checkin: comment 112]

Comment 110

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: local, v4, r=Standard8 [checkin: comment 112]

Comment 111

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: message window, v5, r=mconley [checkin: comment 112]

Comment 112

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c74ff4fc12de
https://hg.mozilla.org/comm-central/rev/fd40aad70424
https://hg.mozilla.org/comm-central/rev/265cc1d9bce2
https://hg.mozilla.org/comm-central/rev/f60966405026
https://hg.mozilla.org/comm-central/rev/aa6a7917b136

Comment 113

[Ian Neal]

Comment on attachment 714795 [details] [diff] [review]
addressbook, v4, r=mconley [checkin: comment 101]

>+++ b/mailnews/addrbook/content/abMailListDialog.js

>     // pull the URL out of the data object
>-    var address = dataObj.data.substring(0, len.value);
>+    len address = dataObj.data.substring(0, len.value);
This has broken things when trying to create/amend mailing lists in the address book.
Needs to be fix on comm-beta, comm-aurora and comm-central.

Comment 114

[:aceman]

Oh, "len" => "let" :)

Error: SyntaxError: missing ; before statement
Source file: chrome://messenger/content/addressbook/abMailListDialog.js
Line: 531, Column: 8
Source code:
    len address = dataObj.data.substring(0, len.value); 

Error: ReferenceError: OnLoadEditList is not defined
Source file: chrome://messenger/content/addressbook/abEditListDialog.xul
Line: 1

Comment 115

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thank you both for catching this, Ian is fixing this already in bug 859068.

Comment 116

[alta88]

the change in ReplaceWithSelection() may have broken Print Preview with selected text.

Comment 117

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Works for me with Daily 2014-11-15. Did you see bug 1092811? If you can still reproduce, please share the steps.

Comment 118

[alta88]

thanks, this is not reproducible in latest trunk.