Last Comment Bug 665586 - Kill AddEventListenerByIID/RemoveEventListenerByIID from plugin code
: Kill AddEventListenerByIID/RemoveEventListenerByIID from plugin code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 836786
Blocks: 661297
  Show dependency treegraph
 
Reported: 2011-06-20 09:52 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2013-02-06 16:06 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Remove ByIID code from plugininstanceowner (22.39 KB, patch)
2011-06-20 09:52 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review
Part 2: Remove from ByIID code from nsPluginDOMContextMenuListener (2.47 KB, patch)
2011-06-20 09:53 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review
Part 1: Remove ByIID code from plugininstanceowner (24.28 KB, patch)
2011-06-28 13:07 PDT, Jonas Sicking (:sicking) PTO Until July 5th
jaas: review-
Details | Diff | Review
Part 2: Remove from ByIID code from nsPluginDOMContextMenuListener (2.47 KB, patch)
2011-06-28 13:12 PDT, Jonas Sicking (:sicking) PTO Until July 5th
jaas: review+
Details | Diff | Review
Part 1: Remove ByIID code from plugininstanceowner (24.19 KB, patch)
2011-08-07 14:35 PDT, Jonas Sicking (:sicking) PTO Until July 5th
jaas: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2011-06-20 09:52:03 PDT
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) PTO Until July 5th 2011-06-20 09:53:30 PDT
Created attachment 540502 [details] [diff] [review]
Part 2: Remove from ByIID code from nsPluginDOMContextMenuListener
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-28 13:07:17 PDT
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) PTO Until July 5th 2011-06-28 13:12:24 PDT
Created attachment 542567 [details] [diff] [review]
Part 2: Remove from ByIID code from nsPluginDOMContextMenuListener
Comment 4 Josh Aas 2011-07-08 08:59:03 PDT
Hey Jonas - go ahead and land this whenever you want. Don't wait for me.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-07-08 09:46:14 PDT
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 2011-07-08 10:59:44 PDT
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) PTO Until July 5th 2011-08-07 14:32:18 PDT
> ::: 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) PTO Until July 5th 2011-08-07 14:35:47 PDT
Created attachment 551348 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner
Comment 9 Josh Aas 2011-08-08 11:09:53 PDT
Nice catch on the MouseMove vs. DispatchMouseToPlugin thing. Better not to duplicate that.
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-08 11:28:06 PDT
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 2011-08-09 07:31:24 PDT
Comment on attachment 551348 [details] [diff] [review]
Part 1: Remove ByIID code from plugininstanceowner

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

Ugh.

Note You need to log in before you can comment on or make changes to this bug.