558970 - nsEditorEventListener should store its owner as nsEditor rather than nsIEditor

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Attachment: Patch v1.0

separated from bug 552914.

Current nsEditorEventListener stores the editor as nsIEditor. However, if we change this to nsEditor, we can remove may casts and QIs.

And also we can remove null check of mEditor because it's set at constructor. And it's never modified, so, it always valid pointer.

There are some random thought.

1. nsEditorEventListener::KeyPress() still QIs nsIPlaintextEditor.

nsEditorEventListener is for nsEditor, so, we shouldn't store the editor as nsPlaintextEditor I think. 

I think that we should move HandleKeyPress() from nsIPlaintextEditor to nsIEditor. Besides, nsIPlaintextEditor should inherit nsIEditor. Then, we don't change the interfaces actually. (And also we can move the flags of nsIPlaintextEditor to nsIEditor too)

If we do so, we can move the contents of nsEditorEventListener::KeyPress() to nsEditor::HandleKeyPress() and nsPlaintextEditor::HandleKeyPress().

# But I'm not sure whether the method should be defined in idl because there are not merits of it being scriptable.

2. nsEditorEventListener::MouseClick() still QIs nsIEditorMailSupport.

nsIEditorMailSupport is implemented nsPlaintextEditor, so, the contents of nsEditorEventListener can be moved to nsEditor::HandleMouseClick() and nsPlaintextEditor::HandleMouseClick() like HandleKeyPress().

Comment 1

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

Attachment: Patch v1.0 (-w)

Comment 3

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

Comment on attachment 438648 [details] [diff] [review]
Patch v1.0

I'm hoping to get a bit different patch.
Ping me on IRC if you have any questions.

Comment 4

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

Attachment: Patch v2.0

nsEditorEventListener can connect/disconect to editor now.

All public methods should check whether mEditor is null or not, but protected/private methods should check as assertion.

The listener installation code and uninstallation code are moved to nsEditorEventListener, by this change, we can uninstall the listener certainly when it's discorrected.

Comment 5

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

Attachment: Patch v2.0 (-w)

Comment 6

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

Comment on attachment 439468 [details] [diff] [review]
Patch v2.0


>+nsresult
>+nsEditorEventListener::InstallToEditor()
>+{
>+  NS_PRECONDITION(mEditor, "The caller must set mEditor");
>+
>+  nsCOMPtr<nsPIDOMEventTarget> piTarget = mEditor->GetPIDOMEventTarget();
>+  NS_ENSURE_TRUE(piTarget, NS_ERROR_FAILURE);
>+
>+  nsresult rv;
>+
>+  // register the event listeners with the listener manager
>+  nsCOMPtr<nsIDOMEventGroup> sysGroup;
>+  piTarget->GetSystemEventGroup(getter_AddRefs(sysGroup));
>+  nsIEventListenerManager* elmP = piTarget->GetListenerManager(PR_TRUE);
>+
>+  // XXX Why the else case of this condition isn't error?
>+  if (sysGroup && elmP) {
Just return early if !sysGroup  || !elmP.
NS_ENSURE_STATE(sysGroup && elmP);

>+
>+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(piTarget));
>+  // XXX Why the else case of this condition isn't error?
Same here. Just return some error code if !target

> NS_IMETHODIMP
> nsEditorEventListener::KeyDown(nsIDOMEvent* aKeyEvent)
> {
>+  NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
>   return NS_OK;
> }
No need for NS_ENSURE_TRUE check here.


> 
> NS_IMETHODIMP
> nsEditorEventListener::KeyUp(nsIDOMEvent* aKeyEvent)
> {
>+  NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
>   return NS_OK;
> }
No need for NS_ENSURE_TRUE check here.


> NS_IMETHODIMP
> nsEditorEventListener::MouseUp(nsIDOMEvent* aMouseEvent)
> {
>+  NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
>   return NS_OK;
> }
No need for NS_ENSURE_TRUE check here.

> 
> NS_IMETHODIMP
> nsEditorEventListener::MouseDblClick(nsIDOMEvent* aMouseEvent)
> {
>+  NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
>   return NS_OK;
> }
No need for NS_ENSURE_TRUE check here.

> 
> NS_IMETHODIMP
> nsEditorEventListener::MouseOver(nsIDOMEvent* aMouseEvent)
> {
>+  NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
>   return NS_OK;
> }
No need for NS_ENSURE_TRUE check here.

> 
> NS_IMETHODIMP
> nsEditorEventListener::MouseOut(nsIDOMEvent* aMouseEvent)
> {
>+  NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
>   return NS_OK;
> }
No need for NS_ENSURE_TRUE check here.


>+  // Detect only "context menu" click
>+  //XXX This should be easier to do!
>+  // But eDOMEvents_contextmenu and NS_CONTEXTMENU is not exposed in any event interface :-(
Could you file a followup for this.
I think HTMLEditor should be listening for contextmenu event.

> 
>+  virtual nsresult Connect(nsEditor* aEditor)
>+  {
>+    NS_WARNING("Use ConnectToHTMLEditor");
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  virtual nsresult ConnectToHTMLEditor(nsHTMLEditor* aEditor)
>+  {
>+    return nsEditorEventListener::Connect(aEditor);
>+  }
I don't understand this.
why you need ConnectToHTMLEditor? 

r+ if you fix all the comments and either explain or fix ConnectToHTMLEditor handling.

Comment 7

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

(In reply to comment #6)
> >+  virtual nsresult Connect(nsEditor* aEditor)
> >+  {
> >+    NS_WARNING("Use ConnectToHTMLEditor");
> >+    return NS_ERROR_NOT_AVAILABLE;
> >+  }
> >+
> >+  virtual nsresult ConnectToHTMLEditor(nsHTMLEditor* aEditor)
> >+  {
> >+    return nsEditorEventListener::Connect(aEditor);
> >+  }
> I don't understand this.
> why you need ConnectToHTMLEditor? 

It guarantees that the mEditor is nsHTMLEditor or its sub class. If I remove this, we need to QI in nsHTMLEditorEventListener instead of cast by GetHTMLEditor.

Comment 8

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

But only nsHTMLEditor should create nsHTMLEditorEventListener.
So you know that editor is nsHTMLEditor.

Comment 9

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

Currently, yes. However, isn't it risky in future? If you don't mind that, I'll just remove it.

Comment 10

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

Perhaps you could just add an ifdef DEBUG block to somewhere to check that
nsHTMLEditorEventListener is connected to HTMLEditor?
That should be enough to capture possible problems in the future.

Comment 11

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

Ah, sounds good. I'll do it before landing the new patch tomorrow.

Comment 12

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

Attachment: Patch v2.1

Comment 13

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

(In reply to comment #6)
> >+  // Detect only "context menu" click
> >+  //XXX This should be easier to do!
> >+  // But eDOMEvents_contextmenu and NS_CONTEXTMENU is not exposed in any event interface :-(
> Could you file a followup for this.
> I think HTMLEditor should be listening for contextmenu event.

I don't understand this well. Looks like nsHTMLEditorEventListener can listen "contextmenu" event by nsIDOMContextMenuListener. The argument of nsIDOMContextMenuListener::ContextMenu() is the cause of opening the context menu, right?

I'm not sure the event comes before or after the event. If it comes before the event, it should prevent the default of the mouse event. But if it comes after the event, looks like mouse down handler still needs to check the event is opening context menu or not for Mac. So, I don't understand if latter is correct.

Comment 14

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

Well, just file a followup bug "Sort out editor's contextmenu/mouse event handling"
or something like that.

Comment 15

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

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

I'll file the bug.

Comment 16

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

I filed bug 560201.