Closed
Bug 854467
Opened 12 years ago
Closed 12 years ago
can't un-set the clicktoplay flag on an nsIPluginTag
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla23
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Keywords: addon-compat)
Attachments
(1 file, 3 obsolete files)
27.12 KB,
patch
|
benjamin
:
review+
Unfocused
:
review+
|
Details | Diff | Splinter Review |
Basically, this fails (run as an xpcshell-test in dom/plugins/test/unit/): function run_test() { let tag = get_test_plugintag(); tag.clicktoplay = true; do_check_true(tag.clicktoplay); tag.clicktoplay = false; do_check_false(tag.clicktoplay); }
Comment 1•12 years ago
|
||
Which check fails? There are only three states for a plugin, currently: * enabled * clicktoplay * disabled So if a plugin is in the disabled state, [get] .clicktoplay will always be false, and [set] .clicktoplay will have no effect. We did this because we wanted to implement bug 830267 without doing external API changes; we can certainly make the API reflect reality a little better in the future.
Assignee | ||
Comment 2•12 years ago
|
||
The second check fails. Basically, the implementation for [set] .clicktoplay can never unset the click-to-play state. Unfortunately, it's not even clear what that should do. nsPluginTags.cpp: 409 NS_IMETHODIMP 410 nsPluginTag::SetClicktoplay(bool clicktoplay) 411 { 412 if (clicktoplay == IsClicktoplay()) { 413 return NS_OK; 414 } 415 416 const PluginState state = GetPluginState(); 417 if (state != ePluginState_Disabled) { 418 SetPluginState(ePluginState_Clicktoplay); 419 } 420 421 mPluginHost->UpdatePluginInfo(this); 422 return NS_OK; 423 }
Assignee | ||
Comment 3•12 years ago
|
||
I guess the real question is, "If pluginTag.clicktoplay is true, what do we do to put that plugin in a state where disabled is false and clicktoplay is also false?" Setting clicktoplay to false won't do it, because that doesn't do anything. Setting disabled to false won't do it either, because SetDisabled(false) calls SetEnabled(true), which exits early without changing any state if IsEnabled() is true, which is the case if the plugin's state is ePluginState_Clicktoplay.
Assignee | ||
Comment 4•12 years ago
|
||
Comment 5•12 years ago
|
||
So I think the "right" way to fix this is as follows: readonly attribute boolean disabled; readonly attribute boolean clicktoplay; const unsigned long STATE_ENABLED = 0; const unsigned long STATE_DISABLED = 1; const unsigned long STATE_CLICKTOPLAY = 2; attribute unsigned long enabledState; But if that's going to be too complicated for your dependent patches, we should take a workaround and fix the API in a subsequent step.
Comment 6•12 years ago
|
||
I can probably look into the proper API changes next week. I don't see a problem with a temporary patch if bug 549697 needs to land before that is done - currently the changes in 549697 are the only ones that actually need this. I actually intended to cover that API usage, but apparently didn't quite see it through :(
Assignee | ||
Comment 7•12 years ago
|
||
I was concerned about how many tests would break if we made these changes, so I actually slapped together something yesterday and ran it on try. There were a few bugs, so I fixed it up and now I'm running it again: https://tbpl.mozilla.org/?tree=Try&rev=125376b6d29b
Attachment #730845 -
Flags: feedback?(georg.fritzsche)
Comment 8•12 years ago
|
||
Comment on attachment 730845 [details] [diff] [review] patch for new api Review of attachment 730845 [details] [diff] [review]: ----------------------------------------------------------------- Apart from the missing notification trigger this looks good to me. Thanks for checking into this. ::: dom/plugins/base/nsPluginTags.cpp @@ +385,5 @@ > +NS_IMETHODIMP > +nsPluginTag::SetEnabledState(uint32_t aEnabledState) { > + if (aEnabledState >= ePluginState_MaxValue) > + return NS_ERROR_ILLEGAL_VALUE; > + Preferences::SetInt(GetStatePrefNameForPlugin(this).get(), aEnabledState); This is now missing the |host->UpdatePluginInfo(this)| for |oldState != newState|.
Attachment #730845 -
Flags: feedback?(georg.fritzsche) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
Thanks, Georg. This is actually passing (almost?) all of the tests now, so I think we can go ahead with making these changes.
Assignee: nobody → dkeeler
Attachment #729122 -
Attachment is obsolete: true
Attachment #730845 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #731953 -
Flags: review?(benjamin)
Assignee | ||
Comment 10•12 years ago
|
||
Yep, the tests look pretty good: https://tbpl.mozilla.org/?tree=Try&rev=2b578ca55b97
Comment 11•12 years ago
|
||
Comment on attachment 731953 [details] [diff] [review] patch Please add static-asserts that each of nsIPluginTag.STATE_* == nsPluginTag.ePluginState_* And also add a comment to nsIPluginTag.idl that the numeric values are stored in prefs, so if new states are added, they should not renumber the existing states.
Attachment #731953 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Thanks for the review - I would appreciate one more quick look just to make sure I put the static asserts in the appropriate place.
Attachment #731953 -
Attachment is obsolete: true
Attachment #734723 -
Flags: review?(benjamin)
Updated•12 years ago
|
Attachment #734723 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Comment on attachment 734723 [details] [diff] [review] patch v2 It would probably also be a good idea for Blair to have a look at these changes. Thanks!
Attachment #734723 -
Flags: review?(bmcbride)
Comment 14•12 years ago
|
||
Comment on attachment 734723 [details] [diff] [review] patch v2 Review of attachment 734723 [details] [diff] [review]: ----------------------------------------------------------------- Me like.
Attachment #734723 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9983874bfa63
Comment 16•12 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/974726290de1 https://tbpl.mozilla.org/php/getParsedLog.php?id=21601515&tree=Mozilla-Inbound cc1plus: warnings being treated as errors ../../../../dom/plugins/base/nsPluginTags.cpp: In member function 'void nsPluginTag::SetPluginState(nsPluginTag::PluginState)': ../../../../dom/plugins/base/nsPluginTags.cpp:411:70: error: comparison between 'enum nsPluginTag::PluginState' and 'enum nsIPluginTag::<anonymous>' ../../../../dom/plugins/base/nsPluginTags.cpp:412:73: error: comparison between 'enum nsPluginTag::PluginState' and 'enum nsIPluginTag::<anonymous>' ../../../../dom/plugins/base/nsPluginTags.cpp:413:69: error: comparison between 'enum nsPluginTag::PluginState' and 'enum nsIPluginTag::<anonymous>' make[6]: *** [nsPluginTags.o] Error 1
Assignee | ||
Comment 17•12 years ago
|
||
Ok - fixed that and enabled warnings-as-errors on my local build (so hopefully this sort of thing won't happen again). https://hg.mozilla.org/integration/mozilla-inbound/rev/e30393f6f85b
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e30393f6f85b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
Keywords: addon-compat
Comment 19•11 years ago
|
||
Hello, Sorry to revive this bug, but it seems that changeset 128189 by David Keeler to address this bug 854467 has modified the nsIPluginTag Interface to limit the "enabled" attribute to be readonly in dom/plugins/base/nsIPluginTag.idl: https://hg.mozilla.org/mozilla-central/rev/9983874bfa63#l3.27 As a consequence, some plug-in management extension are broken in Firefox 23 beta (work in Firefox 22) such as Plugins Toggler 1.2.2 https://addons.mozilla.org/firefox/addon/plugins-toggler/ Is the addition of "readonly" to "nsIPluginTag.enabled" really on purpose and needed? And if yes, what is the alternative approach for e.g. the above-cited extension to work? Best regards, Alexandre http://alexandre.alapetite.fr
Assignee | ||
Comment 20•11 years ago
|
||
Using the "enabledState" attribute with Ci.nsIPluginTag.STATE_CLICKTOPLAY / STATE_ENABLED / STATE_DISABLED should do what you want.
Comment 21•11 years ago
|
||
Thanks David for this fast answer. Problem solved from my side, but I hope not too many extensions are broken. If anyone lands here for the same issue, the relevant information is in: https://hg.mozilla.org/mozilla-central/file/9983874bfa63/dom/plugins/base/nsIPluginTag.idl
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
•