Open Bug 543552 Opened 14 years ago Updated 3 years ago

Initialize textareas lazily

Categories

(Core :: DOM: Editor, defect, P5)

x86
All
defect

Tracking

()

People

(Reporter: ehsan.akhgari, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file)

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.
Blocks: 543494
Blocks: 557720
Blocks: 559976
No longer blocks: 559976
This might be extremely hard, because we need to show misspelling suggestions before the editor for textareas is initialized.  (See bug 569397)
> 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.
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?
(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.
(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.
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.
Summary: Initialize textarea's lazily → Initialize textareas lazily
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)
Yeah, this shows no improvement at all on the bug 543494 benchmark.  No idea what I'm doing wrong.
No longer blocks: 543494, 557720
No longer depends on: 221820
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?
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)
Blocks: 543494, 557720
Depends on: 221820
Unassigning, since I don't really think I'm going to work on this anytime soon.
Assignee: ayg → nobody
Status: ASSIGNED → NEW

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.

Attachment

General

Created:
Updated:
Size: