Closed Bug 558970 Opened 10 years ago Closed 10 years ago

nsEditorEventListener should store its owner as nsEditor rather than nsIEditor

Categories

(Core :: Editor, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch v1.0 (obsolete) — Splinter Review
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().
Attachment #438648 - Flags: review?(Olli.Pettay)
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.
Attachment #438648 - Flags: review?(Olli.Pettay)
Attached patch Patch v2.0 (obsolete) — Splinter Review
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.
Attachment #438648 - Attachment is obsolete: true
Attachment #438649 - Attachment is obsolete: true
Attachment #439468 - Flags: review?(Olli.Pettay)
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.
Attachment #439468 - Flags: review?(Olli.Pettay) → review+
(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.
But only nsHTMLEditor should create nsHTMLEditorEventListener.
So you know that editor is nsHTMLEditor.
Currently, yes. However, isn't it risky in future? If you don't mind that, I'll just remove it.
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.
Ah, sounds good. I'll do it before landing the new patch tomorrow.
Attached patch Patch v2.1Splinter Review
Attachment #439468 - Attachment is obsolete: true
Attachment #439469 - Attachment is obsolete: true
(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.
Well, just file a followup bug "Sort out editor's contextmenu/mouse event handling"
or something like that.
http://hg.mozilla.org/mozilla-central/rev/d7085006cb18

I'll file the bug.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.