Closed
Bug 879392
Opened 12 years ago
Closed 12 years ago
Change - Remove support for sending input to plugins
Categories
(Firefox for Metro Graveyard :: Input, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: TimAbraldes, Assigned: TimAbraldes)
References
Details
(Whiteboard: feature=change u=metro_firefox_user c=Content_features p=1)
Attachments
(1 file, 1 obsolete file)
25.90 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
In bug 812351, we added code that forwards input information to windowless plugins. Windows 8 Style Firefox will not support plugins, so we should remove the input forwarding code.
Updated•12 years ago
|
Blocks: metrov1defect&change
Summary: Remove support for sending input to plugins → Change - Remove support for sending input to plugins
Whiteboard: feature=change c=tbd u=tbd p=0
![]() |
||
Comment 1•12 years ago
|
||
Is there any harm in keeping this just in case?
Assignee | ||
Comment 2•12 years ago
|
||
This is code that we intend never to execute and never to test. I think the harm in keeping this code around is two-fold:
1) It adds maintenance cost; this code makes MetroInput.cpp significantly more cluttered, and anyone who wants to make changes to MetroInput.cpp will have to read through it and figure out what it's doing. If someone makes changes to MetroInput.cpp that break this code, we'll never know because we don't plan to ever test it.
2) This code could have security vulnerabilities (and even if it doesn't have them now, it could have them introduced later). A malicious party could discover a vulnerability and trick users into enabling plugins (which is possible through a pref, and probably not very difficult to convince a user to do since it makes Flash kind of work), then exploit the vulnerability.
We can always add this code back later if we decide to support plugins.
Assignee | ||
Comment 3•12 years ago
|
||
Since I'm messing with widget code for bug 879562 and bug 837293, I figured I'd just go ahead and do this too.
This patch should basically be the reverse of the patch in bug 812351.
Assignee | ||
Comment 4•12 years ago
|
||
This patch also gets rid of the plugin input tests (which were disabled anyway)
Attachment #762924 -
Attachment is obsolete: true
Attachment #762924 -
Flags: review?(netzen)
Attachment #762929 -
Flags: review?(netzen)
Updated•12 years ago
|
QA Contact: jbecerra
Whiteboard: feature=change c=tbd u=tbd p=0 → feature=change c=operations u=operations p=1
Updated•12 years ago
|
Priority: -- → P3
Comment 5•12 years ago
|
||
Comment on attachment 762929 [details] [diff] [review]
Patch v2
Review of attachment 762929 [details] [diff] [review]:
-----------------------------------------------------------------
I'm fine with this but I'd like jimm to give his OK since he expressed some concern in Comment 1.
Attachment #762929 -
Flags: review?(netzen) → review?(jmathies)
![]() |
||
Updated•12 years ago
|
Attachment #762929 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043
The patch incorrectly references bug 879382. Oops :(
Comment 7•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 years ago
|
||
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043
>
> The patch incorrectly references bug 879382. Oops :(
If this happens again please can you backout and reland as soon as possible, as one push, with DONTBUILD in the topmost commit message (to save build resources). The bug marking script used after a merge will handle that fine as long as the original landing and backout are in the same merge.
Updated•12 years ago
|
Target Milestone: --- → Firefox 24
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+1] from comment #8)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043
> >
> > The patch incorrectly references bug 879382. Oops :(
>
> If this happens again please can you backout and reland as soon as possible,
> as one push, with DONTBUILD in the topmost commit message (to save build
> resources). The bug marking script used after a merge will handle that fine
> as long as the original landing and backout are in the same merge.
Absolutely; thanks for the tip!
Comment 10•12 years ago
|
||
No problem :-)
Comment 11•12 years ago
|
||
I tried testing using the information in bug 812351#c4, by going to about:config and changing plugin.disable to false then going to hulu.com and playing some content using touch controls. That worked.
Did this land correctly?
Comment 12•12 years ago
|
||
Hey Tim, please see Juan's question in Comment #11.
Flags: needinfo?(tabraldes)
Assignee | ||
Comment 13•12 years ago
|
||
There's code somewhere that generates plugin messages for mouse input; I think it's probably this [1] but I haven't tested to be sure. When working on bug 812351, I must have missed the fact that mouse input was working with plugins even before applying the patch.
For verifying this bug, it would have been sufficient to ensure that keyboard input was not reaching plugins. However, since bug 843236 landed, we use the KeyboardLayout class [2] to process all our keyboard input, and it sends input to plugins.
Basically there's no way to verify this bug because it's a lie; we always send input to plugins now (and it probably works better now than it did with the implementation that I ripped out as part of this bug).
[1] https://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp?rev=5d38d2a6e4c1#2095
[2] https://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp
Flags: needinfo?(tabraldes)
Updated•12 years ago
|
Whiteboard: feature=change c=operations u=operations p=1 → feature=change u=metro_firefox_user c=Content_features p=1
Updated•11 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•