545602 - Unify the event listeners for editor

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Attachment: Patch v1.0

Currently, we have 6 event listeners for an editor.
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsEditorEventListeners.h

I think this is waste. Their instances need 120 byte. If we unify them to one class, it becomes 44byte. And nsEditor needs to store only one pointer. So, 20byte can be reduced on every nsEditor.

Comment 1

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

Attachment: Patch v1.1

This patch moves editor/libeditor/text/nsEditorEventListeners.* to editor/libeditor/base/nsEditorEventListner.* because the listener doesn't depend on nsPlainTextEditor, so, it can be a part of nsEditor. And if it should not be moved to base, the name should be nsPlainTextEditorEventListener.

And also renames editor/libeditor/html/nsHTMLEditorMouseListener.*  to nsHTMLEditorEventListener.* because the unified new class isn't only for mouse events.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Sorry for the slight delay in reviewing. Doing that hopefully today.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 426878 [details] [diff] [review]
Patch v1.1

+nsEditor::CreateEventListeners()
+{
+  NS_ENSURE_TRUE(!mEventListener, NS_ERROR_ALREADY_INITIALIZED);
+  nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
+  nsRefPtr<nsEditorEventListener> listener =
+    new nsEditorEventListener(this, ps);
+  NS_ENSURE_TRUE(listener, NS_ERROR_OUT_OF_MEMORY);
+  listener->QueryInterface(NS_GET_IID(nsIDOMEventListener),
+                           getter_AddRefs(mEventListener));
+  NS_ASSERTION(mEventListener,
+               "nsEditorEventListener doesn't have nsIDOMEventListener");
+  return NS_OK;
+}

Does nsEditorListener really need the presshell parameter?
Couldn't it always get that from the editor?
(That is actually needed to fix a case where presshell is re-created.
 Don't have the bug# now.)

Also, could you create the listener like this
mEventListener = do_QueryInterface(new nsEditorEventListener(..));



 nsresult
 nsHTMLEditor::CreateEventListeners()
 {
-  nsresult rv = NS_OK;
-
-  if (!mMouseListenerP)
-  {
-    // get a mouse listener
-    rv = NS_NewHTMLEditorMouseListener(getter_AddRefs(mMouseListenerP), this);
-
-    if (NS_FAILED(rv))
-    {
-      return rv;
-    }
-  }
-
-  return nsPlaintextEditor::CreateEventListeners();
+  NS_ENSURE_TRUE(!mEventListener, NS_ERROR_ALREADY_INITIALIZED);
+  nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
+  nsRefPtr<nsEditorEventListener> listener =
+    new nsHTMLEditorEventListener(this, ps);
+  NS_ENSURE_TRUE(listener, NS_ERROR_OUT_OF_MEMORY);
+  listener->QueryInterface(NS_GET_IID(nsIDOMEventListener),
+                           getter_AddRefs(mEventListener));
+  NS_ASSERTION(mEventListener,
+               "nsHTMLEditorEventListener doesn't have nsIDOMEventListener");
+  return NS_OK;
 }
Similar things here.

If you really want, you could change presshell handling in a different bug.

Could still re-review.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> Also, could you create the listener like this
> mEventListener = do_QueryInterface(new nsEditorEventListener(..));
Perhaps this doesn't compile without casting
Maybe
mEventListener = do_QueryInterface(static_cast<nsIDOMKeyEventListener*>(new nsEditorEventListener(..)));

Comment 5

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

Attachment: Patch v1.2

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 430306 [details] [diff] [review]
Patch v1.2

>+nsEditor::CreateEventListeners()
>+{
>+  NS_ENSURE_TRUE(!mEventListener, NS_ERROR_ALREADY_INITIALIZED);
>+  mEventListener = do_QueryInterface(
>+    static_cast<nsIDOMKeyListener*>(new nsEditorEventListener(this)));
>+  NS_ASSERTION(mEventListener,
>+               "nsEditorEventListener doesn't have nsIDOMEventListener");
I think you could replace the assertion with a
NS_ENSURE_TRUE(mEventListener, NS_ERROR_OUT_OF_MEMORY);


>+already_AddRefed<nsIPresShell>
>+nsEditorEventListener::GetPresShell()
>+{
>+  NS_ENSURE_TRUE(mEditor, nsnull);
>+  // mEditor is nsEditor or its inherited class.
>+  // This is guaranteed by constructor.
>+  nsIPresShell* ps;
>+  nsresult rv = static_cast<nsEditor*>(mEditor)->GetPresShell(&ps);
>+  NS_ENSURE_SUCCESS(rv, nsnull);
>+  return ps;
>+}
Could you do something like this:
nsCOMPtr<nsIPresShell> ps;
static_cast<nsEditor*>(mEditor)->GetPresShell(getter_AddRefs(ps));
return ps.forget();


>+NS_INTERFACE_MAP_BEGIN(nsEditorEventListener)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMKeyListener)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMTextListener)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMCompositionListener)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMMouseListener)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMFocusListener)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIDOMEventListener, nsIDOMKeyListener)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMTextListener)
NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMKeyListener)


>+// individual key handlers return NS_OK to indicate NOT consumed
>+// by default, an error is returned indicating event is consumed
>+// joki is fixing this interface.
Just remove this comment. It is old and not valid.


> nsresult
> nsHTMLEditor::CreateEventListeners()
> {
>-  nsresult rv = NS_OK;
>-
>-  if (!mMouseListenerP)
>-  {
>-    // get a mouse listener
>-    rv = NS_NewHTMLEditorMouseListener(getter_AddRefs(mMouseListenerP), this);
>-
>-    if (NS_FAILED(rv))
>-    {
>-      return rv;
>-    }
>-  }
>-
>-  return nsPlaintextEditor::CreateEventListeners();
>+  NS_ENSURE_TRUE(!mEventListener, NS_ERROR_ALREADY_INITIALIZED);
>+  mEventListener = do_QueryInterface(
>+    static_cast<nsIDOMKeyListener*>(new nsHTMLEditorEventListener(this)));
>+  NS_ASSERTION(mEventListener,
>+               "nsHTMLEditorEventListener doesn't have nsIDOMEventListener");
NS_ERROR_OUT_OF_MEMORY check here too

Comment 7

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

Attachment: Patch v1.3

thanky you, smaug!

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/cf8a74ca361d