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

RESOLVED FIXED in mozilla21

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: darkowlzz)

Tracking

unspecified
mozilla21
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 8 obsolete attachments)

4.41 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Whiteboard: [needs Mozmill test update] → [needs Mozmill test update][mentor=cpeterso][lang=js]
(Reporter)

Comment 5

5 years ago
Assigning bug to indiasuny000@gmail.com
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
(Reporter)

Comment 6

5 years ago
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

5 years ago
"State: ". And I wouldn't localize it, I would just include the exact strings "STATE_SOFTBLOCKED" etc.
(Reporter)

Comment 8

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #704248 - Flags: review?(cpeterson)
(Assignee)

Comment 11

5 years ago
Created attachment 704250 [details] [diff] [review]
Added 'state_label' property to plugins.
Attachment #704250 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #704250 - Flags: review? → review?(cpeterson)
(Assignee)

Updated

5 years ago
Attachment #704248 - Attachment is obsolete: true
Attachment #704248 - Flags: review?(cpeterson)
(Assignee)

Updated

5 years ago
Attachment #704250 - Attachment is obsolete: true
Attachment #704250 - Flags: review?(cpeterson)
(Assignee)

Comment 12

5 years ago
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
Attachment #704252 - Flags: review?(cpeterson)
(Assignee)

Updated

5 years ago
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
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
Attachment #704252 - Attachment is obsolete: true
Attachment #704252 - Flags: review?(cpeterson)
(Assignee)

Comment 14

5 years ago
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
Attachment #704257 - Flags: review?(cpeterson)
Attachment #704257 - Flags: feedback?(gfritzsche)
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+
(Assignee)

Comment 16

5 years ago
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
Attachment #704339 - Flags: review?(cpeterson)
(Assignee)

Updated

5 years ago
Attachment #704257 - Attachment is obsolete: true
Attachment #704257 - Flags: review?(cpeterson)
(Reporter)

Comment 17

5 years ago
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+
(Assignee)

Comment 18

5 years ago
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
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'.
(Reporter)

Comment 20

5 years ago
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+
(Assignee)

Comment 21

5 years ago
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
Attachment #705446 - Attachment is obsolete: true
Attachment #705855 - Flags: review?(benjamin)

Comment 22

5 years ago
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-
(Assignee)

Comment 23

5 years ago
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
Attachment #705855 - Attachment is obsolete: true
Attachment #705949 - Flags: review?(benjamin)

Comment 24

5 years ago
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.
Blocks: 834632
(Assignee)

Comment 26

5 years ago
(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

5 years ago
"state" is probably better than "enabled", but I don't care very strongly.
(Assignee)

Comment 28

5 years ago
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
Attachment #705949 - Attachment is obsolete: true
Attachment #706370 - Flags: checkin?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Attachment #706370 - Flags: checkin?
https://hg.mozilla.org/integration/mozilla-inbound/rev/698a863a2771
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/698a863a2771
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Reporter)

Comment 31

5 years ago
Thanks for your help, Sunny! :D
Depends on: 835969

Updated

5 years ago
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.