663461 - Kill AddEventListenerByIID/RemoveEventListenerByIID from editor and embedding

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Remove from editor

First couple of patches for bug 661297

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Remove from embedding

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The surprising part to me was that this actually *reduces* the amount of code that we have. I would have thought that having to register for each individual event would add to it, but it turns out not be less than the amount of code removed.

Comment 3

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

This is tricky to review. Need to make sure that the event is always
QIed to right interface before handling it.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How do you mean? All the functions on nsIDOMMouseListener etc take a nsIDOMEvent, as does nsIDOMEventListener::HandleEvent.

Comment 5

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

It just happens to be a side-effect of nsIDOM***Listener interfaces that the
type of the event is checked before the listener is called.
(That check has been there forever)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, so we need to make sure that the event is QI*able* to the correct interface?

Doesn't the fact that we only listen to trusted events make sure that that is the case?

Comment 7

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

Trusted events don't guarantee anything about interfaces, but sure,
it would be chrome script which would be firing strange events.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, one thing I should fix in both these patches is to explicitly specify the aWantsUntrusted parameter to PR_FALSE.

Probably by adding another inline AddEventListener to nsIDOMEventTarget which takes both aCapture and aWantsUntrusted and forwards to the existing AddEventListener with aOptionalCount set to 2.

Let me know if you want new patches for that.

Comment 9

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

(In reply to comment #8)
> Oh, one thing I should fix in both these patches is to explicitly specify
> the aWantsUntrusted parameter to PR_FALSE.
I don't understand this?

Currently some of the listeners handle also untrusted events.
That is a bug, but should be fixed in a different bug.

Comment 10

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

Er, I was wrong.

Comment 11

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

The IID listeners don't listen for trusted events.

So yes, I'd like to see a new patch.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Remove from editor

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Remove from embedding

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I just verified that all HandleEvent functions can handle events of the wrong type.

The only one that was a bit doubtful (the event got spread further than I was comfortable with) was ChromeContextMenuListener::HandleEvent, so I added

  nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aMouseEvent);
  NS_ENSURE_TRUE(mouseEvent, NS_ERROR_UNEXPECTED);

to the top of that function locally. Let me know if you want a new patch.

Comment 15

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

Comment on attachment 538943 [details] [diff] [review]
Remove from editor

> nsEditor::CreateEventListeners()
> {
>   // Don't create the handler twice
>-  if (mEventListener)
>-    return NS_OK;
>-  mEventListener = do_QueryInterface(
>-    static_cast<nsIDOMKeyListener*>(new nsEditorEventListener()));
>-  NS_ENSURE_TRUE(mEventListener, NS_ERROR_OUT_OF_MEMORY);
>+  if (!mEventListener)
>+    mEventListener = new nsEditorEventListener();
if (expr) {
  stmt;
}

> nsHTMLEditor::CreateEventListeners()
> {
>   // Don't create the handler twice
>-  if (mEventListener)
>-    return NS_OK;
>-  mEventListener = do_QueryInterface(
>-    static_cast<nsIDOMKeyListener*>(new nsHTMLEditorEventListener()));
>-  NS_ENSURE_TRUE(mEventListener, NS_ERROR_OUT_OF_MEMORY);
>+  if (!mEventListener)
>+    mEventListener = new nsHTMLEditorEventListener();
Ditto

Comment 16

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

Comment on attachment 538944 [details] [diff] [review]
Remove from embedding


>+NS_IMETHODIMP
>+ChromeTooltipListener::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
You should check that eEvent is key or mouse event
(depending on the event type)

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #16)
> >+NS_IMETHODIMP
> >+ChromeTooltipListener::HandleEvent(nsIDOMEvent* aEvent)
> >+{
> >+  nsAutoString eventType;
> >+  aEvent->GetType(eventType);
> You should check that eEvent is key or mouse event
> (depending on the event type)

The code that doesn't check the type doesn't use the event object at all. Doesn't seem worth it to check that it's of the right type then.

The MouseMove function does use the event object and does check its type.

Let me know if you still want something changed.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed the editor part:

http://hg.mozilla.org/mozilla-central/rev/347d715650f6

Comment 19

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

(In reply to comment #17)
> The code that doesn't check the type doesn't use the event object at all.
> Doesn't seem worth it to check that it's of the right type then.
Technically you're changing the behavior then, since non-key events
can trigger the code.

Though, in practice it shouldn't matter. r=me

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ec503528d2ef

Checked in to mozilla-inbound. Thanks for review!

Comment 21

[Joe Drew (not getting mail)]

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