Last Comment Bug 831533 - about:plugins should show the enabled/disabled/blocklisted state of plugins
: about:plugins should show the enabled/disabled/blocklisted state of plugins
Status: RESOLVED FIXED
[needs Mozmill test update][mentor=cp...
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All Android
: -- normal with 2 votes (vote)
: mozilla21
Assigned To: Sunny [:darkowlzz]
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 831188 878154
Blocks: 834632
  Show dependency treegraph
 
Reported: 2013-01-16 15:39 PST by Chris Peterson [:cpeterson]
Modified: 2014-09-02 04:53 PDT (History)
8 users (show)
hskupin: in‑qa‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
New field 'State' added which shows enabled/disabled/blocklisted state of plugins. This file depends on the changes in plugins.properties. (3.95 KB, patch)
2013-01-19 10:53 PST, Sunny [:darkowlzz]
no flags Details | Diff | Splinter Review
Added 'state_label' property to plugins. (673 bytes, patch)
2013-01-19 11:00 PST, Sunny [:darkowlzz]
no flags Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.61 KB, patch)
2013-01-19 11:11 PST, Sunny [:darkowlzz]
gfritzsche: feedback+
Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.60 KB, patch)
2013-01-19 11:38 PST, Sunny [:darkowlzz]
gfritzsche: feedback+
Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.80 KB, patch)
2013-01-20 11:30 PST, Sunny [:darkowlzz]
cpeterson: feedback+
Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.50 KB, patch)
2013-01-23 11:01 PST, Sunny [:darkowlzz]
cpeterson: feedback+
Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.50 KB, patch)
2013-01-24 07:50 PST, Sunny [:darkowlzz]
benjamin: review-
Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.35 KB, patch)
2013-01-24 10:16 PST, Sunny [:darkowlzz]
benjamin: review+
Details | Diff | Splinter Review
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins (4.41 KB, patch)
2013-01-25 06:24 PST, Sunny [:darkowlzz]
no flags Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2013-01-16 15:39:08 PST
about:plugins does not list "inactive" (i.e. blocklisted or user-disabled) plugins. Since about:plugins is used to diagnose problems, it would be more useful if all installed plugins were listed, including their enabled/disabled/blocklisted status.
Comment 1 Henrik Skupin (:whimboo) 2013-01-16 16:03:21 PST
Once we have a patch and we can play with it we should also update our Mozmill test.
Comment 2 Justin Dolske [:Dolske] 2013-01-17 10:20:09 PST
I'd rather WONTFIX this and remove about:plugins entirely. The addons manager should be used for this (adding info there, if needed).
Comment 3 Georg Fritzsche [:gfritzsche] 2013-01-17 10:36:17 PST
It's rather useful for triaging/testing/developing as it just plainly dumps the plugin information from the plugin host while the addon manager may do additional work (e.g. caching, beautifying names, ...).
Comment 4 Bill Gianopoulos [:WG9s] 2013-01-17 13:12:38 PST
(In reply to Justin Dolske [:Dolske] from comment #2)
> I'd rather WONTFIX this and remove about:plugins entirely. The addons
> manager should be used for this (adding info there, if needed).

I am posting a strong objection to such change.
Comment 5 Chris Peterson [:cpeterson] 2013-01-18 11:58:22 PST
Assigning bug to indiasuny000@gmail.com
Comment 6 Chris Peterson [:cpeterson] 2013-01-18 12:30:12 PST
Sunny, to implement this feature, you will need to:

1. Remove the isActive check from plugins.html so the for() loop will list all plugins, not just the enabled ones:

https://hg.mozilla.org/mozilla-central/file/b52c02f77cf5/toolkit/content/plugins.html#l117

2. You need to add new status field to the HTML for each plugin. The code to add the field will look a lot like the code for the "Version: " field. I suggest sticking the new field after Version and before Plugin Description:

https://hg.mozilla.org/mozilla-central/file/b52c02f77cf5/toolkit/content/plugins.html#l136

I don't know whether this field should be called "Status:", "State: ", or something else. The name does not matter for now; bsmedberg can suggest an appropriate name after we get the feature working. :)

3. plugin.isActive is defined in PluginProvider.jsm:

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#208

You can see that isActive = (blocklisted || disabled), so it is not useful for determining *why* a plugin is inactive. But you can use plugin.userDisabled and plugin.blocklistState to get more details:

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#210
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/PluginProvider.jsm#222

4. The valid blocklistStates are defiend in nsIBlocklistService.idl:

https://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIBlocklistService.idl#15

I think we might want to list both the enabled/disabled state and the blocklist state. So the status code might look something like:

  status = plugin.isActive ? "Enabled" : "Disabled";
  if (plugin.blocklistState !== Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
    var Ci = Components.interfaces;
    status += " (";
    switch (plugin.blocklistState) {
      case Ci.nsIBlocklistService.STATE_SOFTBLOCKED:
        status += "Soft-blocked";
        break;
      case Ci.nsIBlocklistService.STATE_BLOCKED:
        status += "Blocked";
        break;
      case Ci.nsIBlocklistService.STATE_OUTDATED:
        status += "Outdated";
        break;
      case Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE:
        status += "Vulnerable; Update available";
        break;
      case Ci.nsIBlocklistService.STATE_VULNERABLE_NO_UPDATE:
        status += "Vulnerable; No update available";
        break;
      case Ci.nsIBlocklistService.STATE_NOT_BLOCKED:
      default: // shouldn't happen
        status += plugin.blocklistState;
        break;
      }
      status += ")";
    }

Unfortunately, since we are adding new strings to this web page, they might need to be localized for different languages. But we can implement that after we get the featuring working in one language. :)
Comment 7 Benjamin Smedberg [:bsmedberg] 2013-01-18 12:37:37 PST
"State: ". And I wouldn't localize it, I would just include the exact strings "STATE_SOFTBLOCKED" etc.
Comment 8 Chris Peterson [:cpeterson] 2013-01-18 12:47:01 PST
hskupin: will the MozMill tests need to be updated if about:plugins includes both enabled and disabled plugins and a new "State:" field?
Comment 9 Henrik Skupin (:whimboo) 2013-01-18 14:22:04 PST
(In reply to Chris Peterson (:cpeterson) from comment #8)
> hskupin: will the MozMill tests need to be updated if about:plugins includes
> both enabled and disabled plugins and a new "State:" field?

Yes, we will have to update the test. That's why I put it into the whiteboard field. Once a patch is ready and we have a tryserver build we can easily prepare the test and check it in for the next nightly after this patch's landing.
Comment 10 Sunny [:darkowlzz] 2013-01-19 10:53:25 PST
Created attachment 704248 [details] [diff] [review]
New field 'State' added which shows enabled/disabled/blocklisted state of plugins. This file depends on the changes in plugins.properties.
Comment 11 Sunny [:darkowlzz] 2013-01-19 11:00:02 PST
Created attachment 704250 [details] [diff] [review]
Added 'state_label' property to plugins.
Comment 12 Sunny [:darkowlzz] 2013-01-19 11:11:49 PST
Created attachment 704252 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins
Comment 13 Georg Fritzsche [:gfritzsche] 2013-01-19 11:26:02 PST
Comment on attachment 704252 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Looks good to me apart from the whitespace.

::: toolkit/content/plugins.html
@@ +176,5 @@
> +          status += ")";
> +        }
> +        stateDd.appendChild(document.createTextNode(status));
> +        dl.appendChild(stateDd);
> +        

You should remove the trailing whitespace here.
Comment 14 Sunny [:darkowlzz] 2013-01-19 11:38:48 PST
Created attachment 704257 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins
Comment 15 Georg Fritzsche [:gfritzsche] 2013-01-19 11:42:38 PST
Comment on attachment 704257 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Looks good to me, thanks.
Comment 16 Sunny [:darkowlzz] 2013-01-20 11:30:05 PST
Created attachment 704339 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins
Comment 17 Chris Peterson [:cpeterson] 2013-01-22 12:43:50 PST
Comment on attachment 704339 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Looking good! Here are a couple suggestions:

::: toolkit/content/plugins.html
@@ +13,5 @@
>  
>    var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
>    var pluginsbundle = strBundleService.createBundle("chrome://global/locale/plugins.properties");
>    var regionbundle = strBundleService.createBundle("chrome://global-region/locale/region.properties");
> +  var Ci = Components.interfaces;

Since you are introducing a new Ci variable for Components.interfaces, can you move `var Ci` above `var strBundleService` and replace `Components.interfaces.nsIStringBundleService` with `Ci.nsIStringBundleService`?

@@ +153,5 @@
> +        if (plugin.blocklistState !== Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
> +          status += " (";
> +          switch (plugin.blocklistState)  {
> +            case Ci.nsIBlocklistService.STATE_SOFTBLOCKED:
> +              status += "Soft-blocked";

In Comment 7, bsmedberg recommended using the STATE constants' names as the string. So STATE_SOFTBLOCKED would return the string "STATE_SOFTBLOCKED", etc. That way we don't need to localize the string to different languages and developers can search the code for that constant name.

After further reflection, a dictionary mapping STATE constants to their string names would be more "JavaScripty" than a big switch statement. So you could replace the `if blocklistState !== STATE_NOT_BLOCKED` check and switch code with something like:

  var stateNames = {
    // We don't want to display STATE_NOT_BLOCKED // Ci.nsIBlocklistervice.STATE_NOT_BLOCKED:
    Ci.nsIBlocklistService.STATE_SOFTBLOCKED: "STATE_SOFTBLOCKED",
    Ci.nsIBlocklistService.STATE_BLOCKED: "STATE_BLOCKED",
    Ci.nsIBlocklistService.STATE_OUTDATED: "STATE_OUTDATED",
    Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE: "STATE_VULNERABLE_UPDATE_AVAILABLE",
    Ci.nsIBlocklistService.STATE_VULNERABLE_NO_UPDATE: "STATE_VULNERABLE_NO_UPDATE"
  };
  if (plugin.blocklistState in stateNames) {
    status += " (" + stateNames[plugin.blocklistState] + ")";
  }
Comment 18 Sunny [:darkowlzz] 2013-01-23 11:01:05 PST
Created attachment 705446 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made appropriate changes as mentioned in the feedback
Comment 19 Henrik Skupin (:whimboo) 2013-01-23 11:14:10 PST
Comment on attachment 705446 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

::: toolkit/content/plugins.html
@@ +145,5 @@
>  
> +        // "State: "
> +        var stateDd = document.createElement("dd");
> +        var state = document.createElement("span");
> +        state.setAttribute("class", "label");

Could we give all those entries above the table specific class names? Given that our Mozmill test will parse the page to check that it shows the correct content, it will be hard for us now to get this information. I would appreciate classes like e.g. 'active'.
Comment 20 Chris Peterson [:cpeterson] 2013-01-23 14:45:28 PST
Comment on attachment 705446 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

Sunny, I tested your patch on my Firefox and it looks good! :)

After you address Henrik's comments, you should send your patch to "bsmedberg" for his r+. I'm not a module owner for this code, so an r+ from me is not official. <:)
Comment 21 Sunny [:darkowlzz] 2013-01-24 07:50:10 PST
Created attachment 705855 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made changes as per the previous comments
Comment 22 Benjamin Smedberg [:bsmedberg] 2013-01-24 08:57:44 PST
Comment on attachment 705855 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

