542919 - Refactor the plain text editor initialization to facilitate lazy initialization

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

The patch attached to this bug refactors the initialization steps for the plain text editor by adding the EnsureEditorInitialized API in order to facilitate the work on lazily initializing the editor in bug 221820.

Comment 1

[Jeff Walden [:Waldo]]

Er, which attached patch?  :-)  Or did you mean "to be attached"?

Comment 2

[(no longer active)]

(In reply to comment #1)
> Er, which attached patch?  :-)  Or did you mean "to be attached"?

Well, splitting the patch took longer than I expected, so I thought I'd file the bug and attach the patch later.  Little did I know that I would need to edit the description for it to make sense...  :-)

I'll attach the patch as soon as it's ready.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

This patch introduces the EnsureEditorInitialized and makes the current delayed initialization use that API.  The order of execution remains the same (which means that the editor is still initialized lazily with this patch.)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> This patch introduces the EnsureEditorInitialized and makes the current delayed
> initialization use that API.  The order of execution remains the same (which
> means that the editor is still initialized lazily with this patch.)

I thought it was still supposed to be initialized eagerly with this patch?

+  // never get used.  So, now this method is being called lazily only
+  // when we actually need an editor.

So this comment is invalid.

Comment 5

[(no longer active)]

Attachment: Patch (v1.1)

(In reply to comment #4)
> (In reply to comment #3)
> > This patch introduces the EnsureEditorInitialized and makes the current delayed
> > initialization use that API.  The order of execution remains the same (which
> > means that the editor is still initialized lazily with this patch.)
> 
> I thought it was still supposed to be initialized eagerly with this patch?

Well, yes, I was thinking eagerly, typed lazily.  Sorry for the confusion!

> +  // never get used.  So, now this method is being called lazily only
> +  // when we actually need an editor.
> 
> So this comment is invalid.

You're right.  I moved this comment to the patch which makes the initialization lazy.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 424716 [details] [diff] [review]
Patch (v1.1)

>+++ b/layout/forms/nsTextControlFrame.cpp
>-  if (IsFocusedContent(GetContent()))

This slightly changes the logic, in particular to not call SetFocus if editor init errors out.  I _think_ that's ok, but double-check?

r=bzbarsky

Comment 7

[(no longer active)]

(In reply to comment #6)
> (From update of attachment 424716 [details] [diff] [review])
> >+++ b/layout/forms/nsTextControlFrame.cpp
> >-  if (IsFocusedContent(GetContent()))
> 
> This slightly changes the logic, in particular to not call SetFocus if editor
> init errors out.  I _think_ that's ok, but double-check?

You're right.  I think it's OK, but I don't know how to make the editor initialization fail (except for manually returning a failure code) to test this in real life.

Do you have any ideas how to test this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmmm.  Editor init fails in these cases:

1)  Failed createInstance on the editor.
2)  The presshell has no document.
3)  Calling Init() on the editor fails.
4)  Calling GetControllers on the input fails.
5)  Getting an nsIControllerContext controller fails.
6)  GetFlags on the editor fails.
7)  SetFlags on the editor fails.
8)  EnableUndo fails.

In practice, 1, 2, 6, 7 can't happen sanely.  I don't know about 3, 4, 5, 8, but I bet they can't either...

Comment 9

[(no longer active)]

Attachment: Patch (v1.2)

I wrapped the code in an object's dtor to preserve the current behavior.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 425876 [details] [diff] [review]
Patch (v1.2)

That would run the code after the scriptblocker's destructor.  It needs to run before.  In fact, you want to move the new block to down below the PushNull() call.

Comment 11

[(no longer active)]

Attachment: Patch (v1.3)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426259 [details] [diff] [review]
Patch (v1.3)

r=bzbarsky

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e6cf06cfcef9

Comment 14

[(no longer active)]

I backed this out because I suspect that this might have caused a crash on Windows mochitests:

http://hg.mozilla.org/mozilla-central/rev/132cbfefc488

Failure log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1266532728.1266535683.5721.gz&fulltext=1#err0

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/32471a45b39b

Comment 16

[(no longer active)]

Backed out again as part of http://hg.mozilla.org/mozilla-central/rev/309acf8ab20f.

Comment 17

[(no longer active)]

See bug 557689 comment 11 for more information.

http://hg.mozilla.org/mozilla-central/rev/3dcfd44195d6
http://hg.mozilla.org/mozilla-central/rev/3c1ac0bbeb52