Last Comment Bug 759422 - Remove use of e4x in account creation
: Remove use of e4x in account creation
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: Joshua Cranmer [:jcranmer]
:
Mentors:
: 820922 (view as bug list)
Depends on: 843594
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-29 11:23 PDT by Joshua Cranmer [:jcranmer]
Modified: 2013-03-08 02:08 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
JXON.js, with tolerant |parent.foo| (5.78 KB, application/javascript)
2012-08-02 18:34 PDT, Ben Bucksch (:BenB)
Pidgeot18: feedback+
Details
WIP (33.22 KB, patch)
2012-08-17 06:27 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Splinter Review
Remove all known uses of E4X from c-c (41.58 KB, patch)
2013-01-30 17:46 PST, Joshua Cranmer [:jcranmer]
ben.bucksch: review-
Details | Diff | Splinter Review
Remove e4x from selectionsummaries.js (5.62 KB, patch)
2013-01-31 15:22 PST, Joshua Cranmer [:jcranmer]
mkmelin+mozilla: review+
Details | Diff | Splinter Review
Remove other uses of e4x (39.10 KB, patch)
2013-02-02 11:48 PST, Joshua Cranmer [:jcranmer]
ben.bucksch: review+
Details | Diff | Splinter Review

Description Joshua Cranmer [:jcranmer] 2012-05-29 11:23:06 PDT
The JS people are beginning the process of killing e4x support in SpiderMonkey.

With bug 753542, prefs will be added to disable e4x support in chrome/content, with the intent of flipping them to disable them by default before long. We should make sure that our code no longer uses e4x by that point in time.
Comment 1 Joshua Cranmer [:jcranmer] 2012-07-18 06:48:00 PDT
Complete list of files affected, as determined by failing to parse in non-e4x mode:
* mailnews/base/test/unit/test_searchCustomTerm.js (JS is not C++ >_>)
* mailnews/base/test/unit/test_autoconfigXML.js
* mailnews/base/prefs/content/accountcreation/readFromXML.js

Adding in people who use |new XML| :
* mail/components/cloudfile/nsYouSendIt.js
* mail/components/newmailaccount/content/uriListener.js
* mailnews/base/prefs/content/accountcreation/fetchConfig.js
* mailnews/base/prefs/content/accountcreation/fetchhttp.js
Comment 2 Kent James (:rkent) 2012-07-18 09:50:45 PDT
Not sure what this cryptic comment means "(JS is not C++ >_>)" but test_searchCustomTerm.js does not use E4X (at least not intentionally)
Comment 3 Joshua Cranmer [:jcranmer] 2012-08-02 13:55:10 PDT
The test_searchCustomTerm.js is a trivial patch which I already have locally. The others are mostly found in readFromXML.js. Basic plan of attack:

1. Make readFromXML take in a DOM document for the XML.
2. Use one of the JXON-y parsers to construct the object. Basic properties we ought to enforce:
-- <tag id="foo"> should be reified into tag["@id"] = "foo"
-- <a><b> ... </b></a> should become a 1-element array instead of that element. It simplifies a lot of issues.
-- for-of instead of for-each. The latter is actually an E4X-ism, and while it probably won't be deleted in Gecko 17, using the former is safer and more future-proof.
3. All callers should build the DOM document and pass it in.
Comment 4 Ben Bucksch (:BenB) 2012-08-02 18:32:28 PDT
Totally agreed about using JXON. We used it in an extension project where I had used E4X extensively, and it was not only an easy, straight-forward replacement, but actually make the code even easier to read.

One key was to remove a weakness of both E4X and JXON, with a slight modification to the JXON, the modification being based on the lesson learned in bug 531267. Namely, in normal JXON, the representation changes based on whether there is one or several elements of the same name. You cannot write code (without |if|) that handles both cases. I avoid that problem with the following rule: To access all <foo>s, you have to do |parent.$foo[n].bar|. (In Perl, $ stands for array, so that fits.) To access the first <foo> element, you do |parent.foo|. Both accessors are available in both cases, i.e. if there are 2 <foo>, you can either do |parent.$foo[0]| or |parent.foo|. If there is only 1 <foo>, you can still either do |parent.$foo[0]| or |parent.foo|. So, in cases where you expect exactly 1 element, you just do parent.foo.bar, and if the format is later extended or the file is malformed and there are suddently 2 <foo>s, your code still works. This makes the code a lot more future-proof (see bug 531267) and easier to read.

