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

RESOLVED FIXED in Thunderbird 22.0

Status

Thunderbird
General
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: aryx, Assigned: aryx)

Tracking

Trunk
Thunderbird 22.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(19 attachments, 55 obsolete attachments)

9.23 KB, patch
Details | Diff | Splinter Review
8.11 KB, patch
Details | Diff | Splinter Review
75.96 KB, patch
Details | Diff | Splinter Review
4.31 KB, patch
Details | Diff | Splinter Review
33.30 KB, patch
Details | Diff | Splinter Review
8.24 KB, patch
Details | Diff | Splinter Review
1.30 KB, patch
Details | Diff | Splinter Review
6.51 KB, patch
Details | Diff | Splinter Review
4.97 KB, patch
Details | Diff | Splinter Review
42.63 KB, patch
Details | Diff | Splinter Review
25.44 KB, patch
Details | Diff | Splinter Review
6.46 KB, patch
Details | Diff | Splinter Review
9.50 KB, patch
Details | Diff | Splinter Review
33.79 KB, patch
Details | Diff | Splinter Review
23.44 KB, patch
Details | Diff | Splinter Review
19.87 KB, patch
Details | Diff | Splinter Review
58.93 KB, patch
Details | Diff | Splinter Review
16.83 KB, patch
Details | Diff | Splinter Review
107.62 KB, patch
Details | Diff | Splinter Review
Created attachment 695049 [details] [diff] [review]
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

5 years ago
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.
Created attachment 695203 [details] [diff] [review]
patch (work in progress): about:support, v1
Attachment #695049 - Attachment is obsolete: true
Created attachment 695204 [details] [diff] [review]
patch (work in progress): account creation, v1
Created attachment 695205 [details] [diff] [review]
patch (work in progress): activity manager, v1
Created attachment 695206 [details] [diff] [review]
patch (work in progress): add-ons, v1
Created attachment 695207 [details] [diff] [review]
patch (work in progress): addressbook, v1
Created attachment 695208 [details] [diff] [review]
patch (work in progress): chat, v1
Created attachment 695209 [details] [diff] [review]
patch (work in progress): cloudfile, v1
Created attachment 695210 [details] [diff] [review]
patch (work in progress): compose, v1
Created attachment 695211 [details] [diff] [review]
patch (work in progress): fakeserver, v1
Created attachment 695212 [details] [diff] [review]
patch (work in progress): filter, v1
Created attachment 695213 [details] [diff] [review]
patch (work in progress): gloda, v1
Created attachment 695214 [details] [diff] [review]
patch (work in progress): imap, v1
Created attachment 695215 [details] [diff] [review]
patch (work in progress): import, v1
Created attachment 695216 [details] [diff] [review]
patch (work in progress): internals, v1
Created attachment 695217 [details] [diff] [review]
patch (work in progress): local (/mailnews/local/), v1
Created attachment 695218 [details] [diff] [review]
patch (work in progress): message window including content tabs, v1
Created attachment 695219 [details] [diff] [review]
patch (work in progress): newsblog, v1
Created attachment 695220 [details] [diff] [review]
patch (work in progress): news, v1
Created attachment 695221 [details] [diff] [review]
patch (work in progress): preferences, v1
Created attachment 695222 [details] [diff] [review]
patch (work in progress): test: performance and resources, v1

Comment 22

5 years ago
-          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

