Joshua Cranmer [:jcranmer] Assignee
	Description • 12 years ago

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.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 1 • 12 years ago

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

	Kent James (:rkent)
	Comment 2 • 12 years ago

Not sure what this cryptic comment means "(JS is not C++ >_>)" but test_searchCustomTerm.js does not use E4X (at least not intentionally)

	Joshua Cranmer [:jcranmer] Assignee
	Comment 3 • 12 years ago

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.

	Ben Bucksch (:BenB)
	Comment 4 • 12 years ago

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.

	Ben Bucksch (:BenB)
	Comment 5 • 12 years ago

This is the code from https://developer.mozilla.org/en/JXON , plus the modification described in the last comment, and some more cleanup.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 6 • 12 years ago

This is a WIP, although it apparently causes massive issues with mozmill test failures.

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 11 years ago

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

	Ben Bucksch (:BenB)
	Comment 8 • 11 years ago

Please remove the commented-out E4X and "for each...in" code

	Joshua Cranmer [:jcranmer] Assignee
	Comment 9 • 11 years ago

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.

	Ben Bucksch (:BenB)
	Comment 11 • 11 years ago

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"

	Magnus Melin [:mkmelin]
	Comment 12 • 11 years ago

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.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 13 • 11 years ago

(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.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 14 • 11 years ago

This is for whoever gets here first.

	Ben Bucksch (:BenB)
	Comment 15 • 11 years ago

> * 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.

	Nicholas Nethercote [inactive]
	Comment 16 • 11 years ago

e4x support has now been removed from mozilla-central (bug 788293).

	Magnus Melin [:mkmelin]
	Comment 17 • 11 years ago

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!

	Ben Bucksch (:BenB)
	Comment 19 • 11 years ago

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?

	Ben Bucksch (:BenB)
	Comment 20 • 11 years ago

Oops! Sorry, my bad, I looked at the wrong patch. Stupid.

	Ben Bucksch (:BenB)
	Comment 21 • 11 years ago

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.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 22 • 11 years ago

All patches pushed:
https://hg.mozilla.org/comm-central/rev/eb1a4a4c5a76
https://hg.mozilla.org/comm-central/rev/078dd301fa0c

	Ben Bucksch (:BenB)
	Comment 23 • 11 years ago

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!