The "stateNames" map won't work, because the keys are "Ci.nsIBlocklistService.STATE_SOFTBLOCKED" strings and not the integers they represent.

I think you should construct this map at the top of the file kinda like this:

var stateNames = {};
["STATE_SOFTBLOCKED",
 "STATE_BLOCKED",
 "STATE_OUTDATED",
 "STATE_VULNERABLE_UPDATE_AVAILABLE",
 "STATE_VULNERABLE_NO_UPDATE"].forEach(function(label) {
  stateNames[Ci.nsIBlocklistService[label]] = label;
});
Comment 23 Sunny [:darkowlzz] 2013-01-24 10:16:55 PST
Created attachment 705949 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made suggested changes
Comment 24 Benjamin Smedberg [:bsmedberg] 2013-01-24 10:46:17 PST
Comment on attachment 705949 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

I really think you should move `stateNames` out of the "aPlugins" loop, since it's constant and doesn't change for each plugin. r=me with that change
Comment 25 Henrik Skupin (:whimboo) 2013-01-24 10:50:21 PST
Comment on attachment 705949 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

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

::: toolkit/content/plugins.html
@@ +145,5 @@
>  
> +        // "State: "
> +        var stateDd = document.createElement("dd");
> +        var state = document.createElement("span");
> +        state.setAttribute("class", "enabled");

Shouldn't it be of class "label enabled" then? Or shall we better name it "label state"?  Reading it now the enabled would give a wrong assumption about that field.
Comment 26 Sunny [:darkowlzz] 2013-01-25 05:14:31 PST
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 705949 [details] [diff] [review]
> Added 'state_label' property to plugins and a new 'State' field in
> about:plugins which shows enabled/disabled/blocklisted state of plugins
> 
> Review of attachment 705949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/plugins.html
> @@ +145,5 @@
> >  
> > +        // "State: "
> > +        var stateDd = document.createElement("dd");
> > +        var state = document.createElement("span");
> > +        state.setAttribute("class", "enabled");
> 
> Shouldn't it be of class "label enabled" then? Or shall we better name it
> "label state"?  Reading it now the enabled would give a wrong assumption
> about that field.

What is the right thing to do now?
Comment 27 Benjamin Smedberg [:bsmedberg] 2013-01-25 06:17:06 PST
"state" is probably better than "enabled", but I don't care very strongly.
Comment 28 Sunny [:darkowlzz] 2013-01-25 06:24:15 PST
Created attachment 706370 [details] [diff] [review]
Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins

Made suggested changes
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-01-26 10:01:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/698a863a2771
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-01-26 16:43:27 PST
https://hg.mozilla.org/mozilla-central/rev/698a863a2771
Comment 31 Chris Peterson [:cpeterson] 2013-01-28 10:39:48 PST
Thanks for your help, Sunny! :D
Comment 32 Henrik Skupin (:whimboo) 2014-08-29 15:27:38 PDT
Removing my name from in-qa-testsuite flag for a better query.

Note You need to log in before you can comment on or make changes to this bug.