about:plugins still uses document.write to do its stuff, and it still uses HTML 4.01 (i.e. no DTD for l10n). This patch brings it into the new millennium, with DOM, and DTD for l10n. It also brings with it a nice performance boost, especially for those with lots of plugins. The patch includes string changes (requiring practically no new translation, but moving of existing strings from plugins.properties to plugins.dtd).

There is a lot of code and l10n churn here for a feature that is mostly hidden from end-users. Performance is nice but I've never heard complaints about this page being particularly slow so I'm not sure this is worth the time, it certainly couldn't be taken for Firefox 4 due to the string changes.

(In reply to comment #1) > I'm not sure this is worth the time, it > certainly couldn't be taken for Firefox 4 due to the string changes. Yes, it's not exactly important. It can wait.

Comment on attachment 498079 [details] [diff] [review] about:plugins modernization, v.1 Going through unassigned review requests... Are you still interested in fixing this bug? Does this patch need updated? Bouncing initial review to jared since he knows a bit about plugins. :) Feel free to r-/close/whatever as appropriate. I've not looked at the patch beyond a 10 second skim.

Most of the patch should apply, and the rest should not take long to clean up. But my copy of the Mercurial repository is as dusty as your grandma's windowsills, so I don't intend to update it myself.

Comment on attachment 498079 [details] [diff] [review] about:plugins modernization, v.1 Review of attachment 498079 [details] [diff] [review]: ----------------------------------------------------------------- The patch applies cleanly except for jar.mn, which is easy to update. ::: toolkit/content/jar.mn @@ +17,5 @@ > * content/global/aboutSupport.js > * content/global/aboutSupport.xhtml > content/global/directionDetector.html > + content/global/plugins.xhtml > +* content/global/plugins.js plugins.js doesn't need to use the preprocessor, so we can remove the asterisk at the beginning of this line. ::: toolkit/content/plugins.js @@ +73,5 @@ > + } > + > + var content = document.getElementById("content"); > + > + for (var i = 0; i < numPlugins; i++) { Please use the new for...of syntax here. https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of @@ +77,5 @@ > + for (var i = 0; i < numPlugins; i++) { > + var plugin = navigator.plugins[i]; > + > + if (plugin) { > + var head = document.createElement("h3"); Please replace instances of 'var' with 'let'. @@ +125,5 @@ > + var tbody = document.createElement("tbody"); > + > + var numTypes = plugin.length; > + var mimetype, td; > + for (var j = 0; j < numTypes; j++) { Please use for...of here too. ::: toolkit/themes/winstripe/global/plugins.css @@ +43,5 @@ > + font-weight: bold; > +} > + > +dl { > + margin: 0px 0px 3px 0px; Where 0px is used, please replace it with just 0. ::: toolkit/themes/winstripe/global/jar.mn @@ +34,5 @@ > skin/classic/global/netError.css > skin/classic/global/numberbox.css > * skin/classic/global/notification.css > skin/classic/global/passwordmgr.css > + skin/classic/global/plugins.css There needs to be a plugins.css file for pinstripe.

Updated patch with Jared's comments. Utterly untested, just attaching for the record. I'm going to make some more changes I think.

Mmmmm... After further consideration, let's just move all the relevant into into about:addons. Then we can kill kill about:plugins entirely! All that's currently missing is the mimetype info and library name for the plugin. Blair wants to avoid exposing the technical mimetype stuff since it's not useful for most users, we agreed to just hide it behind a pref. Going to use this bug to just add info to about:addons, will file a followup to remove about:plugins.

Okey dokey. Here's a patch to add the mimetype info to the EM. Just requesting feedback at the moment, since it's a bit rough. * Pref isn't hooked up * Needs a bit better formatting, especially to make the line wrap when it's long. (Currently you get a scrollbar.)

