The 'plugins.force.wmode' pref should override any existing wmode

RESOLVED FIXED in Firefox 10

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

Trunk
Firefox 10
ARM
Android

Details

Attachments

(1 attachment, 2 obsolete attachments)

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: nobody → snorp
Created attachment 564951 [details] [diff] [review]
Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode

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.
Attachment #564951 - Flags: review?(blassey.bugs)
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 3

7 years ago
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-
Created attachment 565312 [details] [diff] [review]
Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode

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.
Attachment #564951 - Attachment is obsolete: true
Attachment #565312 - Flags: review?(joshmoz)
Created attachment 566803 [details] [diff] [review]
Bug 692200 - Make 'plugins.force.wmode' pref override any other wmode

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.
Attachment #565312 - Attachment is obsolete: true
Attachment #565312 - Flags: review?(joshmoz)
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 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+
The failures appear to be caused by bug 692196, so this should be relanded.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96928459cadb
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.