Code cleanup in /editor/: Use new String methods like startsWith, endsWith, contains and use querySelector instead of NodeList calls

RESOLVED FIXED in Thunderbird 24.0

Status

MailNews Core
Composition
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aryx, Assigned: aryx)

Tracking

Trunk
Thunderbird 24.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

56.28 KB, patch
Details | Diff | Splinter Review
1.17 KB, patch
Details | Diff | Splinter Review
This bug is for /editor/ what bug 824150 is for /mail/ and /mailnews/: Where possible, <string>.startsWith(aStringStart) will be used instead of string.substr(0, X) == aStringStart etc., calling node lists and using only the first one also gets replaced by <element>.querySelector

Furthermore, the patch is mconley-ized, meaning that in the modified methods/functions var has been replaced by let.
Created attachment 711563 [details] [diff] [review]
Use new string methods and querySelector in /editor/, v1

Successful Thunderbird-Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=96837c5b8ab0
Attachment #711563 - Flags: review?(mconley)
Comment on attachment 711563 [details] [diff] [review]
Use new string methods and querySelector in /editor/, v1

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

Sorry for the long overdue review. Most of this looks great, just a few notes below. Please ensure that the values I've suggested that you use .trim() on will always be strings. If not, well, do a check before calling trim, I guess. :)

r=me with the following changes.

::: editor/ui/composer/content/editor.js
@@ +236,4 @@
>          if (!params)
>            return;
>  
> +        let editorStatus;

Yes! Thank you for removing var abuse. :)

@@ +346,5 @@
>              HideItem("structSpacer");
>  
>              // Hide everything in "Insert" except for "Symbols"
> +            let menuPopupChildren = document.querySelectorAll('[id="insertMenuPopup"] > :not(#insertChars)');
> +            if (menuPopupChildren)

