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)
Core
DOM: Editor
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 | |
Bug 1465702 - part 2: Make public methods of EditorBase create AutoEditActionDataSetter if necessary
47 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1465702 - part 3: Make public methods of TextEditor create AutoEditActionDataSetter if necessary
47 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1465702 - part 4: Make public methods of HTMLEditor create AutoEditActionDataSetter if necessary
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.
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a35584ea9fd0c1fbcc86ca8652aa6414210066a9
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c80b80d46aa9edb94b366130a0fb3d8072da595
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be24cacb44b3f5705046db8b75c3e92443f445ef
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccc53c93b0f42c9c08338acdbfd823a6f0348a85
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f89cc3a2b178d659bf1eabf877278fea65460e96
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed445f3f2cd4bc3a70df644c9c8af1e570a22c25
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8412ce7183296a8e54137861362b368f73911462
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=819d38e6b82e2879ef3e25bb03f7027616a412fc
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7d3494a9a2658d1fdeb904c993fbbc2c7cabae
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df3d3322859ee0da29291d52c67e5262a0d1e7b
Assignee | ||
Comment 12•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fe5eac89a84eba559ec4fab9be2689ae9c01383
Assignee | ||
Comment 13•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=494ec3e111566ec1a12d9bbdb281a20d50910594
Assignee | ||
Comment 14•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab8385ea4a51d81f7ed7f66b707cbc4459650e6
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=295ad0f31d46f8c82461abc76ddc95baeaa2ab20
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f19695d34dfb69b720dac32b34bfd6ec278e73
Assignee | ||
Comment 17•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ed0f82a8444044c1dc5c03aa90cc9834aefa2d
Assignee | ||
Comment 18•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5365b7e9f292c3f49061789b022d6d034806dac
Assignee | ||
Comment 19•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6a1e58b4974d70a2aa8332d23585b2241ee17c3
Assignee | ||
Comment 20•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed75d62a74c7363a93efc3597b1fd1ee624fd56
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dafbb6a5fa6e31342c77bff45e50829b02d6021
Assignee | ||
Comment 22•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f406ca2b5d746fe48a1344eb13693945af6ed1
Assignee | ||
Comment 23•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f904c26b511eb6d8b4910a3ee968a5cbb5469d60
Assignee | ||
Comment 24•6 years ago
|
||
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.
Assignee | ||
Comment 25•6 years ago
|
||
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.
Assignee | ||
Comment 26•6 years ago
|
||
Assignee | ||
Comment 27•6 years ago
|
||
Assignee | ||
Comment 28•6 years ago
|
||
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.
Assignee | ||
Comment 29•6 years ago
|
||
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.
Assignee | ||
Comment 30•6 years ago
|
||
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.
Updated•6 years ago
|
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()
Comment 31•6 years ago
|
||
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
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc8d04abd39b https://hg.mozilla.org/mozilla-central/rev/1c3326439c30 https://hg.mozilla.org/mozilla-central/rev/27ca7b49a11e https://hg.mozilla.org/mozilla-central/rev/986f21f1f5d5 https://hg.mozilla.org/mozilla-central/rev/1945bfc27c6d https://hg.mozilla.org/mozilla-central/rev/258672244b3e https://hg.mozilla.org/mozilla-central/rev/bda6c79cf03c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Updated•6 years ago
|
status-firefox62:
affected → ---
Comment 33•6 years ago
|
||
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)
Comment 34•6 years ago
|
||
Adding a crash report if that may help: https://crash-stats.mozilla.com/report/index/b57a2b4b-1b98-4d4b-82a6-647e00181103
Assignee | ||
Comment 35•6 years ago
|
||
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)
Assignee | ||
Updated•5 years ago
|
Blocks: redesign-editor-module
You need to log in
before you can comment on or make changes to this bug.
Description
•