Closed Bug 1432528 Opened 6 years ago Closed 6 years ago

Optimize EditorBase::NotifyEditorObservers() to reduce call of nsIEditorObserver methods

Categories

(Core :: DOM: Editor, enhancement)

Product:
Core
Component:
DOM: Editor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

Bug 1432528 - part 1: Expose nsTextInputListener as mozilla::TextInputListener with independent header
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1432528 - part 2: Make EditorBase treat TextInputListener directly and make TextInputListener not derived from nsIEditorObserver
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1432528 - part 3: Make EditorBase store IMEContentObserver directly and make it not derived from nsIEditorObserver
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1432528 - part 4: Remove nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::CancelEditAction() because nobody implements them
59 bytes, text/x-review-board-request
m_kato
: review+
Details
TextInputListener.h at part 1
2.57 KB, text/plain
Details 
EditorBase::NotifyEditorObservers() calls methods of nsIEditorObserver. However, it's implemented by IMEContentObserver, nsTextEditorState and Bluegriffon, and only IMEContentObserver observes BeforeEditAction() and CancelEditAction(). So, we can make EditorBase store IMEContentObserver and nsTextEditorState instances directly and get rid of BeforeEditAction() and CancelEditAction() from nsIEditorObserver.
Comment on attachment 8945031 [details]
Bug 1432528 - part 1: Expose nsTextInputListener as mozilla::TextInputListener with independent header

https://reviewboard.mozilla.org/r/215242/#review220802


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/html/nsTextEditorState.cpp:808
(Diff revision 1)
> -, mSettingValue(false)
> +  , mSettingValue(false)
> -, mSetValueChanged(true)
> +  , mSetValueChanged(true)
>  {
>  }
>  
> -nsTextInputListener::~nsTextInputListener()
> +TextInputListener::~TextInputListener()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Hmm, the diff in attachment 8945031 [details] (part 1) is really ugly. Here is the new header file which is copied from nsTextEditorState.cpp when |hg up| to part 1.
Attachment #8945118 - Attachment is patch: false
Comment on attachment 8945031 [details]
Bug 1432528 - part 1: Expose nsTextInputListener as mozilla::TextInputListener with independent header

https://reviewboard.mozilla.org/r/215242/#review221120

::: dom/html/nsTextEditorState.cpp:825
(Diff revision 2)
>  {
>    bool collapsed;
>    AutoWeakFrame weakFrame = mFrame;
>  
> -  if (!aDoc || !aSel || NS_FAILED(aSel->GetIsCollapsed(&collapsed)))
> +  if (!aDOMDocument || !aSelection ||
> +      NS_FAILED(aSelection->GetIsCollapsed(&collapsed))) {

We should repalce GetIsCollpased with AsSelection()->IsCollapsed.  But I will handle it by anotehr bug.
Attachment #8945031 - Flags: review?(m_kato) → review+
Comment on attachment 8945032 [details]
Bug 1432528 - part 2: Make EditorBase treat TextInputListener directly and make TextInputListener not derived from nsIEditorObserver

https://reviewboard.mozilla.org/r/215244/#review221126
Attachment #8945032 - Flags: review?(m_kato) → review+
Comment on attachment 8945033 [details]
Bug 1432528 - part 3: Make EditorBase store IMEContentObserver directly and make it not derived from nsIEditorObserver

https://reviewboard.mozilla.org/r/215246/#review221134
Attachment #8945033 - Flags: review?(m_kato) → review+
Comment on attachment 8945034 [details]
Bug 1432528 - part 4: Remove nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::CancelEditAction() because nobody implements them

https://reviewboard.mozilla.org/r/215248/#review221136

bluegriffon uses nsIEditorObserver...
Attachment #8945034 - Flags: review?(m_kato) → review+
Comment on attachment 8945034 [details]
Bug 1432528 - part 4: Remove nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::CancelEditAction() because nobody implements them

https://reviewboard.mozilla.org/r/215248/#review221136

Yeah, unfortunately. Could be replaced with an event, but perhaps, it's slower than nsIEditorObserver.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/02c94b335609
part 1: Expose nsTextInputListener as mozilla::TextInputListener with independent header r=m_kato
https://hg.mozilla.org/integration/autoland/rev/39095009ebdd
part 2: Make EditorBase treat TextInputListener directly and make TextInputListener not derived from nsIEditorObserver r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a356d22ebcce
part 3: Make EditorBase store IMEContentObserver directly and make it not derived from nsIEditorObserver r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b1c58e1131b7
part 4: Remove nsIEditorObserver::BeforeEditAction() and nsIEditorObserver::CancelEditAction() because nobody implements them r=m_kato
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: