Closed Bug 1034679 Opened 5 years ago Closed 5 years ago

Visually display that plug-in states are locked in Add-ons Manager

Categories

(Toolkit :: Add-ons Manager, defect, minor)

30 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jason, Assigned: alexbardas, Mentored)

Details

(Whiteboard: [lang=js][lang=c++])

Attachments

(4 files, 10 obsolete files)

24.44 KB, image/png
Details
13.95 KB, patch
alexbardas
: review+
Details | Diff | Splinter Review
3.31 KB, patch
alexbardas
: review+
Details | Diff | Splinter Review
19.71 KB, image/png
Details
Attached image Image of plug-in
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

Example:

1. Close Firefox.
2. Lock the "plugin.state.java" preference in a configuration file that the user does not have file permissions to change.
3. Open Firefox.
4. Open the Add-ons Manager.
5. Click the "Plugins" tab on the left.
6. Look at the Java plug-in state.


Actual results:

The state of the Java plug-in is truly locked and cannot be permanently changed.  However, the user interface gives the impression that it *can* be changed.  The drop-down menu for the state (i.e., the one that contains "Ask to Activate," "Always Activate," and "Never Activate") is not grayed-out.  The state can be changed for a second before it automatically reverts to the locked state.  It almost looks like a glitch -- something unintentional.

This appears to happen in all versions of Firefox.  The latest version that I tested this on was Version 30.


Expected results:

