Closed
Bug 665586
Opened 10 years ago
Closed 10 years ago
Kill AddEventListenerByIID/RemoveEventListenerByIID from plugin code
Categories
(Core :: Plug-ins, defect)
Core
Plug-ins
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: sicking, Assigned: sicking)
References
Details
Attachments
(2 files, 3 obsolete files)
2.47 KB,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
24.19 KB,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → jonas
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #540500 -
Attachment is obsolete: true
Attachment #542566 -
Flags: review?
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #542567 -
Flags: review?
Hey Jonas - go ahead and land this whenever you want. Don't wait for me.
Assignee | ||
Comment 5•10 years ago
|
||
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
Attachment #542566 -
Flags: review? → review?(joshmoz)
Assignee | ||
Updated•10 years ago
|
Attachment #542567 -
Flags: review? → review?(joshmoz)
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?
Attachment #542566 -
Flags: review?(joshmoz) → review-
Attachment #542567 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #540502 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
> ::: 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?
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #542566 -
Attachment is obsolete: true
Attachment #551348 -
Flags: review?
Assignee | ||
Updated•10 years ago
|
Attachment #551348 -
Flags: review? → review?(joshmoz)
Attachment #551348 -
Flags: review?(joshmoz) → review+
Nice catch on the MouseMove vs. DispatchMouseToPlugin thing. Better not to duplicate that.
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
Comment on attachment 551348 [details] [diff] [review] Part 1: Remove ByIID code from plugininstanceowner >+ (ev = (EventRecord*)(((nsGUIEvent*)theEvent)->pluginEvent)) && Ugh.
Comment 12•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ec15be691f62 http://hg.mozilla.org/mozilla-central/rev/196eadbe49de
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•