Comment on attachment 616878 [details] [diff] [review] Patch v.3 Review of attachment 616878 [details] [diff] [review]: ----------------------------------------------------------------- Wherever possible, I want to avoid adding special-case code in the core for each addon type - the backend/frontend tries to stay generic, and let the providers do the special-case stuff. Thankfully, inline prefs are a pretty good match for displaying additional info as well - and let the provider add UI for entire addon types (see bug 335781 for a working example). Basic flow is: * Make PluginWrapper.optionsType = AddonManager.OPTIONS_TYPE_INLINE * Make PluginWrapper.optionsURL = chrome URL for a inline options XUL file * Listen for the addon-options-displayed notification, and fill in the relevant info then Is this meant to apply ontop of another patch? nsIPluginTag.getMimeTypes() doesn't seem to exist in the tree. ::: toolkit/mozapps/extensions/PluginProvider.jsm @@ +272,5 @@ > }); > > + this.__defineGetter__("pluginLibraries", function() { > + let libs = []; > + aTags.forEach(function(aTag) { for...of loops are the new hotness! (https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of) @@ +278,5 @@ > + }); > + return libs; > + }); > + > + this.__defineGetter__("pluginMimeTypes", function() { Feels like pluginMimeTypes, pluginMimeExtensions, and pluginMimeDescriptions should just be one property that returns an array of objects, like what aTag.getMimeTypes({}) seems to do. And yea, it will need to do dupe checks. @@ +283,5 @@ > + let types = []; > + aTags.forEach(function(aTag) { > + let t = aTag.getMimeTypes({}); > + // XXX do dupe checks? > + for (let i = 0; i < t.length; i++) for...of

Any progress on this?

Alan, would you still like to work on this?

Not really. I'm no longer assigned, after all.

