1570954 - Port bug 1479125 to Thunderbird: Rewrite callers firstChild/lastChild/childNodes/previousSibling/nextSibling with firstElementChild/lastElementChild/children/previousElementSibling/nextElementSibling

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Rewrite callers firstChild/lastChild/childNodes/previousSibling/nextSibling with firstElementChild/lastElementChild/children/previousElementSibling/nextElementSibling

https://hg.mozilla.org/integration/autoland/rev/c266a7f4237e - quoting the commit message:

This allows the JS to work in HTML documents, where whitespace is preserved. In XUL
documents, whitespace is ignored when parsing so text nodes are generally not returned.

The following changes were made, with manual cleanups as necessary (i.e. when firstChild actually refers to a text node, or when firstChild is used in a loop to empty out an element):

firstChild->firstElementChild
lastChild->lastElementChild
nextSibling->nextElementSibling
previousSibling->previousElementSibling
childNodes->children

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1570954_rewrite.patch

This is a pretty large patch. I'll attach one with the manual changes needed after the automatic replacements had been done. (See the hg commit message for what was done.)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dba03dc29c307ddc08241e88d82f74bffb0ccb8b

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1570954_rewrite.manual.patch

Manual changes. These are already folded in the main patch to ease review.

Comment 3

[Magnus Melin [:mkmelin]]

Fixed the test failure locally.

Comment 4

[Paul Morris [:pmorris]]

Comment on attachment 9098633 [details] [diff] [review]
bug1570954_rewrite.patch

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

Ooof, this is indeed a large one.  I looked closely over the manual changes (thanks!) and skimmed the automatic ones.  I found one typo to fix, and had a question about the TeXZilla.js file.  I'd be curious to know what the fix was for the test failure.

It is going to be nice once we're through this transition away from XBL/XUL.

