665586 - Kill AddEventListenerByIID/RemoveEventListenerByIID from plugin code

Status: RESOLVED FIXED

Description

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

Created attachment 540500 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

Josh is currently working on a big rewrite of plugin stuff (bug 90268). If that finishes first I'll produce patches on top of that. Otherwise I'll do patches on top of m-c, and then help Josh merge those patches.

So not asking for review on these patches yet. Just attaching for safekeeping

Comment 1

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

Created attachment 540502 [details] [diff] [review]
Part 2: Remove from ByIID code from nsPluginDOMContextMenuListener

Comment 2

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

Created attachment 542566 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

So this is getting close to be the only remaining *EventListenerByIID user, so asking for review.

Josh, find me on irc and we'll figure out a plan for merging with your other work.

Comment 3

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

Created attachment 542567 [details] [diff] [review]
Part 2: Remove from ByIID code from nsPluginDOMContextMenuListener

Comment 4

[Josh Aas]

Hey Jonas - go ahead and land this whenever you want. Don't wait for me.

Comment 5

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

Comment on attachment 542566 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

Oops, thought I had flagged you for reviewer, but apparently not

Comment 6

[Josh Aas]

Comment on attachment 542566 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

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

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1815,3 @@
>        if (nsEventStatus_eConsumeNoDefault == rv) {
>          aMouseEvent->PreventDefault();
> +        // We didn't used to do this when 

This looks like an unfinished comment.

@@ +1834,5 @@
> +    mContentFocused = PR_TRUE;
> +    return DispatchFocusToPlugin(aEvent);
> +  }
> +  if (eventType.EqualsLiteral("blur")) {
> +    mContentFocused = PR_TRUE;

This is incorrect - mContentFocused should be set to PR_FALSE here. Copy and paste error?

@@ +1851,5 @@
> +      return NS_OK;
> +    }
> +    return DispatchMouseToPlugin(aEvent);
> +  }
> +  if (eventType.EqualsLiteral("mousemove") ||

It looks like you didn't retain most of the logic in nsPluginInstanceOwner::MouseMove, which you removed. Did you determine that it is unnecessary for some reason or did you just forget to move it?

Comment 7

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

> ::: dom/plugins/base/nsPluginInstanceOwner.cpp
> @@ +1815,3 @@
> >        if (nsEventStatus_eConsumeNoDefault == rv) {
> >          aMouseEvent->PreventDefault();
> > +        // We didn't used to do this when 
> 
> This looks like an unfinished comment.

Removed the comment, see below.

> @@ +1834,5 @@
> > +    mContentFocused = PR_TRUE;
> > +    return DispatchFocusToPlugin(aEvent);
> > +  }
> > +  if (eventType.EqualsLiteral("blur")) {
> > +    mContentFocused = PR_TRUE;
> 
> This is incorrect - mContentFocused should be set to PR_FALSE here. Copy and
> paste error?

Good catch! Fixed!

> @@ +1851,5 @@
> > +      return NS_OK;
> > +    }
> > +    return DispatchMouseToPlugin(aEvent);
> > +  }
> > +  if (eventType.EqualsLiteral("mousemove") ||
> 
> It looks like you didn't retain most of the logic in
> nsPluginInstanceOwner::MouseMove, which you removed. Did you determine that
> it is unnecessary for some reason or did you just forget to move it?

It looked like nsPluginInstanceOwner::MouseMove did exactly the same thing as nsPluginInstanceOwner::DispatchMouseToPlugin. With the only exception that MouseMove didn't call StopPropagation which I suspect was just an omission (that's what the comment above was going to be about).

Did I miss some logic in nsPluginInstanceOwner::MouseMove that isn't in nsPluginInstanceOwner::DispatchMouseToPlugin?

Comment 8

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

Created attachment 551348 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

Comment 9

[Josh Aas]

Nice catch on the MouseMove vs. DispatchMouseToPlugin thing. Better not to duplicate that.

Comment 10

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

Checked in to inbound. Thanks for the super quick rereview!

http://hg.mozilla.org/integration/mozilla-inbound/rev/ec15be691f62
http://hg.mozilla.org/integration/mozilla-inbound/rev/196eadbe49de

Comment 11

[:Ms2ger (⌚ UTC+1/+2)]

Comment on attachment 551348 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

>+          (ev = (EventRecord*)(((nsGUIEvent*)theEvent)->pluginEvent)) &&

Ugh.

Comment 12

[:Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/21,8/23)]

http://hg.mozilla.org/mozilla-central/rev/ec15be691f62
http://hg.mozilla.org/mozilla-central/rev/196eadbe49de