5 years ago
> +++ 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. ?
(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

5 years ago
(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?
Created attachment 695332 [details] [diff] [review]
patch (work in progress): about:support, v2
Attachment #695203 - Attachment is obsolete: true
Created attachment 695333 [details] [diff] [review]
patch (work in progress): activity manager, v2
Attachment #695205 - Attachment is obsolete: true
Created attachment 695334 [details] [diff] [review]
patch (work in progress): chat, v2
Attachment #695208 - Attachment is obsolete: true
Created attachment 695335 [details] [diff] [review]
patch (work in progress): compose, v2
Attachment #695210 - Attachment is obsolete: true
Created attachment 695336 [details] [diff] [review]
patch (work in progress): internals, v2
Attachment #695216 - Attachment is obsolete: true
Created attachment 695337 [details] [diff] [review]
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
Attachment #695218 - Attachment is obsolete: true
Created attachment 698249 [details] [diff] [review]
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
Attachment #695332 - Attachment is obsolete: true
Attachment #698249 - Flags: review?(sid.bugzilla)
Created attachment 698250 [details] [diff] [review]
account creation, v2, patch
Attachment #695204 - Attachment is obsolete: true
Attachment #698250 - Flags: review?(mconley)
Created attachment 698251 [details] [diff] [review]
activity manager, v3, patch
Attachment #695333 - Attachment is obsolete: true
Attachment #698251 - Flags: review?(bwinton)
Created attachment 698252 [details] [diff] [review]
add-ons, v2, patch
Attachment #695206 - Attachment is obsolete: true
Attachment #698252 - Flags: review?(mbanner)
Created attachment 698254 [details] [diff] [review]
addressbook, v2, patch
Attachment #695207 - Attachment is obsolete: true
Attachment #698254 - Flags: review?(mconley)
Created attachment 698256 [details] [diff] [review]
chat, v3, patch
Attachment #695334 - Attachment is obsolete: true
Attachment #698256 - Flags: review?(florian)
Created attachment 698257 [details] [diff] [review]
cloudfile, v2, patch
Attachment #695209 - Attachment is obsolete: true
Attachment #698257 - Flags: review?(mconley)
Created attachment 698258 [details] [diff] [review]
filter, v2, patch
Attachment #695212 - Attachment is obsolete: true
Attachment #698258 - Flags: review?(kent)
Created attachment 698259 [details] [diff] [review]
import, v2, patch
Attachment #695215 - Attachment is obsolete: true
Attachment #698259 - Flags: review?(mbanner)
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.)
Attachment #698256 - Flags: review?(florian) → review+

Comment 42

5 years ago
Could href be null so that href.trim fails?
(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.
Created attachment 698778 [details] [diff] [review]
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())
Attachment #698256 - Attachment is obsolete: true

Comment 45

5 years ago
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.
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

5 years ago
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.
Created attachment 701545 [details] [diff] [review]
internals, v3, patch

Try run with the same oranges like usual: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=2bc26793024c&noignore=1
Attachment #695336 - Attachment is obsolete: true
Attachment #701545 - Flags: review?(mbanner)
Created attachment 701706 [details] [diff] [review]
message window including content tabs, v3

Successful Try run with the common oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4bd3ef3be8dd&noignore=1
Attachment #695337 - Attachment is obsolete: true
Attachment #701706 - Flags: review?(squibblyflabbetydoo)
Created attachment 701707 [details] [diff] [review]
local (/mailnews/local/), v2

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=add525660cd3
Attachment #695217 - Attachment is obsolete: true
Attachment #701707 - Flags: review?(mbanner)
Created attachment 701709 [details] [diff] [review]
newsblog, v2, patch

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=8e2caf9715a1
Attachment #695219 - Attachment is obsolete: true
Attachment #701709 - Flags: review?(mbanner)
Created attachment 701711 [details] [diff] [review]
news, v2 [ready for check-in]

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=b16ec7e3ba38
Attachment #695220 - Attachment is obsolete: true
Attachment #701711 - Flags: review?(Pidgeot18)
Created attachment 701716 [details] [diff] [review]
preferences, v2

Successful Try run with the known oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=85b44393f82f
Attachment #695221 - Attachment is obsolete: true
Attachment #701716 - Flags: review?(mconley)
Created attachment 701718 [details] [diff] [review]
preferences, v3, patch

Attached wrong file before.
Attachment #701716 - Attachment is obsolete: true
Attachment #701716 - Flags: review?(mconley)
Attachment #701718 - Flags: review?(mconley)
Attachment #701711 - Flags: review?(Pidgeot18) → review+

Comment 55

5 years ago
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)?
Created attachment 701908 [details] [diff] [review]
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).
Attachment #695335 - Attachment is obsolete: true
Attachment #701908 - Flags: review?(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
Attachment #698250 - Flags: review?(mconley) → review+
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.
Attachment #698254 - Flags: review?(mconley)
Created attachment 702444 [details] [diff] [review]
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.
Attachment #698254 - Attachment is obsolete: true
Attachment #702444 - Flags: review?(mconley)

Updated

5 years ago
Attachment #701711 - Attachment description: news, v2 → news, v2 [ready for check-in]

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [please leave open after check-in of marked patches]
Keywords: checkin-needed
Created attachment 702462 [details] [diff] [review]
news, v3. r=jcranmer [checkin: comment 64]
Attachment #701711 - Attachment is obsolete: true
Created attachment 702464 [details] [diff] [review]
account creation, v3, r=mconley [checkin: comment 64]
Attachment #698250 - Attachment is obsolete: true
Attachment #698778 - Attachment description: chat, v4, r=florian → chat, v4, r=florian, [ready for check-in]

