Open
Bug 543552
Opened 15 years ago
Updated 4 years ago
Initialize textareas lazily
Categories
(Core :: DOM: Editor, defect, P5)
Tracking
()
NEW
People
(Reporter: ehsan.akhgari, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: memory-footprint, perf)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
Bug 221820 makes the initialization of text and password inputs lazy (on-demand, and not on page load). We should do the same for textareas.
Reporter | ||
Comment 1•15 years ago
|
||
This might be extremely hard, because we need to show misspelling suggestions before the editor for textareas is initialized. (See bug 569397)
Comment 2•15 years ago
|
||
> because we need to show misspelling suggestions before the editor for textareas > is initialized. (See bug 569397) Do we, though? Let's take that up in bug 569397.
Comment 3•13 years ago
|
||
In bug 364914 it seems like the conclusion was we didn't want to show misspelling suggestions until the textarea is focused. If I wanted to work on this, what would I need to do?
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Aryeh Gregor from comment #3) > In bug 364914 it seems like the conclusion was we didn't want to show > misspelling suggestions until the textarea is focused. If I wanted to work > on this, what would I need to do? I'm not sure if I understand the question. Bug 364914 changed our behavior so that the contents of the textarea are not spell checked until it is focused for the first time, which means that you don't need to have an editor for the textarea element before it is focused.
Comment 5•13 years ago
|
||
(Oops -- I wasn't CC'd to the bug, so I didn't notice your reply! Sorry.) "this" just meant "this bug". If I wanted to work on this bug, what would I need to do? The first sentence was just by way of commenting on comment 2.
Reporter | ||
Comment 6•13 years ago
|
||
Ah, OK. This is the main place where we decide to init textareas eagerly: <http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#430>. You need to change this so that the same thing would happen for textareas. Then you probably want to add something similar to this <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1801> to nsHTMLTextAreaElement, so that the events which need to initialize it can properly do so. Then you need to test things like whether you're breaking normal editing capabilities of textareas, things like drag/dropping content on top of them before they're focused for the first time, being able to scroll their contents before they're focused for the first time, etc. Then you need to run the test suite and see what else is broken, and fix it. I believe most of the infrastructure for this has been implemented in bug 221820, and there has been a ton of regression fixes after it, so you should be walking on solid ground, I hope.
Updated•13 years ago
|
Summary: Initialize textarea's lazily → Initialize textareas lazily
Comment 7•13 years ago
|
||
This isn't ready for real review because it has no tests. It compiles, and basic manual interaction with textareas seems to work. I don't notice an obvious performance improvement on pages with lots of textareas, so I don't know if it's actually doing what I want -- I don't actually understand this code. I'm also not sure what things I should test, or if maybe this is already adequately covered by existing tests. So I'm really just submitting this for feedback on the approach and what I should do differently, how I should test it, how I should make sure it's helping perf the way we want, etc. Try run: https://tbpl.mozilla.org/?tree=Try&rev=e455b7261a1a
Assignee: ehsan → ayg
Attachment #625487 -
Flags: feedback?(ehsan)
Comment 8•13 years ago
|
||
Yeah, this shows no improvement at all on the bug 543494 benchmark. No idea what I'm doing wrong.
Comment 9•13 years ago
|
||
Comment on attachment 625487 [details] [diff] [review] Patch v1, no tests Review of attachment 625487 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLTextAreaElement.cpp @@ +706,5 @@ > + // Initialize the editor if needed. > + if (NeedToInitializeEditorForEvent(aVisitor)) { > + nsITextControlFrame* textControlFrame = do_QueryFrame(GetPrimaryFrame()); > + if (textControlFrame) > + textControlFrame->EnsureEditorInitialized(); Coding style requires you to do: if (foo) { bar; } ::: layout/forms/nsTextControlFrame.cpp @@ +426,5 @@ > rv = UpdateValueDisplay(false); > NS_ENSURE_SUCCESS(rv, rv); > > + // Anything other than a text control is eagerly initialized > + bool initEagerly = !IsSingleLineTextControl() && !IsTextArea(); nsTextControlFrame should *always* be text controls, shouldn't they?
Reporter | ||
Comment 10•13 years ago
|
||
Comment on attachment 625487 [details] [diff] [review] Patch v1, no tests Review of attachment 625487 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/nsTextControlFrame.cpp @@ +426,5 @@ > rv = UpdateValueDisplay(false); > NS_ENSURE_SUCCESS(rv, rv); > > + // Anything other than a text control is eagerly initialized > + bool initEagerly = !IsSingleLineTextControl() && !IsTextArea(); Yes. Basically, you need to set initEagerly to the result of HasCachedSelection() below (as this will just turn into |bool initEagerly = false;|). @@ +434,5 @@ > nsCOMPtr<nsITextControlElement> txtCtrl = do_QueryInterface(GetContent()); > NS_ASSERTION(txtCtrl, "Content not a text control element"); > initEagerly = txtCtrl->HasCachedSelection(); > } > if (!initEagerly) { Is it possible that you're falling into this case here, and GetSpellcheck is setting initEagerly to true unconditionally? Other than that, this seems like what you should be doing.
Attachment #625487 -
Flags: feedback?(ehsan)
Reporter | ||
Updated•13 years ago
|
Comment 11•12 years ago
|
||
Unassigning, since I don't really think I'm going to work on this anytime soon.
Assignee: ayg → nobody
Status: ASSIGNED → NEW
Comment 12•4 years ago
|
||
Bulk-downgrade of unassigned, >=3 years untouched DOM/Storage bug's priority.
If you have reason to believe this is wrong, please write a comment and ni :jstutte.
Severity: normal → S4
Priority: -- → P5
You need to log in
before you can comment on or make changes to this bug.
Description
•