1387522 - Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[(no longer active)]

This method will become a lot slower when bug 651120 lands.

Comment 1

[(no longer active)]

Attachment: Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8893896 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp

I wonder, if there was a way to loop all children with range based loop or iterator, that'd make us happy...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

BTW, if the for loop may change the children, how can we write the loop? Should we create auto array which has all children first?

Comment 4

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> Comment on attachment 8893896 [details] [diff] [review]
> Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp
> 
> I wonder, if there was a way to loop all children with range based loop or
> iterator, that'd make us happy...

We'd need to add types such as iterator, const_iterator, reverse_iterator and const_reverse_iterator to nsINode that conform to the standard iterator types.  It's not hard to do but a lot of work.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> BTW, if the for loop may change the children, how can we write the loop?
> Should we create auto array which has all children first?

Yeah that's how we'd normally do it.  Should I do that here?  It seems like RelativeFontChangeHelper() could do anything!

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away Aug 7-8) from comment #4)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> > Comment on attachment 8893896 [details] [diff] [review]
> > Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp
> > 
> > I wonder, if there was a way to loop all children with range based loop or
> > iterator, that'd make us happy...
> 
> We'd need to add types such as iterator, const_iterator, reverse_iterator
> and const_reverse_iterator to nsINode that conform to the standard iterator
> types.  It's not hard to do but a lot of work.

Yeah, I didn't mean that you should do that in this bug.

> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> > BTW, if the for loop may change the children, how can we write the loop?
> > Should we create auto array which has all children first?
> 
> Yeah that's how we'd normally do it.  Should I do that here?  It seems like
> RelativeFontChangeHelper() could do anything!

I don't think so. I just had the question. Sorry for unclear asking.

Comment 6

[(no longer active)]

No worries!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d511913b5284
Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp; r=masayuki

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing mochitest editor/libeditor/tests/test_bug767684.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4be1e24368459311e7e1cc2bddf895f5f205e829

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d511913b5284d14fc7afbca119b04e88c1a27207&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122863118&repo=mozilla-inbound

10:56:14     INFO - TEST-START | editor/libeditor/tests/test_bug767684.html
10:56:14     INFO - TEST-INFO | started process screencapture
10:56:14     INFO - TEST-INFO | screencapture: exit 0
10:56:14     INFO - TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug767684.html | All selected text must be embiggened - got "<big>foo</big><b>bar</b>baz", expected "<big>foo<b>bar</b>baz</big>"
10:56:14     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
10:56:14     INFO -     @editor/libeditor/tests/test_bug767684.html:13:1

Comment 9

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #5)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> > > BTW, if the for loop may change the children, how can we write the loop?
> > > Should we create auto array which has all children first?
> > 
> > Yeah that's how we'd normally do it.  Should I do that here?  It seems like
> > RelativeFontChangeHelper() could do anything!
> 
> I don't think so. I just had the question. Sorry for unclear asking.

Well, it turns out I needed to do exactly that here!  :-)  Thankfully the tests caught me...

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3618c30d179e
Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp; r=masayuki

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3618c30d179e