if [id="insertMenuPopup"] > :not(#insertChars) doesn't match anything, this will return a nodelist of length 0, so I don't think you need the conditional on line 350.

@@ +351,2 @@
>              {
> +              for (let i = 0; i < menuPopupChildren.length; i++) 

Please strip the trailing whitespace while you're here.

@@ +1629,5 @@
>          }
>  
>          // Get the text for the <title> from the newly-parsed document
>          // (must do this for proper conversion of "escaped" characters)
> +        let titleNode = editor.document.querySelector("title");

Great cleanup here.

@@ +2166,2 @@
>      // let's start by assuming we have an author in case we don't have the pref
> +    let authorFound = domdoc.querySelector('meta[name="author"]') ? true : false;

if the querySelector doesn't find anything, this should return null, so we can just use:

let authorFound = domdoc.querySelector('meta[name="author"]');

and then use

if (prefAuthorString && prefAuthorString != 0 && !author && headelement) {
 // ...
}

@@ +3070,5 @@
>      elt.parentNode.removeChild(elt);
>    }
>  
> +  let anchorNode = theDocument.querySelector('a[name^="mozTocId"]');
> +  while (anchorNode) {

Even better:

let anchorNodes = theDocument.querySelectorAll('a[name^="mozTocId"]');
for (let node of anchorNodes) {
  node.parentNode.removeChild(node);
}

::: editor/ui/composer/content/editorUtilities.js
@@ +517,5 @@
>  }
>  
>  function MakeRelativeUrl(url)
>  {
> +  let inputUrl = TrimString(url);

Do we really need TrimString? What about just .trim()?

So:

if (!inputUrl)
  return inputUrl;

inputUrl = inputUrl.trim();

::: editor/ui/composer/content/publishprefs.js
@@ +694,1 @@
>    baseUrl = StripUsernamePassword(baseUrl, username); 

Trim the trailing whitespace while you're here.

@@ +694,5 @@
>    baseUrl = StripUsernamePassword(baseUrl, username); 
>    username = username.value;
>  
> +  let matchedLength = 0;
> +  let pubUrlFound = publishData.publishUrl ?

Instead of the ternary's, how about:

let pubUrlFound = publishData.publishUrl && baseUrl.startsWith(publishData.publishUrl);
let browseUrlFound = publishData.browseUrl && baseUrl.startsWith(publishData.browseUrl);

::: editor/ui/dialogs/content/EdAEHTMLAttributes.js
@@ +94,1 @@
>    if (nodeMap.length > 0) 

Please strip the trailing whitespace while you're here.

@@ +98,4 @@
>      {
>        if ( CheckAttributeNameSimilarity( nodeMap[i].nodeName, HTMLAttrs ) ||
> +           nodeMap[i].nodeName.toLowerCase().startsWith("on") ||
> +           TrimString( nodeMap[i].nodeName.toLowerCase() ) == "style" ) {

Just .trim()

@@ +150,5 @@
>  }
>  
>  function onInputHTMLAttributeName()
>  {
> +  let attName = TrimString(gDialog.AddHTMLAttributeNameInput.value).toLowerCase();

Just .trim() - assuming gDialog.AddHtmlAttributeNameInput always returns a string.

::: editor/ui/dialogs/content/EdConvertToTable.js
@@ +139,3 @@
>        if (end > start)
>        {
> +        let tagContent = TrimString(str.slice(start + 1, end));

Just use .trim().

@@ +142,4 @@
>  
>          if (/^ol|^ul|^dl/.test(tagContent))
>          {
>            //  Replace list tag with <BR> to start new row 

Please remove the trailing whitespace while you're here.

::: editor/ui/dialogs/content/EdImageOverlay.js
@@ +461,5 @@
>      return false;
>    }
>  
>    // We must convert to "file:///" or "http://" format else image doesn't load!
> +  let src = TrimString(gDialog.srcInput.value);

Yet another place where would probably just use .trim()

@@ +473,5 @@
>    } catch (e) { }
>  
>    globalElement.setAttribute("src", src);
>  
> +  let title = TrimString(gDialog.titleInput.value);

Just use .trim().

::: editor/ui/dialogs/content/EdInsertTOC.js
@@ +74,3 @@
>      // let's look at the children of the TOC ; if we find a comment beginning
>      // with "mozToc", it contains the TOC definition
> +    for (let i = 0; i< nodeList.length; ++i) {

i <

::: editor/ui/dialogs/content/EditorPublishProgress.js
@@ +140,5 @@
>  
>    if (!status)
>      status = "busy";
>  
>    // Just set attribute for status icon 

Strip the trailing whitespace while you're here.
Attachment #711563 - Flags: review?(mconley) → review+
Created attachment 720030 [details]
Use new string methods and querySelector in /editor/, v3

(In reply to Mike Conley (:mconley) from comment #2)
> ::: editor/ui/composer/content/editor.js
> @@ +3070,5 @@
> >      elt.parentNode.removeChild(elt);
> >    }
> >  
> > +  let anchorNode = theDocument.querySelector('a[name^="mozTocId"]');
> > +  while (anchorNode) {
> 
> Even better:
> 
> let anchorNodes = theDocument.querySelectorAll('a[name^="mozTocId"]');
> for (let node of anchorNodes) {
>   node.parentNode.removeChild(node);
> }
querySelectorAll if a non-live node list. If there would be a <a name="mozTocId"><a name="mozTocId">Index</a></a>, we would be in trouble, wouldn't we?

> 
> ::: editor/ui/composer/content/editorUtilities.js
> @@ +517,5 @@
> >  }
> >  
> >  function MakeRelativeUrl(url)
> >  {
> > +  let inputUrl = TrimString(url);
> 
> Do we really need TrimString? What about just .trim()?
> 
> So:
> 
> if (!inputUrl)
>   return inputUrl;
> 
> inputUrl = inputUrl.trim();
> 
> ::: editor/ui/composer/content/publishprefs.js
> @@ +694,1 @@
> >    baseUrl = StripUsernamePassword(baseUrl, username); 
> 
> Trim the trailing whitespace while you're here.
> 
1. You mean:
if (!url)
  return url;

inputUrl = url.trim();

2. But this behaves different if url is " ". So I just converted TrimString to trim() (checked all calls that they already check for it being not null

Needsinfo for you to let me know if you live with this.
Attachment #711563 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Created attachment 720107 [details] [diff] [review]
Use new string methods and querySelector in /editor/, v4

Had to replace some RegExp.leftContext which broke due to some RegExp removals.
Attachment #720030 - Attachment is obsolete: true
Created attachment 723185 [details] [diff] [review]
Use new string methods and querySelector in /editor/, v5, r=mconley
Attachment #720107 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Keywords: checkin-needed

Comment 6

4 years ago
As this is shared code it will need review from the SM side too.
Keywords: checkin-needed
Attachment #723185 - Flags: review?(iann_bugzilla)

Comment 7

4 years ago
Comment on attachment 723185 [details] [diff] [review]
Use new string methods and querySelector in /editor/, v5, r=mconley

I'm not a fan of wholesale changing from var to let unless there is another reason to change the line of code, so I will r- for that reason alone. I will do a partial review of the non var to let changes below, that I can spot, but will wait for a new patch to a full review.

>+++ b/editor/ui/composer/content/ComposerCommands.js

> // nsIPrompt
>   alert : function(dlgTitle, text)
>   {
>-    AlertWithTitle(dlgTitle, text, gProgressDialog ? gProgressDialog : window);
>+    Services.prompt.alert(gProgressDialog ? gProgressDialog : window, dlgTitle, text);
Whilst here can you change the functions to be of the form "foo: function foo(a, b)"

>   },
>   alertCheck : function(dialogTitle, text, checkBoxLabel, checkObj)
>   {
>-    AlertWithTitle(dialogTitle, text);
>+    Services.prompt.alert(window, dialogTitle, text);
Whilst here can you change the functions to be of the form "foo: function foo(a, b)"

If you want me to be more explicit as to which changes should be dropped, then let me know.
Attachment #723185 - Flags: review?(iann_bugzilla)
Attachment #723185 - Flags: review-
Attachment #723185 - Flags: feedback+
(In reply to Ian Neal from comment #7)
> Comment on attachment 723185 [details] [diff] [review]
> Use new string methods and querySelector in /editor/, v5, r=mconley
> 
> I'm not a fan of wholesale changing from var to let unless there is another
> reason to change the line of code, so I will r- for that reason alone.

I guess this is a point where you and I disagree. :) I treat "var" like asbestos, where I believe it should be removed / replaced upon discovery.

This is because I find that let "does what you expect", and I've seen too many confusing abuses of "var", like:

try {
  var x
} catch(e) {
  // something
}

// Do something with x

Can you think of a good reason to keep var around? What advantages does it give us?

Comment 9

4 years ago
(In reply to Mike Conley (:mconley) from comment #8)
> (In reply to Ian Neal from comment #7)
> > Comment on attachment 723185 [details] [diff] [review]
> > Use new string methods and querySelector in /editor/, v5, r=mconley
> > 
> > I'm not a fan of wholesale changing from var to let unless there is another
> > reason to change the line of code, so I will r- for that reason alone.
> 
> I guess this is a point where you and I disagree. :) I treat "var" like
> asbestos, where I believe it should be removed / replaced upon discovery.
> 
> This is because I find that let "does what you expect", and I've seen too
> many confusing abuses of "var", like:
> 
> try {
>   var x
> } catch(e) {
>   // something
> }
> 
> // Do something with x
> 
> Can you think of a good reason to keep var around? What advantages does it
> give us?

Like asbestos, unless you are changing the code (building) around the var (asbestos) then best leave undisturbed.

I would definitely want to split the wholesale var -> let changing from the other code tidy up if only to make those latter changes easier to review.

I would then review the "wholesale" changes in a follow-up patch. As I said I am happy to give more guidance as to which I consider to be "wholesale" or not.
Created attachment 732221 [details] [diff] [review]
patch, v6
Attachment #723185 - Attachment is obsolete: true
Attachment #732221 - Flags: review?(mconley)
Attachment #732221 - Flags: review?(iann_bugzilla)

Comment 11

4 years ago
Comment on attachment 732221 [details] [diff] [review]
patch, v6

>+++ b/editor/ui/composer/content/editor.js
>-  var nodelist = domdoc.getElementsByTagName("meta");
>-  if ( nodelist )
>+  if (domdoc.querySelector('meta'))
Nit: Elsewhere you have used ", so do so here too instead of ' 

>   // add title tag if not present
>-  var titlenodelist = editor.document.getElementsByTagName("title");
>-  if (headelement && titlenodelist && titlenodelist.length == 0)
>+  let titleNode = editor.document.querySelector("title");
>+  if (headelement && !titleNode)
Nit: You could inline titleNode here as it is only used in the one place.

>+++ b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
>           // "limitFirstChar" is the only filtering rule used for core attributes as of 8-20-01
>-          // Add more rules if necessary          
>-          limitFirstChar = name.indexOf("^") >= 0;
>+          // Add more rules if necessary
>+          limitFirstChar = name.contains("^");
>           if (limitFirstChar)
Nit: you could inline limitFirstChar here, removing the var limitFirstChar earlier.
>           {
>             menuitem = gDialog.AddHTMLAttributeNameInput.appendItem(name.replace(/\^/g, ""));
>             menuitem.setAttribute("limitFirstChar", "true");
>           }
>           else
>             gDialog.AddHTMLAttributeNameInput.appendItem(name);
>         }
>@@ -46,22 +46,22 @@ function BuildHTMLAttributeNameList()
>           var sep = document.createElementNS(XUL_NS, "menuseparator");
>           if (sep)
>             popup.appendChild(sep);
>         }        
>       }
>       else
>       {
>         // Get information about value filtering
>-        var forceOneChar = name.indexOf("!") >= 0;
>-        var forceInteger = name.indexOf("#") >= 0;
>-        var forceSignedInteger = name.indexOf("+") >= 0;
>-        var forceIntOrPercent = name.indexOf("%") >= 0;
>-        limitFirstChar = name.indexOf("\^") >= 0;
>-        //var required = name.indexOf("$") >= 0;
>+        let forceOneChar = name.contains("!");
>+        let forceInteger = name.contains("#");
>+        let forceSignedInteger = name.contains("+");
>+        let forceIntOrPercent = name.contains("%");
>+        limitFirstChar = name.contains("\^");
This would become let limitFirstChar = name.contains("\^");
>+        //var required = name.contains("$");
You might as well change this var to let.

>       if ( CheckAttributeNameSimilarity( nodeMap[i].nodeName, HTMLAttrs ) ||
>-          IsEventHandler( nodeMap[i].nodeName ) ||
>-          TrimString( nodeMap[i].nodeName.toLowerCase() ) == "style" ) {
>+           nodeMap[i].nodeName.toLowerCase().startsWith("on") ||
>+           nodeMap[i].nodeName.toLowerCase().trim() == "style" ) {
>         continue;   // repeated or non-HTML attribute, ignore this one and go to next
>       }
>       var name  = nodeMap[i].name.toLowerCase();
Move this to before the if statement and then you can use name in there.

>+++ b/editor/ui/dialogs/content/EdAEJSEAttributes.js
>@@ -70,41 +70,30 @@ function BuildJSEAttributeTable()

>       if( CheckAttributeNameSimilarity( nodeMap[i].nodeName, JSEAttrs ) )
>         continue;   // repeated or non-JS handler, ignore this one and go to next
>-      if( !IsEventHandler( nodeMap[i].nodeName ) )
>+      if (!nodeMap[i].nodeName.toLowerCase().startsWith("on"))
>         continue; // attribute isn't an event handler.
>       var name  = nodeMap[i].nodeName.toLowerCase();
Move this to before the if statement and then you can use name in there.

>+++ b/editor/ui/dialogs/content/EdDialogCommon.js
>   if (name)
>   {
>     name = name.toLowerCase();
>     if (name != "")
There is no need for this additional check.
>     {
>       var editor = GetCurrentEditor();
>       try {
>+        let metaNode = editor.document.querySelector('meta[name="' + name + '"]');
>+        if (metaNode)
>         {
>+          return metaNode;
>         }
I don't think there is any need for the if, just inline metaNode
>       } catch (e) {}
>     }
>   }
>   return null;
> }

>@@ -475,26 +473,20 @@ function GetHTTPEquivMetaElement(name)
> {
>   if (name)
>   {
>     name = name.toLowerCase();
>     if (name != "")
There is no need for this additional check.
>     {
>       var editor = GetCurrentEditor();
>       try {
>+        let metaNode = editor.document.querySelector('meta[http-equiv="' + name + '"]');
>+        if (metaNode)
>         {
>+          return metaNode;
>         }
I don't think there is any need for the if, just inline metaNode
>       } catch (e) {}
>     }
>   }
>   return null;
> }
There is probably a further bit of clean up that could happen with the functions GetMetaElement and GetHTTPEquivMetaElement and also the functions CreateMetaElement, CreateHTTPEquivMetaElement and CreateHTTPEquivElement but that is possibly outside the scope of this bug.

>+++ b/editor/ui/dialogs/content/EdImageOverlay.js
> function GetImageMap()
> {
>   var usemap = globalElement.getAttribute("usemap");
>   if (usemap)
>   {
>     gCanRemoveImageMap = true;
>+    let mapname = usemap.substr(1);
>     var mapCollection;
>     try {
>+      mapCollection = GetCurrentEditor().document.querySelector('[name="' + mapname + '"]');
You could probably just inline mapCollection and return here.
>     } catch (e) {}
>+    if (mapCollection)
>     {
>+      return mapCollection;
>     }
>   }
>   else
>   {
>     gCanRemoveImageMap = false;
>   }
> 
>   return null;

>+++ b/editor/ui/dialogs/content/EditorPublishProgress.js
>+  // Just set attribute for status icon
>+  // if we already have this filename
This comment could fit on a single line, add a full stop at the end too.

Did you remove all the TrimString calls or are there a few left? (I lost count).

I would like to do a final review, so only f+ for the moment.
Attachment #732221 - Flags: review?(iann_bugzilla) → feedback+
Created attachment 734379 [details] [diff] [review]
patch, v7

There are many calls to TrimString which I didn't change, ripping it completely out will require to check if the argument is always or string or can also be null.
Attachment #732221 - Attachment is obsolete: true
Attachment #732221 - Flags: review?(mconley)
Attachment #734379 - Flags: review?(mconley)
Attachment #734379 - Flags: review?(iann_bugzilla)

Comment 13

4 years ago
Comment on attachment 734379 [details] [diff] [review]
patch, v7

>+++ b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
>+      let name = nodeMap[i].name.toLowerCase();
>       if ( CheckAttributeNameSimilarity( nodeMap[i].nodeName, HTMLAttrs ) ||
>+           name.startsWith("on") ||
>+           name == "style" ) {
These two could be on a single line now.

>+++ b/editor/ui/dialogs/content/EdDialogCommon.js

> function GetMetaElement(name)
> {
>   if (name)
>   {
>     name = name.toLowerCase();
>+    var editor = GetCurrentEditor();
>+    try {
>+      return editor.document.querySelector('meta[name="' + name + '"]');
>+    } catch (e) {}
>   }
>   return null;
> }

> function GetHTTPEquivMetaElement(name)
> {
>   if (name)
>   {
>     name = name.toLowerCase();
>+    var editor = GetCurrentEditor();
>+    try {
>+      return editor.document.querySelector('meta[http-equiv="' + name + '"]');
>+    } catch (e) {}
>   }
>   return null;
> }
> 

Would anything be gained from merging GetMetaElement and GetHTTPEquivMetaElement into a single function with two arguments - name and metaType?
Similarly for CreateMetaElement and CreateHTTPEquivMetaElement?

I've also noticed that the function CreateHTTPEquivElement is surplus to requirements, so that could be removed as part of the cleanup.

Further testing to be done yet.
Created attachment 739056 [details] [diff] [review]
patch, v8

(In reply to Ian Neal from comment #13)
> Comment on attachment 734379 [details] [diff] [review]
> patch, v7
> 
> >+++ b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
> >+      let name = nodeMap[i].name.toLowerCase();
> >       if ( CheckAttributeNameSimilarity( nodeMap[i].nodeName, HTMLAttrs ) ||
> >+           name.startsWith("on") ||
> >+           name == "style" ) {
> These two could be on a single line now.
Changed.

> >+++ b/editor/ui/dialogs/content/EdDialogCommon.js
> 
> > function GetMetaElement(name)
> > {
> >   if (name)
> >   {
> >     name = name.toLowerCase();
> >+    var editor = GetCurrentEditor();
> >+    try {
> >+      return editor.document.querySelector('meta[name="' + name + '"]');
> >+    } catch (e) {}
> >   }
> >   return null;
> > }
> 
> > function GetHTTPEquivMetaElement(name)
> > {
> >   if (name)
> >   {
> >     name = name.toLowerCase();
> >+    var editor = GetCurrentEditor();
> >+    try {
> >+      return editor.document.querySelector('meta[http-equiv="' + name + '"]');
> >+    } catch (e) {}
> >   }
> >   return null;
> > }
> > 
> 
> Would anything be gained from merging GetMetaElement and
> GetHTTPEquivMetaElement into a single function with two arguments - name and
> metaType?
> Similarly for CreateMetaElement and CreateHTTPEquivMetaElement?
> 
> I've also noticed that the function CreateHTTPEquivElement is surplus to
> requirements, so that could be removed as part of the cleanup.
> 
> Further testing to be done yet.
Merged the function pairs into one. Thunderbird-Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=049c3eecb852
The failing XPCshell test is from trunk and got fixed later there.
Attachment #734379 - Attachment is obsolete: true
Attachment #734379 - Flags: review?(mconley)
Attachment #734379 - Flags: review?(iann_bugzilla)
Attachment #739056 - Flags: review?(mconley)
Attachment #739056 - Flags: review?(iann_bugzilla)

Comment 15

4 years ago
Comment on attachment 739056 [details] [diff] [review]
patch, v8

>+++ b/editor/ui/composer/content/editor.js

>+            let menuPopupChildren = document.querySelectorAll('[id="insertMenuPopup"] > :not(#insertChars)');
>+            for (let i = 0; i < menuPopupChildren.length; i++)
>             {
>+              menuPopupChildren.item(i).hidden = true;
>             }
Nit: you could get rid of the {} in this for statement.

>+    if (prefAuthorString && prefAuthorString != 0 && !authorFound && headelement)
>     {
>+      /* create meta tag with 2 attributes */
Nit: use // for comments.

>+++ b/editor/ui/composer/content/editorUtilities.js

> function GetDocumentBaseUrl()
> {
>   try {
>     var docUrl;
> 
>     // if document supplies a <base> tag, use that URL instead
>+    let base = GetCurrentEditor().document.querySelector("base");
>+    if (base)
>     {
>+      docUrl = base.getAttribute("href");
>     }
Nit: you could get rid of the {} in this if statement.

>+++ b/editor/ui/dialogs/content/EdAEHTMLAttributes.js
>+          // only filtering rule used for core attributes as of 8-20-01
>+          // Add more rules if necessary
Nit: full stop at the end of the comment if starting with a capital letter.

>+      if (!name.startsWith("_moz") &&
>+          AddTreeItem(name, nodeMap[i].value, "HTMLAList", HTMLAttrs) )
Nit: remove the space between )s
>       {
>         added = true;
>       }
Nit: you could get rid of the {} in this if statement.
Attachment #739056 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 739056 [details] [diff] [review]
patch, v8

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

Great cleanup! Just a few recommendations. r=me with those fixed.

::: editor/ui/composer/content/editor.js
@@ +1554,5 @@
>            var doctypeText = "<!DOCTYPE " + domdoc.doctype.name;
>            if (dt.publicId)
>              doctypeText += " PUBLIC \"" + domdoc.doctype.publicId;
>            if (dt.systemId)
> +            doctypeText += " \"" + dt.systemId;

Heh, nice cleanup. :)

@@ +2163,2 @@
>  
>      var prefAuthorString = 0;

0? Maybe we should use null instead. That saves the prefAuthorString != 0 below, I think.

::: editor/ui/dialogs/content/EdAEHTMLAttributes.js
@@ +53,5 @@
> +        let forceInteger = name.contains("#");
> +        let forceSignedInteger = name.contains("+");
> +        let forceIntOrPercent = name.contains("%");
> +        let limitFirstChar = name.contains("\^");
> +        //let required = name.contains("$");

I guess this line can go.

@@ +94,5 @@
>    {
>      var added = false;
>      for(i = 0; i < nodeMap.length; i++)
>      {
> +      let name = nodeMap[i].name.toLowerCase();

To be functionally equivalent, I think we need to trim this too. Maybe that's being overcautious, but something I noticed.

@@ +101,3 @@
>          continue;   // repeated or non-HTML attribute, ignore this one and go to next
>        }
> +      if (!name.startsWith("_moz") &&

added = (!name.startsWith("_moz") &&
         AddTreeItem(name, nodeMap[i].value, "HTMLAList", HTMLAttrs));
Attachment #739056 - Flags: review?(mconley) → review+
Created attachment 751495 [details] [diff] [review]
patch, v9, r=IanN r=mconley
Attachment #739056 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/72aff817d8ab
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
Created attachment 751783 [details] [diff] [review]
follow-up patch, v1, r=mconley

When I tried to qpop the patch locally, the system detected a not yet qref-ed change. I hadn't saved a file change before exporting the patch, so this is a one-liner follow-up.
Keywords: checkin-needed
Whiteboard: [check in follow-up patch into comm-central, more info in comment 19]
https://hg.mozilla.org/comm-central/rev/bc3dd8a55dd2
Keywords: checkin-needed
Whiteboard: [check in follow-up patch into comm-central, more info in comment 19]
You need to log in before you can comment on or make changes to this bug.