Closed Bug 1465702 Opened 6 years ago Closed 6 years ago

EditorBase, TextEditor and HTMLEditor should use a stack class to store all information which are necessary to handle each EditAction like TextEditRules and HTMLEditRules

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Since fix of bug 1463327, EditorBase, TextEditor and HTMLEditor have public methods only for starting to handle each edit action. And like the fix of bug 1460509, those classes should initialize a stack class in each public methods to share edit action information with all protected/private methods of them (even though some of them may be called by other helper classes but they are called while one of the public methods is being called).

So, here we should do:

* Create new stack class to store EditAction, top-level EditSubAction, Selection (and each of them should be accessible via protected methods).
* Define EditAction for each public method.
* Maybe need to split some public methods, one is for called by outside and the other is for called by outside.

Additionally, we need to check if it's kicked by XUL command or web content. This is required when we implement beforeinput event. So, if it can fix easily and quickly, I'll include this fix into the patches.

Additionally, those patches can reduce redundant calls of EditorBase::GetSelection(). The method is not cheap and I see it a lot when I check profile. So, must be good fix for Quantum Flow too.
Priority: -- → P3
There might be some public methods, which are not caught by my check, though, I finished filing bugs to split all remaining public methods which should be marked each edit action.
Like TextEditRules, EditorBase should have a stack class which cache necessary
objects and current handling edit action.  The edit action will be necessary
when we implement InputEvent.inputType.

Different from TextEditRules, this adds |const RefPtr<Selection>&| instead
of |Selection&|.  The reason is, when I add MOZ_CAN_RUN_SCRIPT to some methods,
it's not allowed like this:

> foo->CanRunScriptMethod(SelectionRef());

I'll update TextEditRules for consistency in the following patches.
This patch makes public methods of EditorBase create AutoEditActionDataSetter
as far as possible.  However, does not do so for some public methods if they are
not necessary to create it.
EditorBase::SelectionRefPtr() is now safe to use in editor and really fast to
retrieve Selection than EditorBase::GetSelection().  Therefore, we can get rid of
all Selection pointer/reference argument of each method which always take
normal Selection.

Note that this changes nsIHTMLEditor.checkSelectionStateForAnonymousButtons()
because its argument is only Selection.  So, BlueGriffon should work even
though it calls the method with nsIEditor.selection.
When each method of TextEditRules and HTMLEditRules should be called only by
EditorBase/TextEditor/HTMLEditor while the editor instance has
AutoEditActionDataSetter instance.  Therefore, we can get rid of all
pointers/references of Selection from each method's argument.

Additionally, this reimplements TextEditRules::SelectionRef() with
EditorBase::SelectionRefPtr() and make it return |const RefPtr<Selection>&|
rather than |Selection&| since RefPtr reference is required when we call
methods which are marked as MOZ_CAN_RUN_SCRIPT.
Now, any protected/private methods of editor classes can refer
SelectionRefPtr() safely.  So, we can cut the cost of calling GetSelection()
only once per edit action handling.
Attachment #9020634 - Attachment description: Bug 1465702 - part 7: Make protected/private methods of EditorBase/TextEditor/HTMLEditor use SelectionRef() instead of GetSelection() → Bug 1465702 - part 7: Make protected/private methods of EditorBase/TextEditor/HTMLEditor use SelectionRefPtr() instead of GetSelection()
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/fc8d04abd39b
part 1: Add EditorBase::AutoEditActionDataSetter to store edit action and grab Selection instance while editor classes handle an edit action r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1c3326439c30
part 2: Make public methods of EditorBase create AutoEditActionDataSetter if necessary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/27ca7b49a11e
part 3: Make public methods of TextEditor create AutoEditActionDataSetter if necessary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/986f21f1f5d5
part 4: Make public methods of HTMLEditor create AutoEditActionDataSetter if necessary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1945bfc27c6d
part 5: Remove unnecessary Selection argument from editor module r=m_kato
https://hg.mozilla.org/integration/autoland/rev/258672244b3e
part 6: Remove unnecessary Selection argument from TextEditRules and HTMLEditRules and rename/reimplement TextEditRules::SelectionRef() as TextEditRules::SelectionRefPtr() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bda6c79cf03c
part 7: Make protected/private methods of EditorBase/TextEditor/HTMLEditor use SelectionRefPtr() instead of GetSelection() r=m_kato
Hello there, I'm having reproducible crashes on Nightly when editing a table on MDN.
I was able to bisect to https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5cdef82c0fb01e17ab451c546f4bff1b90a1f7af&tochange=bda6c79cf03c921d16e4944fc200a424b4af78b0 using mozregression.

The STR for my crashes boil down to:
- Open an MDN page with tables (ex. https://developer.mozilla.org/en-US/docs/User:SphinxKnight/tabCrash )
- Edit it
- After a few strokes, the tab crashes

Please tell me if you prefer me to open another bug and/or provides more information.

Here is the relevant part of the log from mozregression:

54:08.08 INFO: Narrowed inbound regression window from [ba133f47, bda6c79c] (3 builds) to [5cdef82c, bda6c79c] (2 builds) (~1 steps left)
54:08.08 INFO: No more inbound revisions, bisection finished.
54:08.08 INFO: Last good revision: 5cdef82c0fb01e17ab451c546f4bff1b90a1f7af
54:08.08 INFO: First bad revision: bda6c79cf03c921d16e4944fc200a424b4af78b0
54:08.08 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5cdef82c0fb01e17ab451c546f4bff1b90a1f7af&tochange=bda6c79cf03c921d16e4944fc200a424b4af78b0
Flags: needinfo?(masayuki)
It's bug 1504093. Please file a new bug and add bug # which caused the regression into "blocks" when you find a new regression.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: