Last Comment Bug 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
: Code cleanup in /mail/ and /mailnews/: Use new String methods like startsWith...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 22.0
Assigned To: Sebastian H. [:aryx][:archaeopteryx]
:
Mentors:
Depends on: 846694 849540 859068 860093
Blocks: 1110166
  Show dependency treegraph
 
Reported: 2012-12-21 16:25 PST by Sebastian H. [:aryx][:archaeopteryx]
Modified: 2015-04-01 16:08 PDT (History)
14 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
new string methods patch v1 (435.81 KB, patch)
2012-12-21 16:25 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): about:support, v1 (7.65 KB, patch)
2012-12-22 07:49 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): account creation, v1 (32.43 KB, patch)
2012-12-22 07:50 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): activity manager, v1 (918 bytes, patch)
2012-12-22 07:50 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): add-ons, v1 (6.10 KB, patch)
2012-12-22 07:51 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): addressbook, v1 (21.37 KB, patch)
2012-12-22 07:51 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): chat, v1 (8.85 KB, patch)
2012-12-22 07:52 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): cloudfile, v1 (11.71 KB, patch)
2012-12-22 07:52 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): compose, v1 (38.08 KB, patch)
2012-12-22 07:53 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): fakeserver, v1 (30.04 KB, patch)
2012-12-22 07:53 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): filter, v1 (3.91 KB, patch)
2012-12-22 07:54 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): gloda, v1 (23.03 KB, patch)
2012-12-22 07:55 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): imap, v1 (19.46 KB, patch)
2012-12-22 07:55 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): import, v1 (4.57 KB, patch)
2012-12-22 07:56 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): internals, v1 (53.13 KB, patch)
2012-12-22 07:57 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): local (/mailnews/local/), v1 (16.43 KB, patch)
2012-12-22 07:57 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): message window including content tabs, v1 (104.11 KB, patch)
2012-12-22 07:58 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): newsblog, v1 (6.06 KB, patch)
2012-12-22 07:59 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): news, v1 (7.71 KB, patch)
2012-12-22 07:59 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): preferences, v1 (31.22 KB, patch)
2012-12-22 08:00 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): test: performance and resources, v1 (9.05 KB, patch)
2012-12-22 08:00 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): about:support, v2 (8.06 KB, patch)
2012-12-23 10:56 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): activity manager, v2 (1.12 KB, patch)
2012-12-23 10:57 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): chat, v2 (9.06 KB, patch)
2012-12-23 10:57 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): compose, v2 (38.48 KB, patch)
2012-12-23 10:58 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): internals, v2 (60.58 KB, patch)
2012-12-23 10:59 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
patch (work in progress): message window including content tabs, v2 (104.34 KB, patch)
2012-12-23 11:02 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
about:support, v3, patch (8.24 KB, patch)
2013-01-05 02:11 PST, Sebastian H. [:aryx][:archaeopteryx]
florian: review+
Details | Diff | Splinter Review
account creation, v2, patch (32.83 KB, patch)
2013-01-05 02:14 PST, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
activity manager, v3, patch (1.29 KB, patch)
2013-01-05 02:16 PST, Sebastian H. [:aryx][:archaeopteryx]
bwinton: review+
Details | Diff | Splinter Review
add-ons, v2, patch (6.50 KB, patch)
2013-01-05 02:18 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
addressbook, v2, patch (21.77 KB, patch)
2013-01-05 02:19 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
chat, v3, patch (9.23 KB, patch)
2013-01-05 02:20 PST, Sebastian H. [:aryx][:archaeopteryx]
florian: review+
Details | Diff | Splinter Review
cloudfile, v2, patch (12.11 KB, patch)
2013-01-05 02:22 PST, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
filter, v2, patch (4.30 KB, patch)
2013-01-05 02:24 PST, Sebastian H. [:aryx][:archaeopteryx]
rkent: review+
Details | Diff | Splinter Review
import, v2, patch (4.96 KB, patch)
2013-01-05 02:26 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
chat, v4, r=florian [checkin: comment 64] (9.23 KB, patch)
2013-01-07 10:39 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
internals, v3, patch (59.94 KB, patch)
2013-01-13 00:53 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
message window including content tabs, v3 (104.51 KB, patch)
2013-01-14 00:56 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
local (/mailnews/local/), v2 (16.82 KB, patch)
2013-01-14 01:00 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review-
Details | Diff | Splinter Review
newsblog, v2, patch (6.45 KB, patch)
2013-01-14 01:05 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
news, v2 [ready for check-in] (8.10 KB, patch)
2013-01-14 01:08 PST, Sebastian H. [:aryx][:archaeopteryx]
Pidgeot18: review+
Details | Diff | Splinter Review
preferences, v2 (8.10 KB, patch)
2013-01-14 01:11 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
preferences, v3, patch (31.62 KB, patch)
2013-01-14 01:14 PST, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
compose, v3, patch (38.45 KB, patch)
2013-01-14 11:55 PST, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
addressbook, v3, patch (32.97 KB, patch)
2013-01-15 11:49 PST, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
news, v3. r=jcranmer [checkin: comment 64] (8.11 KB, patch)
2013-01-15 12:29 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
account creation, v3, r=mconley [checkin: comment 64] (75.96 KB, patch)
2013-01-15 12:31 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
filter, v3 [checkin: comment 64] (4.31 KB, patch)
2013-01-16 11:41 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
fakeserver, v2 (27.71 KB, patch)
2013-01-21 05:23 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
gloda, v2 (23.42 KB, patch)
2013-01-21 09:20 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
imap, v2 (19.89 KB, patch)
2013-01-21 14:45 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
test: performance and resources, v2 (9.45 KB, patch)
2013-01-21 14:48 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
cloudfile, v3, r=mconley [ready for checkin, r+ in comment #70] (12.13 KB, patch)
2013-02-01 13:49 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
cloudfile, v4, r=mconley [checkin: comment 74] (12.13 KB, patch)
2013-02-04 10:42 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
preferences, v4, r=mconley [ready for checkin, r+ in comment #72] (34.00 KB, patch)
2013-02-04 10:45 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
preferences, v5, r=mconley [checkin: comment 77] (33.30 KB, patch)
2013-02-04 10:58 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
about:support, v4, r=florian [checkin: comment 85] (8.24 KB, patch)
2013-02-05 15:19 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
activity manager, v4, r=bwinton (ready for check-in) (1.30 KB, patch)
2013-02-05 15:21 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
activity manager, v5, r=bwinton [checkin: comment 85] (1.30 KB, patch)
2013-02-05 16:08 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
add-ons, v3, r=Standard8 [checkin: comment 88] (6.51 KB, patch)
2013-02-07 08:22 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
import, v3, r=Standard8 [checkin: comment 88] (4.97 KB, patch)
2013-02-07 08:23 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
local (/mailnews/local/), v3 (16.82 KB, patch)
2013-02-08 09:04 PST, Sebastian H. [:aryx][:archaeopteryx]
standard8: review+
Details | Diff | Splinter Review
compose, v4, r=mconley [checkin: comment 97] (42.63 KB, patch)
2013-02-09 10:54 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
fakeserver, v3, r=Standard8 [checkin: comment 97] (25.44 KB, patch)
2013-02-09 11:08 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
newsblog, v3, r=Standard8 [checkin: comment 97] (6.46 KB, patch)
2013-02-09 11:09 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
test: performance and resources, v3, r=Standard8 [checkin: comment 97] (9.50 KB, patch)
2013-02-09 11:11 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
addressbook, v4, r=mconley [checkin: comment 101] (33.79 KB, patch)
2013-02-16 08:16 PST, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
message window including content tabs, v4 (107.11 KB, patch)
2013-02-16 08:20 PST, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
gloda, v3, r=Standard8 [checkin: comment 112] (23.44 KB, patch)
2013-03-26 11:59 PDT, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
imap, v3, r=Standard8 [checkin: comment 112] (19.87 KB, patch)
2013-03-26 12:00 PDT, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
internals, v4, r=Standard8 [checkin: comment 112] (58.93 KB, patch)
2013-03-26 12:01 PDT, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
local, v4, r=Standard8 [checkin: comment 112] (16.83 KB, patch)
2013-03-26 12:02 PDT, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
message window, v5, r=mconley [checkin: comment 112] (107.62 KB, patch)
2013-03-26 12:03 PDT, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review

Description Sebastian H. [:aryx][:archaeopteryx] 2012-12-21 16:25:05 PST
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 :aceman 2012-12-22 07:44:35 PST
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 H. [:aryx][:archaeopteryx] 2012-12-22 07:49:12 PST
Created attachment 695203 [details] [diff] [review]
patch (work in progress): about:support, v1
Comment 3 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:50:03 PST
Created attachment 695204 [details] [diff] [review]
patch (work in progress): account creation, v1
Comment 4 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:50:41 PST
Created attachment 695205 [details] [diff] [review]
patch (work in progress): activity manager, v1
Comment 5 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:51:11 PST
Created attachment 695206 [details] [diff] [review]
patch (work in progress): add-ons, v1
Comment 6 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:51:41 PST
Created attachment 695207 [details] [diff] [review]
patch (work in progress): addressbook, v1
Comment 7 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:52:07 PST
Created attachment 695208 [details] [diff] [review]
patch (work in progress): chat, v1
Comment 8 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:52:38 PST
Created attachment 695209 [details] [diff] [review]
patch (work in progress): cloudfile, v1
Comment 9 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:53:21 PST
Created attachment 695210 [details] [diff] [review]
patch (work in progress): compose, v1
Comment 10 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:53:55 PST
Created attachment 695211 [details] [diff] [review]
patch (work in progress): fakeserver, v1
Comment 11 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:54:22 PST
Created attachment 695212 [details] [diff] [review]
patch (work in progress): filter, v1
Comment 12 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:55:23 PST
Created attachment 695213 [details] [diff] [review]
patch (work in progress): gloda, v1
Comment 13 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:55:54 PST
Created attachment 695214 [details] [diff] [review]
patch (work in progress): imap, v1
Comment 14 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:56:28 PST
Created attachment 695215 [details] [diff] [review]
patch (work in progress): import, v1
Comment 15 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:57:01 PST
Created attachment 695216 [details] [diff] [review]
patch (work in progress): internals, v1
Comment 16 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:57:54 PST
Created attachment 695217 [details] [diff] [review]
patch (work in progress): local (/mailnews/local/), v1
Comment 17 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:58:36 PST
Created attachment 695218 [details] [diff] [review]
patch (work in progress): message window including content tabs, v1
Comment 18 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:59:06 PST
Created attachment 695219 [details] [diff] [review]
patch (work in progress): newsblog, v1
Comment 19 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 07:59:35 PST
Created attachment 695220 [details] [diff] [review]
patch (work in progress): news, v1
Comment 20 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 08:00:07 PST
Created attachment 695221 [details] [diff] [review]
patch (work in progress): preferences, v1
Comment 21 Sebastian H. [:aryx][:archaeopteryx] 2012-12-22 08:00:52 PST
Created attachment 695222 [details] [diff] [review]
patch (work in progress): test: performance and resources, v1
Comment 22 :aceman 2012-12-22 08:12:42 PST
-          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 2012-12-22 08:26:39 PST
> +++ 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 H. [:aryx][:archaeopteryx] 2012-12-23 02:44:48 PST
(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 2012-12-23 06:10:20 PST
(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 H. [:aryx][:archaeopteryx] 2012-12-23 10:56:33 PST
Created attachment 695332 [details] [diff] [review]
patch (work in progress): about:support, v2
Comment 27 Sebastian H. [:aryx][:archaeopteryx] 2012-12-23 10:57:13 PST
Created attachment 695333 [details] [diff] [review]
patch (work in progress): activity manager, v2
Comment 28 Sebastian H. [:aryx][:archaeopteryx] 2012-12-23 10:57:58 PST
Created attachment 695334 [details] [diff] [review]
patch (work in progress): chat, v2
Comment 29 Sebastian H. [:aryx][:archaeopteryx] 2012-12-23 10:58:37 PST
Created attachment 695335 [details] [diff] [review]
patch (work in progress): compose, v2
Comment 30 Sebastian H. [:aryx][:archaeopteryx] 2012-12-23 10:59:31 PST
Created attachment 695336 [details] [diff] [review]
patch (work in progress): internals, v2
Comment 31 Sebastian H. [:aryx][:archaeopteryx] 2012-12-23 11:02:48 PST
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
Comment 32 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:11:54 PST
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
Comment 33 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:14:09 PST
Created attachment 698250 [details] [diff] [review]
account creation, v2, patch
Comment 34 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:16:00 PST
Created attachment 698251 [details] [diff] [review]
activity manager, v3, patch
Comment 35 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:18:41 PST
Created attachment 698252 [details] [diff] [review]
add-ons, v2, patch
Comment 36 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:19:34 PST
Created attachment 698254 [details] [diff] [review]
addressbook, v2, patch
Comment 37 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:20:26 PST
Created attachment 698256 [details] [diff] [review]
chat, v3, patch
Comment 38 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:22:28 PST
Created attachment 698257 [details] [diff] [review]
cloudfile, v2, patch
Comment 39 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:24:14 PST
Created attachment 698258 [details] [diff] [review]
filter, v2, patch
Comment 40 Sebastian H. [:aryx][:archaeopteryx] 2013-01-05 02:26:18 PST
Created attachment 698259 [details] [diff] [review]
import, v2, patch
Comment 41 Florian Quèze [:florian] [:flo] 2013-01-07 02:57:30 PST
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 2013-01-07 03:10:01 PST
Could href be null so that href.trim fails?
Comment 43 Florian Quèze [:florian] [:flo] 2013-01-07 03:35:24 PST
(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 H. [:aryx][:archaeopteryx] 2013-01-07 10:39:46 PST
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())
Comment 45 Kent James (:rkent) 2013-01-07 11:08:34 PST
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 H. [:aryx][:archaeopteryx] 2013-01-07 11:43:23 PST
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 2013-01-07 12:09:34 PST
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 H. [:aryx][:archaeopteryx] 2013-01-13 00:53:48 PST
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
Comment 49 Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 00:56:33 PST
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
Comment 50 Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 01:00:23 PST
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
Comment 51 Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 01:05:50 PST
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
Comment 52 Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 01:08:12 PST
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
Comment 53 Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 01:11:17 PST
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
Comment 54 Sebastian H. [:aryx][:archaeopteryx] 2013-01-14 01:14:53 PST
Created attachment 701718 [details] [diff] [review]
preferences, v3, patch

Attached wrong file before.
Comment 55 :aceman 2013-01-14 08:11:07 PST
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 H. [:aryx][:archaeopteryx] 2013-01-14 11:55:38 PST
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).
Comment 57 Mike Conley (:mconley) - (Needinfo me!) 2013-01-14 16:44:34 PST
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) - (Needinfo me!) 2013-01-15 08:10:54 PST
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 H. [:aryx][:archaeopteryx] 2013-01-15 11:49:01 PST
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.
Comment 60 Sebastian H. [:aryx][:archaeopteryx] 2013-01-15 12:29:51 PST
Created attachment 702462 [details] [diff] [review]
news, v3. r=jcranmer [checkin: comment 64]
Comment 61 Sebastian H. [:aryx][:archaeopteryx] 2013-01-15 12:31:51 PST
Created attachment 702464 [details] [diff] [review]
account creation, v3, r=mconley [checkin: comment 64]
Comment 62 Kent James (:rkent) 2013-01-16 09:24:02 PST
Comment on attachment 698258 [details] [diff] [review]
filter, v2, patch

Thanks for the patch!
Comment 63 Sebastian H. [:aryx][:archaeopteryx] 2013-01-16 11:41:06 PST
Created attachment 702949 [details] [diff] [review]
filter, v3 [checkin: comment 64]
Comment 65 Sebastian H. [:aryx][:archaeopteryx] 2013-01-21 05:23:26 PST
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
Comment 66 Sebastian H. [:aryx][:archaeopteryx] 2013-01-21 09:20:50 PST
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
Comment 67 Sebastian H. [:aryx][:archaeopteryx] 2013-01-21 14:45:39 PST
Created attachment 704694 [details] [diff] [review]
imap, v2
Comment 68 Sebastian H. [:aryx][:archaeopteryx] 2013-01-21 14:46:56 PST
Try run with the patch applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=9c5d34b1de14
Comment 69 Sebastian H. [:aryx][:archaeopteryx] 2013-01-21 14:48:51 PST
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
Comment 70 Mike Conley (:mconley) - (Needinfo me!) 2013-02-01 09:56:28 PST
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 H. [:aryx][:archaeopteryx] 2013-02-01 13:49:36 PST
Created attachment 709233 [details] [diff] [review]
cloudfile, v3, r=mconley [ready for checkin, r+ in comment #70]
Comment 72 Mike Conley (:mconley) - (Needinfo me!) 2013-02-03 19:15:47 PST
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 H. [:aryx][:archaeopteryx] 2013-02-04 10:42:45 PST
Created attachment 709788 [details] [diff] [review]
cloudfile, v4, r=mconley [checkin: comment 74]

Old patch had wrong checkin comment.
Comment 74 Sebastian H. [:aryx][:archaeopteryx] 2013-02-04 10:45:02 PST
Created attachment 709790 [details] [diff] [review]
preferences, v4, r=mconley [ready for checkin, r+ in comment #72]
Comment 75 Ryan VanderMeulen [:RyanVM] 2013-02-04 10:45:24 PST
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 H. [:aryx][:archaeopteryx] 2013-02-04 10:58:43 PST
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
Comment 77 Ryan VanderMeulen [:RyanVM] 2013-02-04 11:02:57 PST
Comment on attachment 709795 [details] [diff] [review]
preferences, v5, r=mconley [checkin: comment 77]

https://hg.mozilla.org/comm-central/rev/89fa8b687178
Comment 78 Siddharth Agarwal [:sid0] (inactive) 2013-02-04 20:41:37 PST
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] [:flo] 2013-02-05 12:27:50 PST
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) (:☕️) 2013-02-05 12:33:57 PST
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 H. [:aryx][:archaeopteryx] 2013-02-05 15:19:48 PST
Created attachment 710404 [details] [diff] [review]
about:support, v4, r=florian [checkin: comment 85]
Comment 82 Sebastian H. [:aryx][:archaeopteryx] 2013-02-05 15:21:49 PST
Created attachment 710406 [details] [diff] [review]
activity manager, v4, r=bwinton (ready for check-in)
Comment 83 Florian Quèze [:florian] [:flo] 2013-02-05 15:46:06 PST
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 ;).
Comment 84 Sebastian H. [:aryx][:archaeopteryx] 2013-02-05 16:08:14 PST
Created attachment 710426 [details] [diff] [review]
activity manager, v5, r=bwinton [checkin: comment 85]

Thank you for the catch
Comment 86 Sebastian H. [:aryx][:archaeopteryx] 2013-02-07 08:22:15 PST
Created attachment 711359 [details] [diff] [review]
add-ons, v3, r=Standard8 [checkin: comment 88]
Comment 87 Sebastian H. [:aryx][:archaeopteryx] 2013-02-07 08:23:57 PST
Created attachment 711360 [details] [diff] [review]
import, v3, r=Standard8 [checkin: comment 88]
Comment 89 Mark Banner (:standard8) 2013-02-07 15:07:33 PST
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) 2013-02-07 15:13:32 PST
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 H. [:aryx][:archaeopteryx] 2013-02-08 09:04:29 PST
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.
Comment 92 Mike Conley (:mconley) - (Needinfo me!) 2013-02-08 12:54:11 PST
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 H. [:aryx][:archaeopteryx] 2013-02-09 10:54:37 PST
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.
Comment 94 Sebastian H. [:aryx][:archaeopteryx] 2013-02-09 11:08:12 PST
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
Comment 95 Sebastian H. [:aryx][:archaeopteryx] 2013-02-09 11:09:30 PST
Created attachment 712171 [details] [diff] [review]
newsblog, v3, r=Standard8 [checkin: comment 97]
Comment 96 Sebastian H. [:aryx][:archaeopteryx] 2013-02-09 11:11:26 PST
Created attachment 712172 [details] [diff] [review]
test: performance and resources, v3, r=Standard8 [checkin: comment 97]
Comment 98 Mike Conley (:mconley) - (Needinfo me!) 2013-02-10 15:33:00 PST
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 H. [:aryx][:archaeopteryx] 2013-02-16 08:16:11 PST
Created attachment 714795 [details] [diff] [review]
addressbook, v4, r=mconley [checkin: comment 101]
Comment 100 Sebastian H. [:aryx][:archaeopteryx] 2013-02-16 08:20:17 PST
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.
Comment 101 Ryan VanderMeulen [:RyanVM] 2013-02-16 16:22:20 PST
https://hg.mozilla.org/comm-central/rev/5c10bc43843d
Comment 102 Mark Banner (:standard8) 2013-03-22 04:35:57 PDT
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) 2013-03-22 04:58:15 PDT
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) - (Needinfo me!) 2013-03-23 10:55:39 PDT
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 2013-03-23 11:02:49 PDT
(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 neil@parkwaycc.co.uk 2013-03-25 17:24:48 PDT
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 H. [:aryx][:archaeopteryx] 2013-03-26 11:59:29 PDT
Created attachment 729684 [details] [diff] [review]
gloda, v3, r=Standard8 [checkin: comment 112]
Comment 108 Sebastian H. [:aryx][:archaeopteryx] 2013-03-26 12:00:20 PDT
Created attachment 729688 [details] [diff] [review]
imap, v3, r=Standard8 [checkin: comment 112]
Comment 109 Sebastian H. [:aryx][:archaeopteryx] 2013-03-26 12:01:12 PDT
Created attachment 729689 [details] [diff] [review]
internals, v4, r=Standard8 [checkin: comment 112]
Comment 110 Sebastian H. [:aryx][:archaeopteryx] 2013-03-26 12:02:24 PDT
Created attachment 729691 [details] [diff] [review]
local, v4, r=Standard8 [checkin: comment 112]
Comment 111 Sebastian H. [:aryx][:archaeopteryx] 2013-03-26 12:03:21 PDT
Created attachment 729693 [details] [diff] [review]
message window, v5, r=mconley [checkin: comment 112]
Comment 113 Ian Neal (Away until 7th Aug) 2013-04-07 03:25:31 PDT
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 2013-04-07 04:30:22 PDT
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 H. [:aryx][:archaeopteryx] 2013-04-07 07:40:28 PDT
Thank you both for catching this, Ian is fixing this already in bug 859068.
Comment 116 alta88 2014-11-14 09:06:13 PST
the change in ReplaceWithSelection() may have broken Print Preview with selected text.
Comment 117 Sebastian H. [:aryx][:archaeopteryx] 2014-11-15 13:29:24 PST
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 2014-11-20 07:18:46 PST
thanks, this is not reproducible in latest trunk.

Note You need to log in before you can comment on or make changes to this bug.