Optimize nsTextEditorState::GetValue()

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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 6

2 years ago
mozreview-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+

Comment 7

2 years ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
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.