Currently, locked preferences in the "Options" panel are grayed-out and make it very clear to the user that they cannot be changed.  Similarly, I would like to see the same for the drop-down menu for locked plug-ins.  For users, it will let them know that the plug-in states are purposely locked.  For administrators, it gives them an immediate visual notification that their locked-preferences file is working correctly.
Severity: normal → minor
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
I think that especially with click-to-play plugins, we should do this, so I'm setting backlog+.
Flags: firefox-backlog+
OS: Windows 8.1 → All
Hardware: x86_64 → All
I think we should take a patch, but in the list of priorities I don't see that this is something I'd prioritize in the next 6-12 months. I'd argue this should be backlog-.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: firefox-backlog+ → firefox-backlog?
(In reply to Benjamin Smedberg  [:bsmedberg] Away 19-July through 3-Aug from comment #2)
> I think we should take a patch, but in the list of priorities I don't see
> that this is something I'd prioritize in the next 6-12 months. I'd argue
> this should be backlog-.

We agreed on this in the backlog triage meeting, would still good to have so i'll mentor it.
Mentor: georg.fritzsche
Flags: firefox-backlog? → firefox-backlog-
Whiteboard: [good next bug][lang=js]
Assignee: nobody → abardas
Status: NEW → ASSIGNED
I have some thoughts about this, but gfritzsche can you please share some implementation details to make sure I'll be doing this the right way?
Sorry, i only got check on the steps required here now - as it turns out this involve some more steps than i initially thought, please bug me more about it as needed Alex.

We need to:
(1) enable checking whether the backing preference is locked from the PluginProvider (provides the plugins parts of the addon manager)
(2) signal from PluginProvider that the plugin state can't be changed
(3) disable/lock the UI that allows changing the state

For (1) we can add an attribute isEnabledStateLocked to nsIPluginTag.idl. The implementation is in nsPluginTags.h/.cpp - using |GetStatePrefNameForPlugin()| we can check for nsIPrefBranch.getIsLocked().

For (2) we should change the permissions to not allow changes when aTags[0].isEnabledStateLocked, i.e. leave return empty permission flags:
http://hg.mozilla.org/mozilla-central/annotate/104254bd1fc8/toolkit/mozapps/extensions/internal/PluginProvider.jsm#l462

For (3) we'll have to adjust the addon manager frontend code to not hide the menulist (that allows you to pick the plugin state):
http://hg.mozilla.org/mozilla-central/annotate/104254bd1fc8/toolkit/mozapps/extensions/content/extensions.js#l3104
I think we can just fill it always if TYPE_SUPPORTS_ASK_TO_ACTIVATE and set menulist.disabled if the permissions don't allow changing it.


I'll expand on testing after the weekend.
Whiteboard: [good next bug][lang=js] → [lang=js][lang=c++]
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> For (3) we'll have to adjust the addon manager frontend code to not hide the
> menulist (that allows you to pick the plugin state):
> http://hg.mozilla.org/mozilla-central/annotate/104254bd1fc8/toolkit/mozapps/
> extensions/content/extensions.js#l3104
> I think we can just fill it always if TYPE_SUPPORTS_ASK_TO_ACTIVATE and set
> menulist.disabled if the permissions don't allow changing it.

The second part for this is here:
http://hg.mozilla.org/mozilla-central/annotate/104254bd1fc8/toolkit/mozapps/extensions/content/extensions.xml#l1309
(The addon manager has separate code paths for the list view and the detail view)
Blair, do you agree on parts (2) and (3) of comment 5 & and comment 6?
Flags: needinfo?(bmcbride)
Yep.
Flags: needinfo?(bmcbride)
The implementation is mostly finished, I only have to add mochitests, so someone can take a look at it.

I've also left intentionally line 352 commented in nsPluginTags.cpp until I finish adding mochitests. Anyone who wants to test this bug can uncomment that line.
Attachment #8466560 - Flags: feedback?(georg.fritzsche)
Comment on attachment 8466560 [details] [diff] [review]
bug1034679_visually_disable_locked_plugin_states_from_add-on_manager.diff

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

That looks pretty good already!
Most of the comments below are minor, the important points are:
* we have to fix how we check when to show & disable the state menu
* we should check if we can do the visibility handling better

Also, don't forget to check both the addon list view and detail view (e.g. double click on addon item) - you will need to change extensions.js as well, sadly we duplicate a lot of code right now.

::: dom/plugins/base/nsPluginTags.cpp
@@ +36,5 @@
>  inline char* new_str(const char* str)
>  {
>    if (str == nullptr)
>      return nullptr;
> +

No need to mix the whitespace changes in here if you don't touch these parts.

::: dom/plugins/base/nsPluginTags.h
@@ +57,5 @@
>    // plugin is enabled and not blocklisted
>    bool IsActive();
>  
> +  // plugin is enabled but locked
> +  bool IsEnabledStateLocked();

I don't think we'll need this from C++ soon, adding only GetIsEnabledStateLocked() is enough - we could still easily add this later if actually needed.

::: toolkit/mozapps/extensions/content/extensions.css
@@ +225,5 @@
>  .discover-button[disabled="true"] {
>    display: none;
>  }
>  
> +.discover-button.show[disabled="true"] {

I think the discover button is something different and this change not needed here.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1309,5 @@
>            if (addonType.flags & AddonManager.TYPE_SUPPORTS_ASK_TO_ACTIVATE &&
>                (this.hasPermission("ask_to_activate") ||
>                 this.hasPermission("enable") ||
> +               this.hasPermission("disable") ||
> +               this.mAddon.permissions === 0)) {

There might be additional permissions set (e.g. PERM_CAN_UNINSTALL), so we should not check for 0.
Instead i'd only check for TYPE_SUPPORTS_ASK_TO_ACTIVATE here ... (continued)

@@ +1324,5 @@
>                this._stateMenulist.selectedItem = this._alwaysActivateMenuitem;
>              }
>              this._stateMenulist.selectedItem.disabled = false;
>              this._stateMenulist.disabled = false;
> +            if (this.mAddon.permissions === 0) {

... and check that it doesn't have any of the "ask_to_activate", "enable", "disable" permissions here.

@@ +1326,5 @@
>              this._stateMenulist.selectedItem.disabled = false;
>              this._stateMenulist.disabled = false;
> +            if (this.mAddon.permissions === 0) {
> +              this._stateMenulist.disabled = true;
> +              this._stateMenulist.classList.add("show");

Does the element get hidden when you set .disabled=true? Can't we change that behavior instead and set .hidden=true where we want to hide it?

::: toolkit/mozapps/extensions/internal/PluginProvider.jsm
@@ +293,5 @@
>    this.__defineGetter__("homepageURL", function() homepageURL);
>  
>    this.__defineGetter__("isActive", function() !aTags[0].blocklisted && !aTags[0].disabled);
>    this.__defineGetter__("appDisabled", function() aTags[0].blocklisted);
> +  this.__defineGetter__("isEnabledStateLocked", function() aTags[0].isEnabledStateLocked);

We're not using the getter outside of the PluginWrapper, so let's not expose it for now.

@@ +461,5 @@
>    });
>  
>    this.__defineGetter__("permissions", function() {
>      let permissions = 0;
> +    if (this.isEnabledStateLocked) {

Just use aTags[0].isEnabledStateLocked for now?

::: toolkit/themes/osx/mozapps/extensions/extensions.css
@@ +123,5 @@
>    border-radius: 8px;
>    background-image: linear-gradient(rgba(255, 255, 255, 0.7), rgba(236, 241, 247, 0.7));
>    background-clip: padding-box;
>    box-shadow: 0 -3px 0 rgba(58, 78, 103, 0.05) inset,
> +              0 3px 0 rgba(175, 195, 220, 0.3);

Unnecessary white-space changes in this file.

@@ +1104,5 @@
>  
>  /*** buttons ***/
>  
>  .addon-control[disabled="true"] {
>    display: none;

Do we break anything if we remove this rule instead of adding a new one?
Does all of |mach mochitest-browser toolkit/mozapps/extensions/test/browser/| still pass?

::: toolkit/themes/windows/mozapps/extensions/extensions.css
@@ +48,5 @@
>  }
>  
>  #detail-view .global-warning {
>    padding: 4px 12px;
> +  border-bottom: 1px solid #CAD4E0;

Unnecessary white-space changes in this file.
Attachment #8466560 - Flags: feedback?(georg.fritzsche) → feedback+
For testing, you can extend browser_CTP_plugins.js [0].
Add a new test part there that locks the pref for the test plugin, causes reloading, then check the state-menulist to be set up as expected.
Don't forget to add unlocking for the pref in a cleanup function in case there are failures in the test.

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js
Unfortunately, I can't remove the .addon-control[disabled="true"] { display: none; } rule because I break the part where state-menu is hidden when the type is not TYPE_SUPPORTS_ASK_TO_ACTIVATE. I could try to refactor that, if you think it's ok.

Regarding the this.mAddon.permissions === 0 check I do, checking only for TYPE_SUPPORTS_ASK_TO_ACTIVATE in the first if and then for each permission (whether to disable or not the state menu) breaks 4 tests in browser_CTP_plugins.js.
(In reply to Alex Bardas :alexbardas from comment #12)
> Unfortunately, I can't remove the .addon-control[disabled="true"] { display:
> none; } rule because I break the part where state-menu is hidden when the
> type is not TYPE_SUPPORTS_ASK_TO_ACTIVATE. I could try to refactor that, if
> you think it's ok.

Sure, let's do that. We should be able to just use .hidden=true instead - no need to hide the disabled state menu anymore.

> Regarding the this.mAddon.permissions === 0 check I do, checking only for
> TYPE_SUPPORTS_ASK_TO_ACTIVATE in the first if and then for each permission
> (whether to disable or not the state menu) breaks 4 tests in
> browser_CTP_plugins.js.

What does it break? Maybe we need to fix the test or i am missing something about that change.
I've made the suggested changes and use .hidden instead of .show class. I've written the tests in a new file for now, for clarity.

Right now, 4 tests fail in browser_CTP_plugins because of the new changes in extensions[.js,.xml] (I implemented them how I understood. In the previous way, the current logic was not changed).

It this how it was supposed to be implemented?
Attachment #8466560 - Attachment is obsolete: true
Attachment #8467341 - Flags: review?(georg.fritzsche)
Removed a line used for debugging.
Attachment #8467341 - Attachment is obsolete: true
Attachment #8467341 - Flags: review?(georg.fritzsche)
Attachment #8467362 - Flags: review?(georg.fritzsche)
Added complete test suite for state menu from both plugin list and plugin details view.
Attachment #8467362 - Attachment is obsolete: true
Attachment #8467362 - Flags: review?(georg.fritzsche)
Attachment #8467448 - Flags: review?(georg.fritzsche)
(In reply to Alex Bardas :alexbardas from comment #14)
> Right now, 4 tests fail in browser_CTP_plugins because of the new changes in
> extensions[.js,.xml] (I implemented them how I understood. In the previous
> way, the current logic was not changed).

Can you expand on this? What exactly is failing?
Flags: needinfo?(abardas)
Comment on attachment 8467448 [details] [diff] [review]
bug1034679_visually_disable_locked_plugin_states_from_add-on_manager.diff

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

This looks pretty close! There are some things below that we should clear up, also we should use the chance to improve the testing while we can.

As you are adding a new mochitest-browser, we should use promises and tasks for async operations - luckily the addon manager testing utils already support this rather well.
There is a more recent example of async browser tests here: http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_experiments.js

In short, you don't need to use callbacks anymore, you use |add_task(function *() {...})| instead of |add_test()|.
Then you can e.g. just do |let res = yield asyncFunction();| or |yield asyncFunction();| instead of juggling with callback chains.

Also, we should use the test assertion functions from Assert.jsm for newer tests: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Assert.jsm

::: dom/plugins/base/nsPluginTags.cpp
@@ +341,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsPluginTag::GetIsEnabledStateLocked(bool* aIsEnabledStateLocked)
> +{

This function currently doesn't return anything via aIsEnabledStateLocked if there are failures.

@@ +343,5 @@
> +NS_IMETHODIMP
> +nsPluginTag::GetIsEnabledStateLocked(bool* aIsEnabledStateLocked)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);

prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
if (prefs) {
  ...

::: toolkit/mozapps/extensions/content/extensions.js
@@ +3120,4 @@
>        let askItem = document.getElementById("detail-ask-to-activate-menuitem");
>        let alwaysItem = document.getElementById("detail-always-activate-menuitem");
>        let neverItem = document.getElementById("detail-never-activate-menuitem");
> +      menulist.hidden = false;

You're setting menulist.hidden here, but using classlist in extensions.xml?

@@ +3120,5 @@
>        let askItem = document.getElementById("detail-ask-to-activate-menuitem");
>        let alwaysItem = document.getElementById("detail-always-activate-menuitem");
>        let neverItem = document.getElementById("detail-never-activate-menuitem");
> +      menulist.hidden = false;
> +      if (hasAnyPermission(this._addon, ["ask_to_activate", "enable", "disable"])) {

|let hasActivatePermission = ["ask_to_activate", ...].some(e => hasPermission(this._addon, e));| ?
Same in extensions.xml.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1339,2 @@
>            } else {
> +            this._stateMenulist.classList.add('hidden');

You're setting classlist here, but using menulist.hidden in extensions.js?

::: toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js
@@ +5,5 @@
> +// Tests that state menu is displayed correctly (enabled or disabled) in the add-on manager
> +let gManagerWindow;
> +let gPluginEl;
> +
> +const PLUGIN_PREF = "plugin.state.java";

Can we please use the test plugin instead? The fake java plugin we have in the test environment is used for special cases involving Java.
The state pref sadly depends on the platform  - "plugin.state.test" on OS X, "plugin.state.nptest" on Linux & Windows.

@@ +12,5 @@
> +  waitForExplicitFinish();
> +  run_next_test();
> +}
> +
> +function end_test() {

Please hook up cleanup code via |registerCleanupFunction()|.

@@ +35,5 @@
> +      break;
> +    }
> +  }
> +  ok(javaPluginId, "Java Plug-in should exist");
> +  gPluginEl = get_addon_element(gManagerWindow, javaPluginId);

There is no need to use globals here, you can pass the element back.

@@ +51,5 @@
> +    return doc.getAnonymousElementByAttribute(gPluginEl, "anonid", "state-menulist");
> +  }
> +}
> +
> +function check_pref_is_unlocked() {

This and check_pref_is_locked() are nearly the same - please introduce an argument here instead.

@@ +57,5 @@
> +  let menuList = getMenuEl();
> +
> +  is_element_visible(menuList, "State menu should be visible.");
> +  is(menuList.disabled, false, "State menu should not be disabled.");
> +  next();

Having calls like |next()| inside a "check" function is unexpected. I'd rather see this outside at the call site.

@@ +60,5 @@
> +  is(menuList.disabled, false, "State menu should not be disabled.");
> +  next();
> +}
> +
> +function check_detail_pref_is_unlocked() {

The same here, use an argument instead.

@@ +107,5 @@
> +}
> +
> +// Tests that plugin state menu is enabled
> +add_test(function() {
> +  open_manager("addons://list/plugin", function(aWindow) {

|gManagerWindow = yield open_manager();|

You don't need to open & close the manager, you could e.g. just switch between plugins and extensions.
You should use the testing utilities here:
|yield gCategoryUtilities.openType("plugin");|

::: toolkit/themes/linux/mozapps/extensions/extensions.css
@@ +879,5 @@
>  
>  
>  /*** buttons ***/
>  
> +.addon-control.hidden {

You set .hidden=false in the JS, and the docs for the hidden attribute say this is equivalent to display:none from CSS:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/hidden

Are the CSS rules here needed at all? Can't we just remove them?
Attachment #8467448 - Flags: review?(georg.fritzsche) → feedback+
I've refactored the tests so they follow the new async structure. 

There are multiple buttons on the plugin/plugin-detail pages that can be disabled. Buttons with class `addon-control` are a bit special, because when they are disabled, they are also hidden. I can't hide those buttons instead of disabling them (http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1349) because they are mixed up with other commands. 

Having that css rule also keeps everything backwards compatible (lots of buttons would be shown in the detail page otherwise).

With the current patch, when running all the tests from toolkits/mozapps/extensions/test/browser directory I get:

Passed:  12656
Failed:  0
Todo:    0
Attachment #8467448 - Attachment is obsolete: true
Attachment #8468148 - Flags: review?(georg.fritzsche)
Flags: needinfo?(abardas)
Comment on attachment 8468148 [details] [diff] [review]
bug1034679_visually_disable_locked_plugin_states_from_add-on_manager.diff

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

Nothing major, but i think we need one more round addressing things here before we can hand it over to peers for review.

::: dom/plugins/base/nsPluginTags.cpp
@@ +344,5 @@
> +nsPluginTag::GetIsEnabledStateLocked(bool* aIsEnabledStateLocked)
> +{
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefs) {
> +    if (NS_FAILED(prefs->PrefIsLocked(GetStatePrefNameForPlugin(this).get(), 

Nit: whitespace.
If you look at the splinter review you can see it highlighted in red or set your editor up to highlight it.

@@ +346,5 @@
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefs) {
> +    if (NS_FAILED(prefs->PrefIsLocked(GetStatePrefNameForPlugin(this).get(), 
> +                                      &*aIsEnabledStateLocked))) {
> +      return NS_ERROR_FAILURE;

This way you only return a failure if getting |prefs| succeeded, but PrefIsLocked() failed.
Thinking about this today after not being so tired anymore, we really should just default to returning |false| if something failed and not return an error.

@@ +349,5 @@
> +                                      &*aIsEnabledStateLocked))) {
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +  

whitespace.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +3110,5 @@
>        let askItem = document.getElementById("detail-ask-to-activate-menuitem");
>        let alwaysItem = document.getElementById("detail-always-activate-menuitem");
>        let neverItem = document.getElementById("detail-never-activate-menuitem");
> +      menulist.hidden = false;
> +      let hasActivatePermission = 

Nit: whitespace.

@@ +3114,5 @@
> +      let hasActivatePermission = 
> +        ["ask_to_activate", "enable", "disable"].some(perm => hasPermission(this._addon, perm));
> +      if (hasActivatePermission) {
> +        if (this._addon.userDisabled === true) {
> +          menulist.selectedItem = neverItem;

Why is setting the selectedItem here getting set inside the hasActivatePermissionCheck?
What happened to setting menulist.disabled=false here?

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1318,5 @@
>                this._stateMenulist.selectedItem = this._askToActivateMenuitem;
>              } else {
>                this._stateMenulist.selectedItem = this._alwaysActivateMenuitem;
>              }
> +            let hasActivatePermission = 

Nit: whitespace.

@@ +1322,5 @@
> +            let hasActivatePermission = 
> +              ["ask_to_activate", "enable", "disable"].some(perm => this.hasPermission(perm));
> +            if (hasActivatePermission) {
> +              this._stateMenulist.selectedItem.disabled = false;
> +              this._stateMenulist.disabled = false;

Just |this._stateMenulist.disabled = !hasActivatePermission;| instead of the repetition?

@@ +1325,5 @@
> +              this._stateMenulist.selectedItem.disabled = false;
> +              this._stateMenulist.disabled = false;
> +            } else {
> +              this._stateMenulist.disabled = true;
> +            }

Missing _stateMenulist.hidden=false?

::: toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js
@@ +10,5 @@
> +  ("@mozilla.org/gio-service;1" in Cc);
> +
> +let gManagerWindow;
> +let gCategoryUtilities;
> +let gPluginEl;

No need to shorten the name here.

@@ +12,5 @@
> +let gManagerWindow;
> +let gCategoryUtilities;
> +let gPluginEl;
> +
> +function get_test_plugin_pref() {

Please stay with camelCase() naming per this:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

@@ +13,5 @@
> +let gCategoryUtilities;
> +let gPluginEl;
> +
> +function get_test_plugin_pref() {
> +  if (gIsWindows || gIsLinux)

These conditions don't change during the test - just save this in a global?

@@ +34,5 @@
> +function get_test_plugin(aPlugins) {
> +  let testPluginId;
> +
> +  for (let plugin of aPlugins) {
> +    dump('\n\n' + plugin.name + '\n\n');

Left-over dump.

@@ +42,5 @@
> +    }
> +  }
> +
> +  Assert.ok(testPluginId, "Test Plug-in should exist");
> +  

Nit: whitespace. There's some more trailing whitespace below in this file.

@@ +50,5 @@
> +  return pluginEl;
> +}
> +
> +// Return the state menu element for plugin list or plugin details
> +function getMenuEl(aDetail=false) {

The function name could be more descriptive - but given this is only used in two places, lets just move contents of this function there.

@@ +59,5 @@
> +    return doc.getAnonymousElementByAttribute(gPluginEl, "anonid", "state-menulist");
> +  }
> +}
> +
> +function check_pref(locked=false) {

The function name doesn't quite match what this does - maybe checkStateMenu()?
There is no need for a default value for the argument.

@@ +69,5 @@
> +  Assert.equal(menuList.disabled, locked, 
> +    "State menu should" + (locked === true ? "" : " not") + " be disabled.");
> +}
> +
> +function check_detail_pref(locked=false) {

The function name doesn't quite match what this does - maybe checkStateMenuDetail()?
There is no need for a default value for the argument.

@@ +92,5 @@
> +
> +function* initializeState() {
> +  Services.prefs.setIntPref(get_test_plugin_pref(), Ci.nsIPluginTag.STATE_ENABLED);
> +  Services.prefs.unlockPref(get_test_plugin_pref());
> +  gManagerWindow = yield open_manager("addons://list/plugin");

No need for passing the URL here, you can switch using the CategoryUtilities.

@@ +103,5 @@
> +function* testCleanup() {
> +  yield close_manager(gManagerWindow);
> +}
> +
> +add_task(initializeState);

Just move the function in here.

@@ +112,5 @@
> +});
> +
> +// Tests that plugin state menu from details is enabled
> +add_task(function* task_chec_detail_pref_in_unlocked() {
> +  return check_detail_pref(false);

Nit: I think yield instead of return gives better failure stacks.
Given this is just one line, let's just move both unlocked checks together in one task. Same for the locked variations below.

@@ +135,5 @@
> +add_task(function* task_check_detail_pref_is_locked() {
> +  return check_detail_pref(true);
> +});
> +
> +add_task(testCleanup);
\ No newline at end of file

Just move the function in here.
No newline at end of file.
Attachment #8468148 - Flags: review?(georg.fritzsche)
I've modified nsPluginTags.cpp to assign false to aIsEnabledStateLocked when there is a failure + other style changes.
Attachment #8468148 - Attachment is obsolete: true
Attachment #8468555 - Flags: review?(georg.fritzsche)
I've also ran the tests on try and browser_plugin_enabled_state_locked is failing on linux. Are you sure "plugin.state.nptest" is for both linux & windows?
Use "plugin.state.libnptest" for linux.
Attachment #8468555 - Attachment is obsolete: true
Attachment #8468555 - Flags: review?(georg.fritzsche)
Attachment #8468589 - Flags: review?(georg.fritzsche)
Comment on attachment 8468589 [details] [diff] [review]
bug1034679_visually_disable_locked_plugin_states_from_add-on_manager.diff

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

Thanks, this looks much better!
When you addressed the below comments i'd recommend splitting the patch up into two parts (e.g. using hgs "qref -X" or "qref -I"):
* nsIPluginTag.idl, nsPluginTags.cpp - request review from bsmedberg
* the rest - request review from Unfocused

Feel free to flag me for feedback again if you're unsure about anything.

::: dom/plugins/base/nsIPluginTag.idl
@@ +25,5 @@
>     */
>    readonly attribute boolean blocklisted;
>  
> +  /**
> +   * true only if the plugin is enabled but locked

This doesn't match the actual behavior - true if the state is non-default and locked, false otherwise.

::: dom/plugins/base/nsPluginTags.cpp
@@ +346,5 @@
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (!prefs || (prefs && NS_FAILED(prefs->PrefIsLocked(GetStatePrefNameForPlugin(this).get(),
> +                                                        &*aIsEnabledStateLocked)))) {
> +      *aIsEnabledStateLocked = false;
> +    }

Nit: Incorrect indentation.
This could be much cleaner if you e.g. always set it to aIsEnabledStateLocked to false at the start of the function.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +3114,5 @@
> +      let hasActivatePermission =
> +        ["ask_to_activate", "enable", "disable"].some(perm => hasPermission(this._addon, perm));
> +      if (hasActivatePermission) {
> +        if (this._addon.userDisabled === true) {
> +          menulist.selectedItem = neverItem;

Why are you only setting the .selectedItem if hasActivatePermission==true?
You are always setting it in extensions.xml.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1321,5 @@
>              }
> +            let hasActivatePermission =
> +              ["ask_to_activate", "enable", "disable"].some(perm => this.hasPermission(perm));
> +            if (hasActivatePermission) {
> +              this._stateMenulist.selectedItem.disabled = false;

Is this line really needed? You're not doing this in extensions.js.

::: toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js
@@ +14,5 @@
> +let gCategoryUtilities;
> +let gPluginElement;
> +
> +function getTestPluginPref() {
> +  let preffix = "plugin.state.";

Nit: prefix vs. preffix?

@@ +54,5 @@
> +}
> +
> +function checkStateMenu(locked) {
> +  Assert.equal(Services.prefs.prefIsLocked(getTestPluginPref()), locked,
> +    "Preference should be " + (locked === true ? "" : "un") + "locked.");

Enough to say e.g. "Preference lock state should be correct.", the bool values can be seen in the failure message.
Same for the other messages below.
Attachment #8468589 - Flags: review?(georg.fritzsche) → feedback+
Thank you very much for the reviews, Georg! :)
Add a separate c++ patch for this bug.
Attachment #8468589 - Attachment is obsolete: true
Attachment #8469534 - Flags: review?(benjamin)
Created a js/css patch for the code involved in this bug. It was already also reviewed by Georg and the current tests pass.
Attachment #8469538 - Flags: review?(bmcbride)
Comment on attachment 8469534 [details] [diff] [review]
bug1034679_add_is_enabled_state_locked_property_for_plugins.diff

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

::: dom/plugins/base/nsPluginTags.cpp
@@ +344,5 @@
> +nsPluginTag::GetIsEnabledStateLocked(bool* aIsEnabledStateLocked)
> +{
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (!prefs || (prefs && NS_FAILED(prefs->PrefIsLocked(GetStatePrefNameForPlugin(this).get(),
> +                                                        &*aIsEnabledStateLocked)))) {

I think you missed addressing part of the last feedback here? :)
Comment on attachment 8469534 [details] [diff] [review]
bug1034679_add_is_enabled_state_locked_property_for_plugins.diff

This code is correct but hard to read, and in some cases we should just be throwing:

  nsCOMPtr<...> prefs...; //ok
  if (NS_WARN_IF(!prefs)) {
    return NS_ERROR_FAILURE; // prefer early returns in error handling
  }

  // It is expected for PrefIsLocked to fail if the pref isn't set or locked. In
  // that case it's not locked.
  *aIsEnabledStateLocked = false;
  unused << prefs->PrefIsLocked(...);
  return NS_OK;
}

Please do something like this for readability.
Attachment #8469534 - Flags: review?(benjamin) → review-
Added the suggested improvements for the c++ part.
Attachment #8469534 - Attachment is obsolete: true
Attachment #8470172 - Flags: review?(benjamin)
Comment on attachment 8469538 [details] [diff] [review]
bug1034679_visually_disable_locked_plugin_states_from_add-on_manager.diff

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

Looks good :)

r+ with a couple of small fixups.

::: toolkit/mozapps/extensions/test/browser/browser_plugin_enabled_state_locked.js
@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci} = Components;
> +const gIsWindows = ("@mozilla.org/windows-registry-key;1" in Cc);
> +const gIsOSX = ("nsILocalFileMac" in Ci);
> +const gIsLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc) ||
> +  ("@mozilla.org/gio-service;1" in Cc);

You only need this indirect method of finding the OS in xpcshell tests. Here you can just use Services.appinfo.OS (Windows = "WINNT", OSX = "Darwin", Linux = anything else).

::: toolkit/themes/linux/mozapps/extensions/extensions.css
@@ +879,5 @@
>  
>  
>  /*** buttons ***/
>  
> +.addon-control[disabled="true"]:not(#detail-state-menulist):not([anonid="state-menulist"]) {

It's generally nice to avoid specific exceptions like this if there's an easy alternative. So I'd prefer you used a class for these elements, something like .no-auto-hide
Attachment #8469538 - Flags: review?(bmcbride) → review+
Attachment #8470172 - Flags: review?(benjamin) → review+
It seems that a test failed, but it has nothing to do with this patch. Should I run the try tests again?
That test failure is unrelated, so you can go ahead.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/79cbd825caac
https://hg.mozilla.org/integration/fx-team/rev/adaf86c7288e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][lang=c++] → [lang=js][lang=c++][fixed-in-fx-team]
Comment on attachment 8470172 [details] [diff] [review]
bug1034679_add_is_enabled_state_locked_property_for_plugins.diff

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

nsIPluginTag.idl needs a UUID bump - e.g. |mach update-uuids nsIPluginTag|.
I've canceled those and re-pushed with a more comprehensive suite: 

https://tbpl.mozilla.org/?tree=Try&rev=225199957b6c
FWIW, if the only change is the UUID change, another Try run isn't going to tell you anything helpful. Try builds are always clobbers and won't show the kind of bustage this was backed out for.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/683b45b75b40
https://hg.mozilla.org/mozilla-central/rev/f9a519ff098f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][lang=c++][fixed-in-fx-team] → [lang=js][lang=c++]
Target Milestone: --- → mozilla34
I just tested this in Nightly 2014-08-23.

It seems to work by graying out the control for the plug-in, but it is not showing the actual state, anymore.  The drop-down list is completely empty, and the drop-down arrow itself seems to be going off to the side of the control, just kind of floating.  Honestly, it looks broken.  

I have attached an image of it.  The top plug-in, "Shockwave Flash," is the broken one; it is locked to "Always Activate."  The one below it, "Microsoft Office 2013," is not locked and is, therefore, not broken.

I would imagine this isn't intentional, is it?
(In reply to Jason from comment #47)
> I would imagine this isn't intentional, is it?

Thanks Jason, that is certainly not intentional! We usually file new bugs for what landed patches broke, so i moved this to bug 1058039.
You need to log in before you can comment on or make changes to this bug.