Kill AddEventListenerByIID/RemoveEventListenerByIID from editor and embedding

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 538554 [details] [diff] [review]
Remove from editor

First couple of patches for bug 661297
Attachment #538554 - Flags: review?(Olli.Pettay)
Created attachment 538555 [details] [diff] [review]
Remove from embedding
Assignee: nobody → jonas
Attachment #538555 - Flags: review?(Olli.Pettay)
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

6 years ago
This is tricky to review. Need to make sure that the event is always
QIed to right interface before handling it.
How do you mean? All the functions on nsIDOMMouseListener etc take a nsIDOMEvent, as does nsIDOMEventListener::HandleEvent.

Comment 5

6 years ago
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)
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

6 years ago
Trusted events don't guarantee anything about interfaces, but sure,
it would be chrome script which would be firing strange events.
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

6 years ago
(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.
Er, I was wrong.
The IID listeners don't listen for trusted events.

So yes, I'd like to see a new patch.
Created attachment 538943 [details] [diff] [review]
Remove from editor
Attachment #538554 - Attachment is obsolete: true
Attachment #538554 - Flags: review?(Olli.Pettay)
Attachment #538943 - Flags: review?(Olli.Pettay)
Created attachment 538944 [details] [diff] [review]
Remove from embedding
Attachment #538555 - Attachment is obsolete: true
Attachment #538555 - Flags: review?(Olli.Pettay)
Attachment #538944 - Flags: review?(Olli.Pettay)
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 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
Attachment #538943 - Flags: review?(Olli.Pettay) → review+
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)
Attachment #538944 - Flags: review?(Olli.Pettay) → review+
(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.
Landed the editor part:

http://hg.mozilla.org/mozilla-central/rev/347d715650f6
(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
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec503528d2ef

Checked in to mozilla-inbound. Thanks for review!
http://hg.mozilla.org/mozilla-central/rev/ec503528d2ef
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.