::: calendar/test/modules/ItemEditingHelpers.jsm
@@ +387,5 @@
> +      for (let attachment of attachments) {
> +        if (attachment.tooltipText.includes(data.attachment.remove)) {
> +          dialog.click(new elementslib.Elem(attachment));
> +          dialog.keypress(attachmentBox, "VK_DELETE", {});
> +        }

Much better.

::: mail/components/compose/content/dialogs/EdLinkProps.js
@@ +133,5 @@
>        // Check if selection has some text - use that first
>        selectedText = GetSelectionAsText();
>        if (!selectedText) {
>          // No text, look for first image in the selection
> +        imageElement = anchorElement.querySelector("img");

Ah, so much better!

::: mail/components/compose/content/editor.js
@@ +1765,5 @@
>        // Start at actual selected node
>        var offset = editor.selection.anchorOffset;
>        // Note: If collapsed, offset points to element AFTER caret,
>        //  thus node may be null
> +      node = anchorNode.childNode.item(offset);

Typo: childNodes

::: mail/components/compose/texzilla/TeXZilla.js
@@ +692,5 @@
>  }}}l.fa=function(b){this.D=b};try{l.H=new XMLSerializer}catch(I){l.H={serializeToString:function(){throw"XMLSerializer undefined. Did you call TeXZilla.setXMLSerializer?";}}}l.ja=function(b){this.H=b};l.U=function(b){return this.D.parseFromString(b,"application/xml").documentElement};l.ia=function(b){this.f.w=b};l.ha=function(b){this.f.da=b};l.ca=function(b){"string"===typeof b&&(b=this.U(b));return C(b)};l.Z=function(b,a,d,e){var g;try{g=this.parse("\\("+b+"\\)")}catch(c){if(e)throw c;g=w(["<merror><mtext>"+
>  t(c.message)+"</mtext></merror>"],p,b)}d&&(g=g.replace(/^<math/,'<math dir="rtl"'));a&&(g=g.replace(/^<math/,'<math display="block"'));return g};l.Y=function(b,a,d,e){return this.U(this.Z(b,a,d,e))};l.na=function(b,a,d,e,g){var c,f;void 0===e&&(e=64);void 0===g&&(g=window.document);a=this.Y(b,h,a);a.setAttribute("mathsize",e+"px");e=document.createElement("div");e.style.visibility="hidden";e.style.position="absolute";e.appendChild(a);g.body.appendChild(e);c=a.getBoundingClientRect();g.body.removeChild(e);
>  e.removeChild(a);d?(d=Math.pow(2,Math.ceil(Math.log(c.width)/Math.LN2)),g=Math.pow(2,Math.ceil(Math.log(c.height)/Math.LN2))):(d=Math.ceil(c.width),g=Math.ceil(c.height));f=document.createElementNS("http://www.w3.org/2000/svg","svg");f.setAttribute("width",d+"px");f.setAttribute("height",g+"px");e=document.createElementNS("http://www.w3.org/2000/svg","g");e.setAttribute("transform","translate("+(d-c.width)/2+","+(g-c.height)/2+")");f.appendChild(e);e=document.createElementNS("http://www.w3.org/2000/svg",
> +"foreignObject");e.setAttribute("width",c.width);e.setAttribute("height",c.height);e.appendChild(a);f.firstElementChild.appendChild(e);a=new Image;a.src="data:image/svg+xml;base64,"+window.btoa(E(this.H.serializeToString(f)));a.width=d;a.height=g;a.alt=t(b);return a};l.R=function(b,a){try{return this.parse(b)}catch(d){if(a)throw d;return b}};l.Q=function(b,a){var d,e,f;for(f=b.firstElementChild;f;f=f.nextElementSibling)switch(f.nodeType){case 1:this.Q(f,a);break;case 3:this.f.P=h;d=this.D.parseFromString("<root>"+u.R(f.data,
> +a)+"</root>","application/xml").documentElement;for(this.f.P=p;e=d.firstElementChild;)b.insertBefore(d.removeChild(e),f);e=f.previousElementSibling;b.removeChild(f);f=e}};l.d=function(){return{K:1,parseError:function(b,a){if(this.f.V)this.f.V.parseError(b,a);else throw Error(b);},ga:function(b){this.h=b;this.v=this.C=this.t=p;this.g=this.r=0;this.a=this.i=this.match="";this.e=["INITIAL"];this.c={s:1,m:0,p:1,n:0};this.options.z&&(this.c.o=[0,0]);this.offset=0;return this},input:function(){var b=this.h[0];this.a+=

Hmm, what to make of this?  Looks like an external dependency (maybe https://fred-wang.github.io/TeXZilla/).  Usually that's not something we'd want to modify, right?.  Hard to know what the effects would be.  Might be fine since we will be changing from a XUL to an HTML context.  Any reassuring thoughts?

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1570954_rewrite.patch

Yeah you're right we shouldn't change TeXZilla.js
In general, the files I've now excluded are related to composition (or addr book since that has a childNodes, grr!).

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: bug1570954_rewrite.manual.patch

The manual parts, updated.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9098937 [details] [diff] [review]
bug1570954_rewrite.patch

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

::: calendar/lightning/modules/ltnInvitationUtils.jsm
@@ +307,1 @@
>        }

This is what what the test failure was about earlier, it really needs to be child and not elementchild. I notice this got into the "automatic" part now. But anyway.

Comment 8

[Paul Morris [:pmorris]]

Comment on attachment 9098937 [details] [diff] [review]
bug1570954_rewrite.patch

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

Looks good, typo is fixed and TeXZilla.js is not changed.  Thanks for the explanation of test failure.

Comment 9

[Magnus Melin [:mkmelin]]

Final try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1a7bb64dbbbc418984d2ef639bc6c8ede8c36276
Landing it later tonight.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a128f9557e9d
Port bug 1479125 to Thunderbird: Rewrite callers firstChild/lastChild/childNodes/previousSibling/nextSibling with firstElementChild/lastElementChild/children/previousElementSibling/nextElementSibling. r=pmorris

Comment 11

[Jorg K (CEST = GMT+2)]

Magnus, I think you caused a whole bunch of test failures here.
Already on your try:
TEST-UNEXPECTED-FAIL | Z:\task_1570966607\build\tests\mozmill\message-header\test-reply-identity.js | test-reply-identity.js::test_reply_no_matching_identity

And then on the push:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a128f9557e9db7951a9d8d6467e553fdccb8a0f5
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_matching_only_deliveredto
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/message-header/test-reply-identity.js | test-reply-identity.js::test_reply_matching_subaddress

Comment 12

[Jorg K (CEST = GMT+2)]

mozmake SOLO_TEST=message-header/test-reply-identity.js mozmill-one passes locally.

Comment 13

[Jorg K (CEST = GMT+2)]

Hmm, I tried to run the entire directory and crashed on test-phishing-bar.js with:

TEST-START | c:\mozilla-source\comm-central\comm\mail\test\mozmill\message-header\test-phishing-bar.js | test_no_phishing_warning_for_ip_sameish_text
[5788, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.cpp, line 176
++DOMWINDOW == 84 (000001FC4CD1C000) [pid = 5788] [serial = 95] [outer = 000001FC493B05C0]
[5788, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file c:/mozilla-source/comm-central/parser/html/nsHtml5StreamParser.cpp, line 1142
Assertion failure: (((HRESULT)(hr)) >= 0), at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/ShellHeaderOnlyUtils.h:132
#01: nsExternalHelperAppService::LoadURI (c:\mozilla-source\comm-central\uriloader\exthandler\nsExternalHelperAppService.cpp:966)
#02: XPTC__InvokebyIndex[c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\xul.dll +0x8dd6c62]
#03: CallMethodHelper::Call (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNative.cpp:1183)
#04: XPCWrappedNative::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNative.cpp:1149)
#05: XPC_WN_CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNativeJSOps.cpp:946)

I took some subtests of test-phishing-bar.js out and now the entire directory passes.

Here's a try run with that. I guess I need to file a new bug for the test-phishing-bar.js failures which drag everything else down:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=80ec64e7abea4411e3b5a0b075ac13e3c2226046