I'm attaching a version of JXON implementation that implements the above.
Comment 5 Ben Bucksch (:BenB) 2012-08-02 18:34:25 PDT
Created attachment 648588 [details]
JXON.js, with tolerant |parent.foo|

This is the code from https://developer.mozilla.org/en/JXON , plus the modification described in the last comment, and some more cleanup.
Comment 6 Joshua Cranmer [:jcranmer] 2012-08-17 06:27:41 PDT
Created attachment 652746 [details] [diff] [review]
WIP

This is a WIP, although it apparently causes massive issues with mozmill test failures.
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2013-01-29 09:35:08 PST
Comment on attachment 652746 [details] [diff] [review]
WIP

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

Hey jcranmer - the following problems were causing the account provisioner and account tests to fail.

::: mailnews/base/prefs/content/accountcreation/readFromXML.js
@@ +56,5 @@
>    exception = null;
>  
>    // incoming server
> +  //for each (let iX in xml.incomingServer) // input (XML)
> +  for (let iX of array_or_undef(xml.$incomingServer) // input (XML))

Missing close parens

@@ +144,5 @@
>    exception = null;
>  
>    // outgoing server
> +  //for each (let oX in xml.outgoingServer) // input (XML)
> +  for (let oX of array_or_undef(xml.$outgoingServer) // input (XML))

Missing close parens

@@ +417,5 @@
> +  this.build = function(oXMLParent, nVerbosity /* optional */, bFreeze /* optional */, bNesteAttributes /* optional */) {
> +    if (typeof oXMLParent === "string") {
> +      let domParser = Cc["@mozilla.org/xmlextras/domparser;1"]
> +                       .createInstance(Ci.nsIDOMParser);
> +      oXMLParent = domParser.parseFromString(xmlstr, "text/xml");

xmlstr is not defined
Comment 8 Ben Bucksch (:BenB) 2013-01-30 10:31:46 PST
Please remove the commented-out E4X and "for each...in" code
Comment 9 Joshua Cranmer [:jcranmer] 2013-01-30 17:46:02 PST
Created attachment 708406 [details] [diff] [review]
Remove all known uses of E4X from c-c

With bug 788293 moving towards completion, I think this has bumped up slightly in importance. I've also included the fix to bug 820922 in this (someone might want to check that my escaping works properly), and have gone ahead and deleted our override of e4x chrome disabling. Tests appear to have passed try.
Comment 10 Joshua Cranmer [:jcranmer] 2013-01-30 17:46:43 PST
*** Bug 820922 has been marked as a duplicate of this bug. ***
Comment 11 Ben Bucksch (:BenB) 2013-01-30 18:40:03 PST
Thanks a lot, jcranmer, for the patch.

Review:
* Please keep bug 820922 out of here. Any use of innerHTML would get r- from me.
* As said in comment 8, please remove the commented-out code. You did for some, but there's still more left.
* As for array_or_undef(), please modify JXON.js to always return an array for $foo, an empty array if necessary. This would remove the need for that function and look a lot nicer. Any other similar caller would need the same, so it's good to change JXON.js directly.
* Keep JXON in a separate file JXON.js. It's generic and can be used elsewhere in the tree.
* I like that we can remove a lot of "[0]"
* I like your use of "for..of"
Comment 12 Magnus Melin 2013-01-31 02:56:58 PST
I'd question if JXON is really the best way to go. It may make the changes required small, but personally i find it hard to read - at least if one isn't used to e4x, and not many people are. (I'd have tried using plain xpaths as the structure isn't that complicated.)

That said, good to see this worked on.
Comment 13 Joshua Cranmer [:jcranmer] 2013-01-31 15:10:31 PST
(In reply to Ben Bucksch (:BenB) from comment #11)
> Thanks a lot, jcranmer, for the patch.
> 
> Review:
> * Please keep bug 820922 out of here. Any use of innerHTML would get r- from
> me.

I can request someone else review that file individually if you like. Besides, I did not add that innerHTML. If it weren't for the fact that we're going to lose E4X on m-c in 48 hours or less, I would have rewritten it to not use it.

> * As for array_or_undef(), please modify JXON.js to always return an array
> for $foo, an empty array if necessary. This would remove the need for that
> function and look a lot nicer. Any other similar caller would need the same,
> so it's good to change JXON.js directly.

That's not really possible, since we would need to create a universal getter function or use a proxy, neither of which are easy to do.
Comment 14 Joshua Cranmer [:jcranmer] 2013-01-31 15:22:37 PST
Created attachment 708822 [details] [diff] [review]
Remove e4x from selectionsummaries.js

This is for whoever gets here first.
Comment 15 Ben Bucksch (:BenB) 2013-01-31 17:31:27 PST
> * As for array_or_undef(), please modify JXON.js to always return an array for $foo

Given that $foo isn't a function right now, and you can't make properties appear that aren't even mentioned in the XML, I agree with you and retract that review comment.
Comment 16 Nicholas Nethercote [:njn] 2013-02-02 00:53:51 PST
e4x support has now been removed from mozilla-central (bug 788293).
Comment 17 Magnus Melin 2013-02-02 03:48:39 PST
Comment on attachment 708822 [details] [diff] [review]
Remove e4x from selectionsummaries.js

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

Stealing the review. r=mkmelin with the below fixed

::: mail/base/content/selectionsummaries.js
@@ +109,5 @@
>    }
>  }
>  
> +function _mm_escapeHTML(aUnescaped) {
> +  return aUnescaped.replace('<', "&lt;")

need to be global replace.
 replace('<', "&lt;", "g") and so on...

@@ +111,5 @@
>  
> +function _mm_escapeHTML(aUnescaped) {
> +  return aUnescaped.replace('<', "&lt;")
> +                   .replace('>', "&gt;")
> +                   .replace('&', "&amp;");

... and you definitely want do the & replace first!
Comment 18 Joshua Cranmer [:jcranmer] 2013-02-02 11:48:43 PST
Created attachment 709412 [details] [diff] [review]
Remove other uses of e4x
Comment 19 Ben Bucksch (:BenB) 2013-02-03 15:44:39 PST
Please commit the account config parts in a separate commit than the other stuff.

yousendit.js
-          this._uploadResponse = docResponse;

Did you check that this isn't used anywhere? Because you remove it without replacement.

+  //d.id = sanitize.hostname(xml.@id);

You *still* have the commented-out old code in there. Please remove them all.
This is now the third time that I make this comment.

+const JXON = new (function() {

This belongs in its own file.

All points are still open; I don't know why you ask for review again. As said in my review in comment 11:
* Please keep bug 820922 out of here.
* Please remove the commented-out code. You did for some, but there's still more left.
* Keep JXON in a separate file JXON.js. It's generic and can be used elsewhere in the tree.
These are reasonable requests, are they not?
Comment 20 Ben Bucksch (:BenB) 2013-02-03 15:46:08 PST
Oops! Sorry, my bad, I looked at the wrong patch. Stupid.
Comment 21 Ben Bucksch (:BenB) 2013-02-03 15:59:02 PST
OK, ignore my comment 19. To my defense, it's 1 AM and it was a long day for me.
Real review:
All good, all points covered. Thanks a lot. r+

JXON.js: You changed:
1. Added license. (I can't confirm or deny, source is <https://developer.mozilla.org/en-US/docs/JXON> with "copyleft 2011", whatever that means, but I assume it's OK)
2. You added:
+    if (typeof oXMLParent === "string") {
+      let domParser = Cc["@mozilla.org/xmlextras/domparser;1"]
+                       .createInstance(Ci.nsIDOMParser);
+      oXMLParent = domParser.parseFromString(oXMLParent, "text/xml");
+    }
Not sure what this is supposed to do.

Please do commit the account config stuff separate from the rest, though. Should be easy enough.

Also, this comment stays:
yousendit.js
-          this._uploadResponse = docResponse;

Did you check that this isn't used anywhere? Because you remove it without replacement.
If you did check that and are sure that's fine, then that's fine with me.

Thanks, jcranmer. Nice code, good change.
Comment 23 Ben Bucksch (:BenB) 2013-02-03 16:24:14 PST
Discussed with jcranmer on IRC:
* License: Given the fact that new MDC code is public domain per <https://developer.mozilla.org/en-US/docs/Project:Copyrights>, we use the public domain header from <http://www.mozilla.org/MPL/headers/>.
* jcranmer moved the DOMParser stuff from JXON.js to fetchConfig.js. That means JXON.js is unchanged, apart from adding the copyright header.
* yousendit.js this._uploadResponse is indeed unused: <http://mxr.mozilla.org/comm-central/search?find=\.js%24&string=_uploadResponse>

That resolves all issues. Thanks a lot!

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