Closed Bug 831533 Opened 8 years ago Closed 8 years ago

about:plugins should show the enabled/disabled/blocklisted state of plugins

Categories

(Core :: Plug-ins, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: cpeterson, Assigned: darkowlzz)

References

Details

(Whiteboard: [needs Mozmill test update][mentor=cpeterso][lang=js])

Attachments

(1 file, 8 obsolete files)

4.41 KB, patch
Details | Diff | Splinter Review
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.
Once we have a patch and we can play with it we should also update our Mozmill test.
Flags: in-qa-testsuite?(hskupin)
Whiteboard: [needs Mozmill test update]
I'd rather WONTFIX this and remove about:plugins entirely. The addons manager should be used for this (adding info there, if needed).
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, ...).
(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.
Whiteboard: [needs Mozmill test update] → [needs Mozmill test update][mentor=cpeterso][lang=js]
Assigning bug to indiasuny000@gmail.com
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
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. :)
"State: ". And I wouldn't localize it, I would just include the exact strings "STATE_SOFTBLOCKED" etc.
hskupin: will the MozMill tests need to be updated if about:plugins includes both enabled and disabled plugins and a new "State:" field?
(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.
Attachment #704250 - Flags: review?
Attachment #704250 - Flags: review? → review?(cpeterson)
Attachment #704248 - Attachment is obsolete: true
Attachment #704248 - Flags: review?(cpeterson)
Attachment #704250 - Attachment is obsolete: true
Attachment #704250 - Flags: review?(cpeterson)
Attachment #704252 - Attachment description: Added 'state_label' property to plugins and a new 'State' field about:plugins → Added 'state_label' property to plugins and a new 'State' field in about:plugins which shows enabled/disabled/blocklisted state of plugins
Attachment #704252 - Flags: feedback?(gfritzsche)
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.
Attachment #704252 - Flags: feedback?(gfritzsche) → feedback+
Attachment #704252 - Attachment is obsolete: true
Attachment #704252 - Flags: review?(cpeterson)
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.
Attachment #704257 - Flags: feedback?(gfritzsche) → feedback+
Attachment #704257 - Attachment is obsolete: true
Attachment #704257 - Flags: review?(cpeterson)
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] + ")";
  }
Attachment #704339 - Flags: review?(cpeterson) → feedback+
Made appropriate changes as mentioned in the feedback
Attachment #704339 - Attachment is obsolete: true
Attachment #705446 - Flags: review?(cpeterson)
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 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. <:)
Attachment #705446 - Flags: review?(cpeterson) → feedback+
Made changes as per the previous comments
Attachment #705446 - Attachment is obsolete: true
Attachment #705855 - Flags: review?(benjamin)
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;
});
Attachment #705855 - Flags: review?(benjamin) → review-
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
Attachment #705949 - Flags: review?(benjamin) → review+
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.
(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?
"state" is probably better than "enabled", but I don't care very strongly.
Keywords: checkin-needed
Attachment #706370 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/698a863a2771
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Thanks for your help, Sunny! :D
Depends on: 835969
No longer depends on: 835969
Depends on: 878154
Removing my name from in-qa-testsuite flag for a better query.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.