Closed Bug 1570954 Opened 1 year ago Closed 1 year ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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

Attached patch bug1570954_rewrite.patch (obsolete) — Splinter Review

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

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9098633 - Flags: review?(paul)
Attached patch bug1570954_rewrite.manual.patch (obsolete) — Splinter Review

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

Fixed the test failure locally.

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?
Attachment #9098633 - Flags: feedback+

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!).

Attachment #9098633 - Attachment is obsolete: true
Attachment #9098633 - Flags: review?(paul)
Attachment #9098937 - Flags: review?(paul)

The manual parts, updated.

Attachment #9098634 - Attachment is obsolete: true
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 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.
Attachment #9098937 - Flags: review?(paul) → review+

Pushed by mkmelin@iki.fi:
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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0

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

Flags: needinfo?(mkmelin+mozilla)

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

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

Flags: needinfo?(mkmelin+mozilla)
Regressions: 1591399
Regressions: 1596685
Regressions: 1611442
You need to log in before you can comment on or make changes to this bug.