Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Description • 12 years ago

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);
}

	Benjamin Smedberg
	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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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 }

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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.

	Benjamin Smedberg
	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.

	Georg Fritzsche [:gfritzsche]
	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 :(

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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

	Georg Fritzsche [:gfritzsche]
	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|.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 10 • 12 years ago

Yep, the tests look pretty good: https://tbpl.mozilla.org/?tree=Try&rev=2b578ca55b97

	Benjamin Smedberg
	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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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!

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 14 • 12 years ago

Comment on attachment 734723 [details] [diff] [review]
patch v2

Review of attachment 734723 [details] [diff] [review]:
-----------------------------------------------------------------

Me like.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 15 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9983874bfa63

	Ryan VanderMeulen [:RyanVM]
	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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) 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

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/e30393f6f85b

	Alexandre Alapetite
	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

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews) Assignee
	Comment 20 • 11 years ago

Using the "enabledState" attribute with Ci.nsIPluginTag.STATE_CLICKTOPLAY / STATE_ENABLED / STATE_DISABLED should do what you want.

	Alexandre Alapetite
	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