Closed Bug 472821 Opened 15 years ago Closed 15 years ago

Force wmode preference required to make all plugins visible in fennec

Categories

(Core Graveyard :: Plug-ins, defect)

Other
Linux
defect
Not set
normal

Tracking

(status1.9.2 beta1-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

(Keywords: mobile)

Attachments

(1 file, 3 obsolete files)

      No description provided.
Attachment #356173 - Flags: review?(pavlov)
Component: General → Plug-ins
Product: Fennec → Core
QA Contact: general → plugins
Attached patch Better solution (obsolete) — Splinter Review
I think this version should be better.
Attachment #356173 - Attachment is obsolete: true
Attachment #356316 - Flags: review?(kinetik)
Attachment #356173 - Flags: review?(pavlov)
I don't think I'm the right person to review this.  I suggest asking one of the layout/plugin peers (roc, bz, maybe jst?) for review.

Having said that, a couple of comments:

1) If the element already has wmode set, it'll use the specified value rather than the value being forced via plugins.force.wmode, is that intentional?  I'm guessing you want to use this pref to force Flash into windowless mode (wmode isn't a general plugin attribute, it's specific to Flash), but that won't happen if the element has wmode="window" set.

2) If the element already has wmode set, a second wmode is added to the end (and due to #1, it has the value of the first wmode, not the value from plugins.force.wmode).

3) nsPluginInstanceOwner::GetAttribute (and probably other code) searches mCachedAttrParamNames for the first occurrence of an attribute, so you can't override an already present attribute by adding it later in mCachedAttrParamNames with the new value, you'll need to clobber the value of the existing attr.

4) wmode can be set via a child param element, so you will need to deal with forcing/overriding the wmode value there too.
Attachment #356316 - Flags: review?(kinetik)
> than the value being forced via plugins.force.wmode, is that intentional?  I'm
...
> happen if the element has wmode="window" set.
Hmm, originally yes, but unfortunately I forgot about wmode=window value, which we have to override too.

> 
> 2) If the element already has wmode set, a second wmode is added to the end
> (and due to #1, it has the value of the first wmode, not the value from
> plugins.force.wmode).
Ok, then I have to check for already existing wmode param, and deal with static array size... 

> 
> 4) wmode can be set via a child param element, so you will need to deal with

Hmm, "child param element"  what do you mean? can you provide some example?

> forcing/overriding the wmode value there too.
> Hmm, "child param element"  what do you mean? can you provide some example?

<object ...>
  <param name="wmode" value="window">
Attachment #356316 - Attachment is obsolete: true
Keywords: mobile
Attachment #362712 - Flags: review?(jst)
Comment on attachment 362712 [details] [diff] [review]
Fixed NP_FULL mode, and moved force value on top of arguments list

Looks ok to me, but Josh should look at this one too.
Attachment #362712 - Flags: superreview+
Attachment #362712 - Flags: review?(jst)
Attachment #362712 - Flags: review?(joshmoz)
Comment on attachment 362712 [details] [diff] [review]
Fixed NP_FULL mode, and moved force value on top of arguments list

>-  if (pDoc) {
>+  nsAdoptingCString wmodeType = nsContentUtils::GetCharPref("plugins.force.wmode");
>+  if (pDoc && wmodeType.IsEmpty()) {
>     *aMode = nsPluginMode_Full;
>   } else {
>     *aMode = nsPluginMode_Embedded;

I'm not really sure what this is about. You're getting a pref which presumably forces the plugin to be windowed or windowless, then forcing full page or embedded. The two don't seem related. Are you sure you even need this part of the patch?

Also, it should be documented what the possible values for the pref you're adding are. Just make a comment near wherever you end up using the pref and you might as well write it out in a comment in this bug too.
Also, can you describe the problem you're trying to solve in this bug? There is basically no summary saying why you want to do what you're doing, no background information.
Assignee: nobody → romaxa
I did this check because some plugins even not checking any arguments (wmode e.t.c) if NPMode = NP_FULL... 

> >+  nsAdoptingCString wmodeType = nsContentUtils::GetCharPref("plugins.force.wmode");
> >+  if (pDoc && wmodeType.IsEmpty()) {

Probably I need to disable NP_FULL mode only if wmode != windowed

> Also, it should be documented what the possible values for the pref you're
> adding are. Just make a comment near wherever you end up using the pref and 
Originally I was meaning here only values "opaque" and "transparent"... "windowed" mode was not in scope of this problem...

(In reply to comment #8)
> Also, can you describe the problem you're trying to solve in this bug?
Problem is that we are rendering whole layout with RenderDocument API... and there are only way to see plugins when they are in windowless mode.

Also see bug 442109, 442109#c10
So we need a new patch with a couple of changes...

- include documentation on possible values for the new pref
- change the first part of the patch to "wmode != windowed" and add comments explaining exactly why you're messing with full vs. embedded there, or remove the first part of the patch altogether

Thanks for the explanation.
Attachment #362712 - Flags: review?(joshmoz) → review-
oleg maybe you probably want FLASH to be force windowless , once it can actually work that way ... but not other plugins, which require to be windowed by its nature to work, like quicktime and friends.

so maybe doing something like (pseudo-code):

nsAutoString typeData;
if (force_flash_windowless)
  if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, typeData))
    if (typeData == NS_LITERAL_STRING("application/x-shockwave-flash"))
          //XXX: force it windowless
          ...
What effect does this have on plugins that don't support wmode?  I guess they'll just ignore it?
(In reply to comment #11)
> oleg maybe you probably want FLASH to be force windowless , once it can
> actually work that way ... but not other plugins, which require to be windowed
> by its nature to work, like quicktime and friends.

Plugins are putting final word about supporting windowless mode, and if engine trying to force windowless mode but plugins are not supporting it, they will ignore it, say about it Get/SetValue... and will work in windowed mode.
I think we have to fix flash about NP_FULL mode...
Attachment #362712 - Attachment is obsolete: true
Attachment #369633 - Flags: review?
Attachment #369633 - Flags: review? → review?(joshmoz)
(In reply to comment #14)

> I think we have to fix flash about NP_FULL mode...

What does this mean?
Flash does not work in windowless + NP_FULL combination
Attachment #369633 - Flags: superreview?(jst)
Attachment #369633 - Flags: review?(joshmoz)
Attachment #369633 - Flags: review+
Attachment #369633 - Flags: superreview?(jst) → superreview+
Blocks: 524413
Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/de69f2b10b67
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #369633 - Flags: approval1.9.2?
Comment on attachment 369633 [details] [diff] [review]
Added some comments, and removed hack for flash plugin

a192=beltzner
Attachment #369633 - Flags: approval1.9.2? → approval1.9.2+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: