Closed Bug 1387522 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

This method will become a lot slower when bug 651120 lands.
Assignee: nobody → ehsan
Blocks: 651120
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...
Attachment #8893896 - Flags: review?(masayuki) → review+
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?
(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!
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
No worries!
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d511913b5284
Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp; r=masayuki
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
Flags: needinfo?(ehsan)
(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...
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3618c30d179e
Avoid using nsINode::GetChildAt() in HTMLStyleEditor.cpp; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/3618c30d179e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.