Updated

5 years ago
Keywords: checkin-needed

Comment 62

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

Thanks for the patch!
Attachment #698258 - Flags: review?(kent) → review+
Created attachment 702949 [details] [diff] [review]
filter, v3 [checkin: comment 64]
Attachment #698258 - Attachment is obsolete: true
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
Keywords: checkin-needed
Whiteboard: [please leave open after check-in of marked patches] → [leave open]
Attachment #698778 - Attachment description: chat, v4, r=florian, [ready for check-in] → chat, v4, r=florian [checkin: comment 64]
Attachment #702462 - Attachment description: news, v3. r=jcranmer [ready for check-in] → news, v3. r=jcranmer [checkin: comment 64]
Attachment #702464 - Attachment description: account creation, v3, patch, r=mconley [ready for check-in] → account creation, v3, patch, r=mconley [checkin: comment 64]
Attachment #702949 - Attachment description: filter, v3, patch, r=rkent [ready for check-in] → filter, v3 [checkin: comment 64]
Attachment #702464 - Attachment description: account creation, v3, patch, r=mconley [checkin: comment 64] → account creation, v3, r=mconley [checkin: comment 64]
Created attachment 704523 [details] [diff] [review]
fakeserver, v2

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=e5d7f51c80e7
Attachment #695211 - Attachment is obsolete: true
Attachment #704523 - Flags: review?(mbanner)
Created attachment 704576 [details] [diff] [review]
gloda, v2

Successful Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=ab5f667af62f
Attachment #695213 - Attachment is obsolete: true
Attachment #704576 - Flags: review?(bugmail)
Created attachment 704694 [details] [diff] [review]
imap, v2
Attachment #695214 - Attachment is obsolete: true
Attachment #704694 - Flags: review?(mozilla)
Try run with the patch applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=9c5d34b1de14
Created attachment 704695 [details] [diff] [review]
test: performance and resources, v2

