Last Comment Bug 839279 - Code cleanup in /editor/: Use new String methods like startsWith, endsWith, contains and use querySelector instead of NodeList calls
: Code cleanup in /editor/: Use new String methods like startsWith, endsWith, c...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 24.0
Assigned To: Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-07 15:16 PST by Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
Modified: 2013-05-20 13:56 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Use new string methods and querySelector in /editor/, v1 (100.93 KB, patch)
2013-02-07 15:30 PST, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
mconley: review+
Details | Diff | Splinter Review
Use new string methods and querySelector in /editor/, v3 (101.57 KB, text/plain)
2013-03-01 10:33 PST, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
no flags Details
Use new string methods and querySelector in /editor/, v4 (101.88 KB, patch)
2013-03-01 13:05 PST, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
no flags Details | Diff | Splinter Review
Use new string methods and querySelector in /editor/, v5, r=mconley (101.79 KB, patch)
2013-03-10 03:44 PDT, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
iann_bugzilla: review-
iann_bugzilla: feedback+
Details | Diff | Splinter Review
patch, v6 (52.48 KB, patch)
2013-04-02 01:23 PDT, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
iann_bugzilla: feedback+
Details | Diff | Splinter Review
patch, v7 (52.96 KB, patch)
2013-04-07 08:09 PDT, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
no flags Details | Diff | Splinter Review
patch, v8 (56.26 KB, patch)
2013-04-18 07:34 PDT, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
iann_bugzilla: review+
mconley: review+
Details | Diff | Splinter Review
patch, v9, r=IanN r=mconley (56.28 KB, patch)
2013-05-19 11:40 PDT, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
no flags Details | Diff | Splinter Review
follow-up patch, v1, r=mconley (1.17 KB, patch)
2013-05-20 12:07 PDT, Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
no flags Details | Diff | Splinter Review

Description Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-02-07 15:16:36 PST
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.
Comment 1 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-02-07 15:30:03 PST
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
Comment 2 Mike Conley (:mconley) 2013-02-28 19:23:54 PST
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.
Comment 3 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-03-01 10:33:54 PST
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.
Comment 4 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-03-01 13:05:46 PST
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.
Comment 5 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-03-10 03:44:40 PDT
Created attachment 723185 [details] [diff] [review]
Use new string methods and querySelector in /editor/, v5, r=mconley
Comment 6 Ian Neal 2013-03-10 03:52:10 PDT
As this is shared code it will need review from the SM side too.
Comment 7 Ian Neal 2013-03-18 15:25:32 PDT
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.
Comment 8 Mike Conley (:mconley) 2013-03-19 06:39:34 PDT
(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 Ian Neal 2013-03-19 17:18:47 PDT
(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.
Comment 10 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-04-02 01:23:00 PDT
Created attachment 732221 [details] [diff] [review]
patch, v6
Comment 11 Ian Neal 2013-04-06 14:57:53 PDT
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.
Comment 12 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-04-07 08:09:02 PDT
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.
Comment 13 Ian Neal 2013-04-13 20:04:33 PDT
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.
Comment 14 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-04-18 07:34:43 PDT
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.
Comment 15 Ian Neal 2013-04-29 11:43:04 PDT
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.
Comment 16 Mike Conley (:mconley) 2013-05-18 09:18:04 PDT
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));
Comment 17 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-05-19 11:40:10 PDT
Created attachment 751495 [details] [diff] [review]
patch, v9, r=IanN r=mconley
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-05-20 05:02:08 PDT
https://hg.mozilla.org/comm-central/rev/72aff817d8ab
Comment 19 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-05-20 12:07:57 PDT
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.
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-05-20 13:56:46 PDT
https://hg.mozilla.org/comm-central/rev/bc3dd8a55dd2

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