Closed Bug 545602 Opened 10 years ago Closed 10 years ago

Unify the event listeners for editor

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: masayuki, Assigned: masayuki)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #426467 - Attachment is obsolete: true
Attachment #426878 - Flags: review?(Olli.Pettay)
Sorry for the slight delay in reviewing. Doing that hopefully today.
Attachment #426878 - Flags: review?(Olli.Pettay) → review-
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.
(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(..)));
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #426878 - Attachment is obsolete: true
Attachment #430306 - Flags: review?(Olli.Pettay)
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
Attachment #430306 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v1.3Splinter Review
thanky you, smaug!
Attachment #430306 - Attachment is obsolete: true
Attachment #430792 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/cf8a74ca361d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
You need to log in before you can comment on or make changes to this bug.