Closed Bug 542919 Opened 11 years ago Closed 11 years ago

Refactor the plain text editor initialization to facilitate lazy initialization

Categories

(Core :: DOM: Editor, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Er, which attached patch?  :-)  Or did you mean "to be attached"?
(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.
Attached patch Patch (v1) (obsolete) — Splinter Review
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.)
Attachment #424657 - Flags: superreview?(roc)
Attachment #424657 - Flags: review?(bzbarsky)
(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.
Attached patch Patch (v1.1) (obsolete) — Splinter Review
(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.
Attachment #424657 - Attachment is obsolete: true
Attachment #424716 - Flags: superreview?(roc)
Attachment #424716 - Flags: review?(bzbarsky)
Attachment #424657 - Flags: superreview?(roc)
Attachment #424657 - Flags: review?(bzbarsky)
Attachment #424716 - Flags: superreview?(roc) → superreview+
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
Attachment #424716 - Flags: review?(bzbarsky) → review+
(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?
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...
Attached patch Patch (v1.2) (obsolete) — Splinter Review
I wrapped the code in an object's dtor to preserve the current behavior.
Attachment #424716 - Attachment is obsolete: true
Attachment #425876 - Flags: review?(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.
Attachment #425876 - Flags: review?(bzbarsky) → review-
Attached patch Patch (v1.3)Splinter Review
Attachment #425876 - Attachment is obsolete: true
Attachment #426259 - Flags: review?(bzbarsky)
Comment on attachment 426259 [details] [diff] [review]
Patch (v1.3)

r=bzbarsky
Attachment #426259 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/e6cf06cfcef9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/mozilla-central/rev/32471a45b39b
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a2 → mozilla1.9.3a4
Backed out again as part of http://hg.mozilla.org/mozilla-central/rev/309acf8ab20f.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 557689
See bug 557689 comment 11 for more information.

http://hg.mozilla.org/mozilla-central/rev/3dcfd44195d6
http://hg.mozilla.org/mozilla-central/rev/3c1ac0bbeb52
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a4 → mozilla1.9.3a5
No longer depends on: 557689
You need to log in before you can comment on or make changes to this bug.