Based on my interpretation of comment 7, it seems like the pref default should be false, so I changed it to be so. (The default was true in v.3). Addressed review comments. (In reply to Blair McBride (:Unfocused) from comment #9) > Wherever possible, I want to avoid adding special-case code in the core for > each addon type - the backend/frontend tries to stay generic, and let the > providers do the special-case stuff. Thankfully, inline prefs are a pretty > good match for displaying additional info as well - and let the provider add > UI for entire addon types (see bug 335781 for a working example). Basic flow > is: > * Make PluginWrapper.optionsType = AddonManager.OPTIONS_TYPE_INLINE > * Make PluginWrapper.optionsURL = chrome URL for a inline options XUL file > * Listen for the addon-options-displayed notification, and fill in the > relevant info then I'm having trouble figuring out what you mean by this. Is there something that needs to be done to the patch to address this?

Comment on attachment 657248 [details] [diff] [review] patch v.4 Review of attachment 657248 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Frank Yan (:fryn) from comment #13) > I'm having trouble figuring out what you mean by this. > Is there something that needs to be done to the patch to address this? Heh, yes. Basic take-away is that this shouldn't require any changes to extensions.js/xul. Since these fields are only relevant to plugins, the plugin provider should handle it itself - it can do that by using an inline options file for all plugins: https://developer.mozilla.org/en-US/docs/Extensions/Inline_Options The end result of doing that is that it *looks* the same in the UI, but the code is nicely separated. Bug 335781 has a working example of this, which probably illustrates it better than I'm explaining. In the patch in that bug, see searchPrefs.xul, SearchEngineAddon.optionsURL/SearchEngineAddon.optionsType in SearchEngineProvider.jsm, and uses of "addon-options-displayed" and "addon-options-hidden" in SearchEngineProvider.jsm.

(In reply to Frank Yan (:fryn) from comment #13) > Based on my interpretation of comment 7, it seems like the pref default > should be false, so I changed it to be so. (The default was true in v.3). Yeah, it was just that way for convenience while developing.

Rewritten with better code separation as explained by Blair. :)

Comment on attachment 661004 [details] [diff] [review] patch v5 Review of attachment 661004 [details] [diff] [review]: ----------------------------------------------------------------- Ah, much better :) ::: toolkit/mozapps/extensions/PluginProvider.jsm @@ +55,5 @@ > plugins: null, > > + startup: function PL_startup() { > + Services.obs.addObserver(this, ADDON_OPTIONS_DISPLAYED, false); > + AddonManager.addAddonListener(this); Don't seem to handle any events, so no need to register this as a listen. @@ +71,5 @@ > + > + observe: function(aSubject, aTopic, aData) { > + switch (aTopic) { > + case ADDON_OPTIONS_DISPLAYED: > + if (aTopic != "addon-options-displayed") This seems a little redundant. Also, it's superfluous. @@ +75,5 @@ > + if (aTopic != "addon-options-displayed") > + return; > + > + let doc = aSubject; > + let addon = doc.defaultView.gViewController.currentViewObj.getSelectedAddon(); There's a very strong separation between the backend and frontend - providers shouldn't touch the frontend (and anyway, this would break on mobile). Inline prefs being the exception, since the provider is putting it there, and controls it. aData is the extension ID, so can just check if aData is in this.plugins, and/or use this.getAddonByID() @@ +82,5 @@ > + > + doc.getElementById("pluginLibraries").value = addon.pluginLibraries.join(", "); > + > + let label = doc.getElementById("pluginMimeTypes"); > + if (Services.prefs.getBoolPref("extensions.ui.showAdvanced")) { This should hide the libraries too. @@ +87,5 @@ > + let types = addon.pluginMimeTypes; > + let infos = []; > + for (let type of types) { > + let extras = [type.description, type.suffixes].filter(function(x) x).join(": "); > + infos.push(type.type + (extras ? " (" + extras + ")" : "")); Trim leading/trailing whitespace from type.description - the data that some plugins provide is sometimes very messy :( @@ +342,5 @@ > } > > PluginWrapper.prototype = { > + optionsType: AddonManager.OPTIONS_TYPE_INLINE, > + optionsURL: "chrome://mozapps/content/extensions/pluginPrefs.xul", These should depend on the extensions.ui.showAdvanced pref. ::: toolkit/mozapps/extensions/content/pluginPrefs.xul @@ +10,5 @@ > + <setting type="control" title="&pluginLibraries;"> > + <label id="pluginLibraries"/> > + </setting> > + <setting type="control" title="&pluginMimeTypes;"> > + <label id="pluginMimeTypes" class="text-list"/> Should make this text (and the libraries) user-selectable via -moz-user-select, it seems like the type of data some people would copy (the type of people that would even look at it in the first place). ::: toolkit/themes/winstripe/mozapps/extensions/extensions.css @@ +867,5 @@ > min-height: 30px; > } > > +.text-list { > + white-space: pre-line; Hmm, should probably have this in content.

Addressed review comments. I tried to format the plugin mime types nicely like we do in about:plugins, but I ran into all sorts of problems, including the Add-ons Manager code removing the text nodes of each <setting/> element and using them for its desc attribute and also <xhtml:table/> child text nodes not retaining proper formatting when being copied out of the manager. To avoid adding lots of complexity to attempt to work around these issues, I decided to stick to plain text. Since we're putting it inside each plugin's "Preferences", I don't think it makes sense to have a pref for it. It already requires going to the Add-ons Manager -> Plugins -> Preferences to retrieve information about each plugin. If we require a pref, with the pref off, each plugin will display a "Preferences" button that doesn't really provide any information at all. Requiring the user to go to about:config and toggle a value first is unnecessary user inconvenience, especially when no runtime overhead is incurred until the moment that a user chooses to click "Preferences".

(In reply to Frank Yan (:fryn) from comment #18) > Since we're putting it inside each plugin's "Preferences", I don't think it > makes sense to have a pref for it. > It already requires going to the Add-ons Manager -> Plugins -> Preferences > to retrieve information about each plugin. That's the Details pane, can also get to it via "More" link and just double clicking on an entry. Bug 625465 (held up by a test failure) is making that more accessible, such that it won't be relegated to being a semi-hidden UI. The library and mimetypes data are strongly in the technical realm - an implementation detail. IMO, it's not something that should be presented by default, for the same reason that about:plugins was never linked to in any UI (except about:support). I don't want to elevate that to a more prominent UI, especially as plugins are becoming less important. > If we require a pref, with the > pref off, each plugin will display a "Preferences" button that doesn't > really provide any information at all. That's why optionsType/optionsURL should depend on the pref, each having a value of null when the advanced UI is disabled (when they're null, the Preferences button isn't displayed). > Requiring the user to go to > about:config and toggle a value first is unnecessary user inconvenience, We could add a toggle to the utilities menu (dropdown by the search field) - separate bug though, as I'm not even convinced that's needed. But either way, its very rare to need this info. In the 80/20/2 rule, this is like the leftover 0.001%. > especially when no runtime overhead is incurred until the moment that a user > chooses to click "Preferences". I don't think that's something that should factor in here - any runtime overhead will be too small to measure, let alone perceive.

Comment on attachment 661527 [details] [diff] [review] patch v6 Review of attachment 661527 [details] [diff] [review]: ----------------------------------------------------------------- See comment 19. ::: toolkit/mozapps/extensions/PluginProvider.jsm @@ +75,5 @@ > + aSubject.getElementById("libraries").textContent = plugin.pluginLibraries.join(", "); > + > + let label = aSubject.getElementById("mimeTypes"), types = []; > + for (let type of plugin.pluginMimeTypes) { > + let extras = [type.description.trim(), type.suffixes].filter(function(x) x).join(": "); Nit: 80 char? ::: toolkit/mozapps/extensions/content/pluginPrefs.xul @@ +7,5 @@ > +<!DOCTYPE window SYSTEM "chrome://mozapps/locale/plugins/plugins.dtd"> > + > +<vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + <setting type="control" title="&plugin.file;"> > + <label class="text-list" id="libraries"/> "libraries" is a bit too generic - revert this back to the ID that was used in previous patches? mimeTypes too.

Comment on attachment 661527 [details] [diff] [review] patch v6 Review of attachment 661527 [details] [diff] [review]: ----------------------------------------------------------------- Oh, and: tests? The "Test plug-in" plugin is available for testing, and handles the application/x-test mimetype. tests/xpcshell/test_plugins.js for the getters, and a new browser-chrome test for the inline settings stuff.

(Part 2 will be test(s).) I asked Dolske about the reasoning for this bug, and he described the objective as centralizing one easy location in the browser to find information or change settings for plugins. To achieve this end, he would like about:plugins redirect to the plugins pane of about:addons, because he surmised that people are used to typing in about:plugins. (That's to be done in a separate bug.) My view is that, if we want to achieve the above objective, it doesn't make sense to add a pref. If people type in about:plugins to find their plugin information, but as a result of this, it doesn't show up, and they have to use a search engine and then scroll through an arbitrary forum topic to figure out how to find it again, we will have failed at this objective.

Comment on attachment 670981 [details] [diff] [review] part 1: patch Review of attachment 670981 [details] [diff] [review]: ----------------------------------------------------------------- Discussed this with Boriss and came to the following conclusion: yep, ok.

Comment on attachment 673000 [details] [diff] [review] part 2: test Review of attachment 673000 [details] [diff] [review]: ----------------------------------------------------------------- Good to go!

Part 1: Implementation https://hg.mozilla.org/integration/mozilla-inbound/rev/81e812a83e9b Part 2: Test https://hg.mozilla.org/integration/mozilla-inbound/rev/907385f76803

https://hg.mozilla.org/mozilla-central/rev/81e812a83e9b https://hg.mozilla.org/mozilla-central/rev/907385f76803

(In reply to Frank Yan (:fryn) from comment #22) > To achieve this end, he would > like about:plugins redirect to the plugins pane of about:addons, because he > surmised that people are used to typing in about:plugins. (That's to be done > in a separate bug.) about:plugins lives on; was a bug ever filed for this?

Yes, bug 747301. Which was wontfix'd.