1433345 - A lot of classes related to editor listens to selection changes separately

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

class IMEContentObserver final : public nsISelectionListener
class TextInputListener final : public nsISelectionListener
class nsComposerCommandsUpdater : public nsISelectionListener
class ResizerSelectionListener final : public nsISelectionListener
class TypeInState final : public nsISelectionListener

If EditorBase can store those instances directly, they can listen to selection changes via EditorBase without nsISelectionListener.  Fortunately, first two classes are now stored by EditorBase directly.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsComposerCommandsUpdater can be stored by HTMLEditor directly:
https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/editor/composer/nsEditingSession.cpp#381,419

ResizerSelectionListener is already stored by HTMLEditor but as nsISelectionListener:
https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/editor/libeditor/HTMLEditor.cpp#303

TypeInState is already stored by HTMLEditor directly:
https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/editor/libeditor/HTMLEditor.cpp#300

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd487ff8b05fd6564c9fadad674914a72fc9da26

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e625279489830d5732e5fc66f0d76076947deb0

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10b1255fda077672e9a1fe91ef2b6c67d86e6034

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1433345 - part 1: Expose nsComposerCommandsUpdater with renaming it to mozilla::ComposerCOmmandsUpdater

For making HTMLEditor stores it directly, we should expose
nsComposerCommandsUpdater first.  Then, it should be in mozilla namespace.

Review commit: https://reviewboard.mozilla.org/r/215892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215892/

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1433345 - part 2: Make HTMLEditor store ComposerCommandsUpdater directly

For making ComposerCommandsUpdater not derived from nsISelectionListener,
HTMLEditor needs to store it directly.

This patch also makes ComposerCommandsUpdater cycle collectable because it
stores a strong pointer and HTMLEditor also needs to store it with RefPtr.
Therefore, ComposerCommandsUpdater becomes unnecessary to use nsWeakPtr.

Review commit: https://reviewboard.mozilla.org/r/215894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215894/

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1433345 - part 3: Make HTMLEditor store ResizerSelectionListener directly

This patch makes HTMLEditor store ResizerSelectionListener directly and
make it cycle collectable.  However, in the following patch, this class will
be removed completely because it doesn't necessary if HTMLEditor becomes a
selection listener.

Review commit: https://reviewboard.mozilla.org/r/215896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215896/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1433345 - part 4: Make EditorBase derived from nsISelectionListener and notify its owning classes of selection change

This patch makes EditorBase derived from nsISelectionListener.  Then, we can
make IMEContentObserver, TextInputListener, ComposerCommandsUpdater,
TypeInState not derived from nsISelectionListener since EditorBase or
HTMLEditor can notify them of selection change directly.  Additionally,
ResizerSelectionListener is not necessary anymore since it just implements
nsISelectionListener and calls only a method of HTMLEditor.  So, HTMLEditor
can call it directly.

Note that the order of selection listeners may be different.  However,
according to what each selection listener does, changing the order isn't
problem.

Review commit: https://reviewboard.mozilla.org/r/215898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215898/

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8945815 [details]
Bug 1433345 - part 1: Expose nsComposerCommandsUpdater with renaming it to mozilla::ComposerCOmmandsUpdater

https://reviewboard.mozilla.org/r/215892/#review221892

::: editor/composer/ComposerCommandsUpdater.h:27
(Diff revision 1)
>  class nsITransactionManager;
>  class nsPICommandUpdater;
>  
> -class nsComposerCommandsUpdater : public nsISelectionListener,
> -                                  public nsIDocumentStateListener,
> -                                  public nsITransactionListener,
> +namespace mozilla {
> +
> +class ComposerCommandsUpdater : public nsISelectionListener

class ComposerCommandsUpdater final : ...

::: editor/composer/ComposerCommandsUpdater.h:51
(Diff revision 1)
>    NS_DECL_NSITIMERCALLBACK
>  
>    // nsINamed
>    NS_DECL_NSINAMED
>  
> -  /** nsITransactionListener interfaces
> +  // nsITransactionListener

Use NS_DECL_NSITRANSACTIONLISTENER

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8945816 [details]
Bug 1433345 - part 2: Make HTMLEditor store ComposerCommandsUpdater directly

https://reviewboard.mozilla.org/r/215894/#review221894

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8945817 [details]
Bug 1433345 - part 3: Make HTMLEditor store ResizerSelectionListener directly

https://reviewboard.mozilla.org/r/215896/#review221920

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8945818 [details]
Bug 1433345 - part 4: Make EditorBase derived from nsISelectionListener and notify its owning classes of selection change

https://reviewboard.mozilla.org/r/215898/#review221922

::: dom/events/IMEContentObserver.cpp:720
(Diff revision 1)
> +  if (!mIsObserving) {
> +    return;
> +  }
> +
>    int32_t count = 0;
> -  nsresult rv = aSelection->GetRangeCount(&count);
> +  nsresult rv = aSelection.GetRangeCount(&count);

Use uint32_t count = aSelection.RangeCount() to avoid checking nsresult and non-virtual function.

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8945818 [details]
Bug 1433345 - part 4: Make EditorBase derived from nsISelectionListener and notify its owning classes of selection change

https://reviewboard.mozilla.org/r/215898/#review221922

> Use uint32_t count = aSelection.RangeCount() to avoid checking nsresult and non-virtual function.

s/non-virtual/virtual/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=437ebf52fe2ee796a1664fbb2d693bf304ae9246

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8945815 [details]
Bug 1433345 - part 1: Expose nsComposerCommandsUpdater with renaming it to mozilla::ComposerCOmmandsUpdater

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215892/diff/1-2/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8945816 [details]
Bug 1433345 - part 2: Make HTMLEditor store ComposerCommandsUpdater directly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215894/diff/1-2/

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8945817 [details]
Bug 1433345 - part 3: Make HTMLEditor store ResizerSelectionListener directly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215896/diff/1-2/

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8945818 [details]
Bug 1433345 - part 4: Make EditorBase derived from nsISelectionListener and notify its owning classes of selection change

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215898/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0cd3d4c1a04
part 1: Expose nsComposerCommandsUpdater with renaming it to mozilla::ComposerCOmmandsUpdater r=m_kato
https://hg.mozilla.org/integration/autoland/rev/510153b3ba8d
part 2: Make HTMLEditor store ComposerCommandsUpdater directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f2d590678c07
part 3: Make HTMLEditor store ResizerSelectionListener directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1bb95c1b49c8
part 4: Make EditorBase derived from nsISelectionListener and notify its owning classes of selection change r=m_kato

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d0cd3d4c1a04
https://hg.mozilla.org/mozilla-central/rev/510153b3ba8d
https://hg.mozilla.org/mozilla-central/rev/f2d590678c07
https://hg.mozilla.org/mozilla-central/rev/1bb95c1b49c8