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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: 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.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 3

[Josh Aas]

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.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: 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.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: 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.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 7

[Johnny Stenback (: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

Comment 8

[Matt Brubeck (:mbrubeck)]

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

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

The failures appear to be caused by bug 692196, so this should be relanded.

Comment 10

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96928459cadb

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/96928459cadb