Closed Bug 1395146 Opened 6 years ago Closed 6 years ago

Optimize nsTextEditorState::GetValue()

Categories

(Core :: DOM: Editor, enhancement)

Product:
Core
Component:
DOM: Editor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1395146 - part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor()
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1395146 - part3: nsTextEditorState should cache empty value with mCachedValue
59 bytes, text/x-review-board-request
smaug
: review+
Details 
I still see nsTextEditorState::GetValue() when running attachment 8848015 [details]. Mainly, it's slow due to the cost of TextEditor::OutputToString() and number of calls of that. The reason is, nsTextEditorState doesn't use mCachedValue with <input>. I think that it's worthwhile to use it with any editable elements.

I wonder, IMEContentObserver may not need to observe mutations by itself since nsTextEditorState will observe all mutations of <input> and <textarea>. However, changing it needs more changes than I expected and it still needs to be a mutation observer for HTMLEditor. Therefore, even if we do this hack, we might be able to improve the performance of moving focus to/from <input> and <textarea> only a bit. So, I don't want to do that in this bug. If we'll find that it's a bottleneck of something, I'd work on it.
Comment on attachment 8902734 [details]
Bug 1395146 - part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor()

https://reviewboard.mozilla.org/r/174378/#review179624

Odd old code.
Attachment #8902734 - Flags: review?(bugs) → review+
Comment on attachment 8902735 [details]
Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control

https://reviewboard.mozilla.org/r/174380/#review179630

We should probably merge nsAnonDivObserver to nsTextEditorState to reduce memory usage and extra new/delete, but that can happen in a separate bug.

::: commit-message-274f5:1
(Diff revision 1)
> +Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor isn't single line text control r?ehsan

This comment talks about "even when ... isn't single line text control"

::: commit-message-274f5:3
(Diff revision 1)
> +Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor isn't single line text control r?ehsan
> +
> +Currently, nsTextEditorState caches the value of TextEditor only when it's for

but this about multi-line.
I guess this latter is correct, and the first sentence in the comment should have
s/isn't/is/
Attachment #8902735 - Flags: review+
Attachment #8902735 - Flags: review?(ehsan)
Comment on attachment 8902736 [details]
Bug 1395146 - part3: nsTextEditorState should cache empty value with mCachedValue

https://reviewboard.mozilla.org/r/174382/#review179644
Attachment #8902736 - Flags: review?(bugs) → review+
Comment on attachment 8902735 [details]
Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control

https://reviewboard.mozilla.org/r/174380/#review179630

> but this about multi-line.
> I guess this latter is correct, and the first sentence in the comment should have
> s/isn't/is/

Ah, yes. Exactly.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/bd95ed2a8abc
part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor() r=smaug
https://hg.mozilla.org/integration/autoland/rev/bd7325a8425e
part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control r=smaug
https://hg.mozilla.org/integration/autoland/rev/929846a88603
part3: nsTextEditorState should cache empty value with mCachedValue r=smaug
You need to log in before you can comment on or make changes to this bug.