Closed
Bug 692200
Opened 13 years ago
Closed 13 years ago
The 'plugins.force.wmode' pref should override any existing wmode
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: snorp, Assigned: snorp)
References
Details
Attachments
(1 file, 2 obsolete files)
5.02 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
Right now the 'plugins.force.wmode' preference only adds the requested wmode. It does not override any wmode that the page sets. This isn't very useful; it should override any other values for wmode.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → snorp
Assignee | ||
Comment 1•13 years ago
|
||
This also enables the preference on Android. We need this because the fix for bug 692200 breaks 32bit support, which is used in 'transparent' (and probably other) wmodes. We force it to 'opaque' to avoid this.
Assignee | ||
Updated•13 years ago
|
Attachment #564951 -
Flags: review?(blassey.bugs)
Comment 2•13 years ago
|
||
Comment on attachment 564951 [details] [diff] [review] Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode josh would be better to review this
Attachment #564951 -
Flags: review?(blassey.bugs) → review?(joshmoz)
Comment on attachment 564951 [details] [diff] [review] Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode Review of attachment 564951 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see the final patch before I r+ this. ::: dom/plugins/base/nsPluginInstanceOwner.cpp @@ +1200,5 @@ > > // "plugins.force.wmode" preference is forcing wmode type for plugins > // possible values - "opaque", "transparent", "windowed" > nsAdoptingCString wmodeType = Preferences::GetCString("plugins.force.wmode"); > + bool wmodeSet = false; Why declare this further up than necessary? I'd move it down to above the comment "Add attribute name/value pairs." @@ +1264,5 @@ > + mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(NS_LITERAL_STRING("")); > + } else { > + mCachedAttrParamNames [nextAttrParamIndex] = ToNewUTF8String(NS_LITERAL_STRING("wmode")); > + mCachedAttrParamValues[nextAttrParamIndex] = ToNewUTF8String(NS_ConvertUTF8toUTF16(wmodeType)); > + } If you're forcing a wmode (!wmodeType.IsEmpty()) and wmode was already set why would you do anything here? Seems like you only need to handle the (!wmodeSet) case. If you're doing this because you need to have the correct number of attributes due to this code much earlier in the function: // "plugins.force.wmode" preference is forcing wmode type for plugins // possible values - "opaque", "transparent", "windowed" nsAdoptingCString wmodeType = Preferences::GetCString("plugins.force.wmode"); if (!wmodeType.IsEmpty()) { mNumCachedAttrs++; } then you either 1) need to make it clear with a comment why you're adding empty string entries to comply with that or 2) fix this code to not increment mNumCachedAttrs unless you're actually going to add one. I realize the second option is tougher due to the malloc, so I'm fine with a comment.
Attachment #564951 -
Flags: review?(joshmoz) → review-
Assignee | ||
Comment 4•13 years ago
|
||
This also enables the preference on Android. We need this because the fix for bug 692200 breaks 32bit support, which is used in 'transparent' (and probably other) wmodes. We force it to 'opaque' to avoid this.
Assignee | ||
Updated•13 years ago
|
Attachment #564951 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #565312 -
Flags: review?(joshmoz)
Assignee | ||
Comment 5•13 years ago
|
||
This also enables the preference on Android. We need this because the fix for bug 692200 breaks 32bit support, which is used in 'transparent' (and probably other) wmodes. We force it to 'opaque' to avoid this.
Assignee | ||
Updated•13 years ago
|
Attachment #565312 -
Attachment is obsolete: true
Attachment #565312 -
Flags: review?(joshmoz)
Assignee | ||
Comment 6•13 years ago
|
||
Comment on attachment 566803 [details] [diff] [review] Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode This patch avoids assigning bogus values as requested by jst and also fixes an incorrect assertion I noticed.
Attachment #566803 -
Flags: review?(jst)
Comment 7•13 years ago
|
||
Comment on attachment 566803 [details] [diff] [review] Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode Looks good. But the fact that Android plugins expect different things than regular NPAPI plugins is disturbing. Is that something that's different in Flash for Android than Flash for desktop? r=jst
Attachment #566803 -
Flags: review?(jst) → review+
Comment 8•13 years ago
|
||
Landed on inbound but backed out because of test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/f005ff7b0e2a https://hg.mozilla.org/integration/mozilla-inbound/rev/2f53f26d9e1b
Assignee | ||
Comment 9•13 years ago
|
||
The failures appear to be caused by bug 692196, so this should be relanded.
Keywords: checkin-needed
Comment 10•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/96928459cadb
Keywords: checkin-needed
Comment 11•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/96928459cadb
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in
before you can comment on or make changes to this bug.
Description
•