Closed Bug 1056545 Opened 5 years ago Closed 5 years ago

Cleanup event handlers of nsEditorEventListener and nsHTMLEditorEventListener

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(5 files)

I'm working on bug 826657. In the bug, I want to change nsEditorEventListener and nsHTMLEditorEventListener. However, unrelated changes become big. Therefore, I'd like to clean them up first.
First, let's sort out event handlers of ns*EditorEventListener.

This stops unnecessary virtual calls.
Attachment #8477304 - Flags: review?(ehsan)
Using the message of internal event of DOM event instance is safer and faster than checking event type string.

If a DOM event is created with strange type, e.g., "click" event is created with KeyboardEvent, the message is always 0. Therefore, this patch bans invalid events which make us harder to write editor code.

However, we should keep accepting strange "focus" and "blur" event. They are tested by test_focus.xul. So, dispatching them may be our official hack for setting or stealing focus from an element.

FYI: In HandleEvent(), mEditor is checked if it's nullptr. Therefore, each event handler doesn't need to check it. However, now, each handler needs to check it argument because QI before it is called *might* fail.
Attachment #8477309 - Flags: review?(ehsan)
This patch just make ns*EditorEventListener code conform to our coding rules:

1. Cleaning up class member initializing code in constructor.
2. Add or rewrite { and }.
3. Wrap long lines.
4. Make variable name of nsresult |rv| rather than |res|.
Attachment #8477311 - Flags: review?(ehsan)
Let's use early return style if each if block is big and possible.

I'll attach -w version of this patch.
Attachment #8477312 - Flags: review?(ehsan)
Comment on attachment 8477304 [details] [diff] [review]
part.1 Make event handlers of ns*EditorEventListener protected and virtual as far as possible

Review of attachment 8477304 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditorEventListener.h
@@ -42,5 @@
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIDOMEVENTLISTENER
>  
> -#ifdef HANDLE_NATIVE_TEXT_DIRECTION_SWITCH
> -  NS_IMETHOD KeyDown(nsIDOMEvent* aKeyEvent);

NS_IMETHOD is the *worst* macro in our code.  :(
Attachment #8477304 - Flags: review?(ehsan) → review+
Comment on attachment 8477309 [details] [diff] [review]
part.2 Each event handler of ns*EditorEventListener should take specific event interface rather than nsIDOMEvent if it's necessary

Review of attachment 8477309 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditorEventListener.cpp
@@ +56,5 @@
>  #include "nsIBidiKeyboard.h"            // for nsIBidiKeyboard
>  #endif
> +#ifdef DEBUG
> +#include "nsPrintfCString.h"            // for nsPrintfCString
> +#endif // #ifdef DEBUG

Conditional includes are a footgun, please remove the ifdef.

@@ +314,5 @@
>  nsEditorEventListener::HandleEvent(nsIDOMEvent* aEvent)
>  {
> +  if (NS_WARN_IF(!mEditor)) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }

Please keep using the NS_ENSURE macros in editor code.  I disagree with the decision to use this pattern.  :-)

@@ +328,1 @@
>        return DragEnter(dragEvent);

Perhaps add a comment to this switch statement saying what happens if these QIs fail..

@@ +389,5 @@
> +    // compositionend
> +    case NS_COMPOSITION_END:
> +      HandleEndComposition(aEvent);
> +      return NS_OK;
> +    default: {

Nit: move the contents of this block to after the switch statement, because we are returning from all cases, so this is effectively an else after return.
Attachment #8477309 - Flags: review?(ehsan) → review+
Comment on attachment 8477311 [details] [diff] [review]
part.3 ns*EditorEventListener should conform to current coding rules

Review of attachment 8477311 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditorEventListener.cpp
@@ +34,5 @@
>  nsHTMLEditorEventListener::Connect(nsEditor* aEditor)
>  {
>    nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryObject(aEditor);
> +  nsCOMPtr<nsIHTMLInlineTableEditor> htmlInlineTableEditor =
> +                                       do_QueryObject(aEditor);

Nit: indent with two spaces, please.
Attachment #8477311 - Flags: review?(ehsan) → review+
Attachment #8477312 - Flags: review?(ehsan) → review+
Thank you fro the quick review!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> Comment on attachment 8477311 [details] [diff] [review]
> part.3 ns*EditorEventListener should conform to current coding rules
> 
> Review of attachment 8477311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/nsHTMLEditorEventListener.cpp
> @@ +34,5 @@
> >  nsHTMLEditorEventListener::Connect(nsEditor* aEditor)
> >  {
> >    nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryObject(aEditor);
> > +  nsCOMPtr<nsIHTMLInlineTableEditor> htmlInlineTableEditor =
> > +                                       do_QueryObject(aEditor);
> 
> Nit: indent with two spaces, please.

Do you think it as:

nsCOMPtr<nsIHTMLInlineTableEditor> htmlInlineTableEditor =
  do_QueryObject(aEditor);

?
Flags: needinfo?(ehsan)
Yes.
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.