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)
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)
2.15 KB,
patch
|
jaas
:
review+
jst
:
superreview+
beltzner
:
approval1.9.2+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #356173 -
Flags: review?(pavlov)
Updated•15 years ago
|
Component: General → Plug-ins
Product: Fennec → Core
QA Contact: general → plugins
Assignee | ||
Comment 1•15 years ago
|
||
I think this version should be better.
Attachment #356173 -
Attachment is obsolete: true
Attachment #356316 -
Flags: review?(kinetik)
Attachment #356173 -
Flags: review?(pavlov)
Comment 2•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #356316 -
Flags: review?(kinetik)
Assignee | ||
Comment 3•15 years ago
|
||
> 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.
Comment 4•15 years ago
|
||
> Hmm, "child param element" what do you mean? can you provide some example?
<object ...>
<param name="wmode" value="window">
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #356316 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #362712 -
Flags: review?(jst)
Comment 6•15 years ago
|
||
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 | ||
Comment 9•15 years ago
|
||
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
Comment 10•15 years ago
|
||
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-
Comment 11•15 years ago
|
||
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 ...
Comment 12•15 years ago
|
||
What effect does this have on plugins that don't support wmode? I guess they'll just ignore it?
Assignee | ||
Comment 13•15 years ago
|
||
(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.
Assignee | ||
Comment 14•15 years ago
|
||
I think we have to fix flash about NP_FULL mode...
Attachment #362712 -
Attachment is obsolete: true
Attachment #369633 -
Flags: review?
Updated•15 years ago
|
Attachment #369633 -
Flags: review? → review?(joshmoz)
Comment 15•15 years ago
|
||
(In reply to comment #14) > I think we have to fix flash about NP_FULL mode... What does this mean?
Assignee | ||
Comment 16•15 years ago
|
||
Flash does not work in windowless + NP_FULL combination
Attachment #369633 -
Flags: superreview?(jst)
Attachment #369633 -
Flags: review?(joshmoz)
Attachment #369633 -
Flags: review+
Updated•15 years ago
|
Attachment #369633 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 17•15 years ago
|
||
Pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/de69f2b10b67
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Attachment #369633 -
Flags: approval1.9.2?
Comment 18•15 years ago
|
||
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+
Comment 19•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b0641752027
status1.9.2:
--- → beta1-fixed
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•