Try run with the usual oranges: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=9c5d34b1de14
Attachment #695222 - Attachment is obsolete: true
Attachment #704695 - Flags: review?(mbanner)
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!
Attachment #698257 - Flags: review?(mconley) → review+
Created attachment 709233 [details] [diff] [review]
cloudfile, v3, r=mconley [ready for checkin, r+ in comment #70]
Attachment #698257 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][checkin to 'cloudfile' patch to comm-central]
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
Attachment #701718 - Flags: review?(mconley) → review+
Attachment #709233 - Attachment is patch: true

Updated

4 years ago
Whiteboard: [leave open][checkin to 'cloudfile' patch to comm-central] → [leave open][checkin the 'cloudfile' patch to comm-central]
Created attachment 709788 [details] [diff] [review]
cloudfile, v4, r=mconley [checkin: comment 74]

Old patch had wrong checkin comment.
Attachment #709233 - Attachment is obsolete: true
Created attachment 709790 [details] [diff] [review]
preferences, v4, r=mconley [ready for checkin, r+ in comment #72]
Attachment #701718 - Attachment is obsolete: true
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
Attachment #709788 - Attachment description: cloudfile, v4, r=mconley [ready for checkin, r+ in comment #70] → cloudfile, v4, r=mconley [checkin: comment 74]
Keywords: checkin-needed
Whiteboard: [leave open][checkin the 'cloudfile' patch to comm-central] → [leave open]
Created attachment 709795 [details] [diff] [review]
preferences, v5, r=mconley [checkin: comment 77]

Updated patch for preferences, got bitrotted by aceman's http://hg.mozilla.org/comm-central/rev/35d92e1faf46
Attachment #709788 - Attachment is obsolete: true
Attachment #709790 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][checkin 'preferences' patch to comm-central]
Keywords: checkin-needed
Whiteboard: [leave open][checkin 'preferences' patch to comm-central] → [leave open]
Comment on attachment 709795 [details] [diff] [review]
preferences, v5, r=mconley [checkin: comment 77]

https://hg.mozilla.org/comm-central/rev/89fa8b687178
Attachment #709795 - Attachment description: preferences, v5, r=mconley [ready for checkin, r+ in comment #72] → preferences, v5, r=mconley [checkin: comment 77]
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.
Attachment #698249 - Flags: review?(sid.bugzilla)
Attachment #698249 - Flags: review?(mconley)
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?
Attachment #698249 - Flags: review?(mconley) → review+
Comment on attachment 698251 [details] [diff] [review]
activity manager, v3, patch

rs=me.  Thanks, and thanks to Florian for mentioning the bug.  ;)
Attachment #698251 - Flags: review?(bwinton) → review+
Created attachment 710404 [details] [diff] [review]
about:support, v4, r=florian [checkin: comment 85]
Attachment #698249 - Attachment is obsolete: true
Created attachment 710406 [details] [diff] [review]
activity manager, v4, r=bwinton (ready for check-in)
Attachment #698251 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][check in patches 'about:support' and 'activity manager' to comm-central]
Comment on attachment 710406 [details] [diff] [review]
activity manager, v4, r=bwinton (ready for check-in)

># HG changeset patch
># User Sebastian Hengst <archaeopteryx@coole-files.de>
># 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 ;).
Created attachment 710426 [details] [diff] [review]
activity manager, v5, r=bwinton [checkin: comment 85]

Thank you for the catch
Attachment #710406 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/bca970870c77
https://hg.mozilla.org/comm-central/rev/cca94582cef4
Keywords: checkin-needed
Whiteboard: [leave open][check in patches 'about:support' and 'activity manager' to comm-central] → [leave open]
Attachment #710404 - Attachment description: about:support, v4, r=florian (ready for check-in) → about:support, v4, r=florian [checkin: comment 85]
Attachment #710426 - Attachment description: activity manager, v5, r=bwinton (ready for check-in) → activity manager, v5, r=bwinton [checkin: comment 85]
Attachment #698259 - Flags: review?(mbanner) → review+
Attachment #698252 - Flags: review?(mbanner) → review+
Created attachment 711359 [details] [diff] [review]
add-ons, v3, r=Standard8 [checkin: comment 88]
Attachment #698252 - Attachment is obsolete: true
Created attachment 711360 [details] [diff] [review]
import, v3, r=Standard8 [checkin: comment 88]
Attachment #698259 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][check in to comm-central]
Whiteboard: [leave open][check in to comm-central] → [leave open][check in patches 'add-ons' and 'import' to comm-central]
https://hg.mozilla.org/comm-central/rev/cd2d38507626
https://hg.mozilla.org/comm-central/rev/c39b215c927a
Keywords: checkin-needed
Whiteboard: [leave open][check in patches 'add-ons' and 'import' to comm-central] → [leave open]
Attachment #711359 - Attachment description: add-ons, v3, r=Standard8 [ready for check-in] → add-ons, v3, r=Standard8 [checkin: comment 88]
Attachment #711360 - Attachment description: import, v3, r=bwinton [ready for check-in] → import, v3, r=Standard8 [checkin: comment 88]
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...
Attachment #701707 - Flags: review?(mbanner) → review-
Attachment #701709 - Flags: review?(mbanner) → review+
Comment on attachment 704523 [details] [diff] [review]
fakeserver, v2

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

r=me assuming this passes try server.
Attachment #704523 - Flags: review?(mbanner) → review+
Attachment #704695 - Flags: review?(mbanner) → review+
Created attachment 711865 [details] [diff] [review]
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.
Attachment #701707 - Attachment is obsolete: true
Attachment #711865 - Flags: review?(mbanner)
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.
Attachment #701908 - Flags: review?(mconley) → review+
Created attachment 712169 [details] [diff] [review]
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.
Attachment #701908 - Attachment is obsolete: true
Created attachment 712170 [details] [diff] [review]
fakeserver, v3, r=Standard8 [checkin: comment 97]

Patch passes Try server: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=93afab1873d8
Attachment #704523 - Attachment is obsolete: true
Created attachment 712171 [details] [diff] [review]
newsblog, v3, r=Standard8 [checkin: comment 97]
Attachment #701709 - Attachment is obsolete: true
Created attachment 712172 [details] [diff] [review]
test: performance and resources, v3, r=Standard8 [checkin: comment 97]
Attachment #704695 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][check into comm-central the patches 'compose', 'fakeserver', 'newsblog' and 'test: performance and resources']
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
Keywords: checkin-needed
Whiteboard: [leave open][check into comm-central the patches 'compose', 'fakeserver', 'newsblog' and 'test: performance and resources'] → [leave open]
Attachment #712169 - Attachment description: compose, v4, r=mconley [ready for check-in] → compose, v4, r=mconley [checkin: comment 97]
Attachment #712170 - Attachment description: fakeserver, v3, r=Standard8 [ready for check-in] → fakeserver, v3, r=Standard8 [checkin: comment 97]
Attachment #712171 - Attachment description: newsblog, v3, r=Standard8 [ready for check-in] → newsblog, v3, r=Standard8 [checkin: comment 97]
Attachment #712172 - Attachment description: test: performance and resources, v3, r=Standard8 [ready for check-in] → test: performance and resources, v3, r=Standard8 [checkin: comment 97]
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 =, <
Attachment #702444 - Flags: review?(mconley) → review+
Created attachment 714795 [details] [diff] [review]
addressbook, v4, r=mconley [checkin: comment 101]
Attachment #702444 - Attachment is obsolete: true
Created attachment 714796 [details] [diff] [review]
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.
Attachment #701706 - Attachment is obsolete: true
Attachment #701706 - Flags: review?(squibblyflabbetydoo)
Attachment #714796 - Flags: review?(mconley)
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][check in patch 'addressbook' into comm-central]
https://hg.mozilla.org/comm-central/rev/5c10bc43843d
Keywords: checkin-needed
Whiteboard: [leave open][check in patch 'addressbook' into comm-central] → [leave open]
Attachment #714795 - Attachment description: addressbook, v4, r=mconley [ready for check-in] → addressbook, v4, r=mconley [checkin: comment 101]

Updated

4 years ago
Depends on: 846694
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.
Attachment #701545 - Flags: review?(mbanner) → review+
Attachment #711865 - Flags: review?(mbanner) → review+
Attachment #704694 - Flags: review?(mozilla) → review+
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.
Attachment #704576 - Flags: review?(bugmail) → review+
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.
Attachment #714796 - Flags: review?(mconley) → review+

Comment 105

4 years ago
(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.
Flags: needinfo?(neil)
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.
Flags: needinfo?(neil)
Created attachment 729684 [details] [diff] [review]
gloda, v3, r=Standard8 [checkin: comment 112]
Attachment #704576 - Attachment is obsolete: true
Created attachment 729688 [details] [diff] [review]
imap, v3, r=Standard8 [checkin: comment 112]
Attachment #704694 - Attachment is obsolete: true
Created attachment 729689 [details] [diff] [review]
internals, v4, r=Standard8 [checkin: comment 112]
Attachment #701545 - Attachment is obsolete: true
Created attachment 729691 [details] [diff] [review]
local, v4, r=Standard8 [checkin: comment 112]
Attachment #711865 - Attachment is obsolete: true
Created attachment 729693 [details] [diff] [review]
message window, v5, r=mconley [checkin: comment 112]
Attachment #714796 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [leave open] → [push to comm-central]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [push to comm-central]
Target Milestone: --- → Thunderbird 22.0
Attachment #729684 - Attachment description: gloda, v3, r=Standard8 [ready for check-in] → gloda, v3, r=Standard8 [checkin: comment 112]
Attachment #729688 - Attachment description: imap, v3, r=Standard8 [ready for check-in] → imap, v3, r=Standard8 [checkin: comment 112]
Attachment #729689 - Attachment description: internals, v4, r=Standard8 [ready for check-in] → internals, v4, r=Standard8 [checkin: comment 112]
Attachment #729691 - Attachment description: local, v4, r=Standard8 [ready for check-in] → local, v4, r=Standard8 [checkin: comment 112]
Attachment #729693 - Attachment description: message window, v5, r=mconley [ready for check-in] → message window, v5, r=mconley [checkin: comment 112]

Comment 113

4 years ago
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.

Updated

4 years ago
Blocks: 859068

Comment 114

4 years ago
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
Flags: needinfo?(archaeopteryx)
Thank you both for catching this, Ian is fixing this already in bug 859068.
No longer blocks: 859068
Depends on: 859068
Flags: needinfo?(archaeopteryx)

Updated

4 years ago
Depends on: 860093

Comment 116

3 years ago
the change in ReplaceWithSelection() may have broken Print Preview with selected text.
Works for me with Daily 2014-11-15. Did you see bug 1092811? If you can still reproduce, please share the steps.

Comment 118

3 years ago
thanks, this is not reproducible in latest trunk.
Blocks: 1110166

Updated

2 years ago
Depends on: 849540
You need to log in before you can comment on or make changes to this bug.