Since (almost) everything is in toolkit/mozapps/extensions/content and completely replaces existing files, I'll attach the files themselves, rather than generating diffs that remove every line of existing files. This is as of http://hg.mozilla.org/projects/addonsmgr/rev/f06fe80c963c

BL bindings

Note: This includes global and theme CSS. I refrained from separating them out into the themes, so I don't have to keep updating 3 identical files. This will need to be done before final review and landing. Eventually the 3 theme files will diverge, but I don't expect that to happen until much later, when we have the final polished design.

I'll sort out the final diffs tomorrow, which will include about.js/xul and a change to browser.js

Didn't get much change to look at these so going to review the revisions from http://hg.mozilla.org/projects/addonsmgr/rev/5c4b8b9de70c.

Comment on attachment 435427 [details] extensions.xul This file is a big block of text, it would be helpful to add some spacing to separate out the major parts of the UI and add some comments to them too. Some of the lines go longer than 80 characters, clean those up please. Throughout this UI you're using different panels to display that you seem to be selecting by switching all but one to be hidden. Please use decks instead. I probably need to do another pass on this after I've looked through the JS. A couple of the comments here are UI questions that need to be worked through with Boriss. > <hbox flex="1"> > <richlistbox id="categories"> > <richlistitem id="category-search" name="&view.search.label;" value="moz-em://search/" hidden="true"/> > <richlistitem id="category-discover" name="&view.discover.label;" value="moz-em://discover/"/> > <richlistitem id="category-languages" name="&view.languages.label;" value="moz-em://list/language"/> > <richlistitem id="category-searchengines" name="&view.searchengines.label;" value="moz-em://list/searchengine"/> > <richlistitem id="category-extensions" name="&view.features.label;" value="moz-em://list/extension"/> > <richlistitem id="category-themes" name="&view.appearance.label;" value="moz-em://list/theme"/> > <richlistitem id="category-plugins" name="&view.plugins.label;" value="moz-em://list/plugin"/> > </richlistbox> Not totally keen on the uri-like values but I'll think on it. At the very least I think addons:// would be more appropriate. > <splitter class="pane-splitter" collapse="none"/> > <vbox id="view-container" flex="1"> > <hbox id="header"> > <label id="header-name"/> > <hbox id="header-link-container"> > <image class="go-back"/> > <label id="header-link"/> > </hbox> I believe you should be able to do this with list-style-image on the label rather than having to put an explicit image into the XUL. Also try using text-link (or other means) to make the label focusable. > <spacer flex="1"/> > <!-- XXXunf after we get the fix for bug 547224, replace emptytext="..." with placeholder="..." --> > <textbox id="header-search" type="search" searchbutton="true" emptytext="&search.placeholder;"/> > </hbox> Looks like that bug is fixed now, are we waiting on anything else? > <vbox id="discover-view" flex="1" class="view-pane" hidden="true"> > <browser id="discover-browser" type="content" flex="1" disablehistory="true"/> > </vbox> I think we need to hide the browser and display a loading throbber until the page has actually loaded, it just sits there blank for me for a while. Can be done in a follow-up bug. > <vbox id="search-view" flex="1" class="view-pane" hidden="true"> > <hbox class="view-header"> > <spacer flex="1"/> > <hbox id="search-sorters" class="sorters" sortby="name" ascending="true"/> > </hbox> The class "sorters" doesn't sound right, how about "sort-header"? > <vbox class="list-container" flex="1"> > <hbox class="search-filter"> > <label value="&search.filter.label;"/> > <checkbox id="search-filter-local" label="&search.filter.installed.label;" > checked="true" disabled="true"/> > <checkbox id="search-filter-remote" label="&search.filter.available.label;" > checked="false" disabled="true"/> > </hbox> Could this work better in the view-header above? I also wonder if it might be more understandable to people to be a radio buttons with "All", "Installed" and "Available", though I think the word Available is totally wrong. See what Boriss thinks. > <hbox id="detail-view" flex="1" class="view-pane" hidden="true"> > <spacer flex="1"/> > <hbox class="loading" flex="1"> > <image/> > <label value="&loading.label;"/> > </hbox> > <vbox class="detail-view-container" flex="8"> These flex values seem a bit weird. I'm not sure why the top and bottom spacers are here and I think the loading and detail should just be in a deck. Or do we even need to care? I've never seen what the loading bit looks like, this is all likely fast enough that I don't think we need to worry about it for now. > <hbox class="detail-desc"> > <!-- XXXunf placeholder --> > <vbox hidden="true"> > <image id="detail-screenshot"/> > <label value="1 of 5"/> > <spacer flex="1"/> > </vbox> > <vbox flex="1"> > <description flex="1" id="detail-desc"/> > <!-- XXXunf placeholder --> > <label class="meta-value meta-rating" userrating="3" avgrating="4" maxrating="5" numreviews="23"/> None of the mockups show a rating here. > <hbox> > <button id="Detail-contribute" class="contribute" No uppercase in the id please. > label="&cmd.contribute.label;" > accesskey="&cmd.contribute.accesskey;" > tooltiptext="&cmd.contribute.tooltip;"/> > <spacer flex="2"/> > <button id="detail-uninstall" class="addon-control remove" > label="&cmd.uninstall.label;" > accesskey="&cmd.uninstall.accesskey;" > tooltiptext="&cmd.uninstall.tooltip;" > command="cmd_uninstallItem"/> > <button id="detail-enable" class="addon-control enable" > label="&cmd.enable.label;" > accesskey="&cmd.enable.accesskey;" > tooltiptext="&cmd.enable.tooltip;" > command="cmd_enableItem"/> > <button id="detail-disable" class="addon-control disable" > label="&cmd.disable.label;" > accesskey="&cmd.disable.accesskey;" > tooltiptext="&cmd.disable.tooltip;" > command="cmd_disableItem"/> > <spacer flex="1"/> > </hbox> On add-ons with little/no description this makes the buttons seem way off on their own, not sure the best way to tackle that. Maybe the problem goes away once we have screenshots working. > <hbox class="detail-extra"> > <vbox class="detail-prefs" flex="1"> > <checkbox id="detail-autoupdate" checked="true" > label="&detail.updateAutomatically.label;" > accesskey="&detail.updateAutomatically.accesskey;" > tooltiptext="&detail.updateAutomatically.tooltip;"/> > <button id="detail-findupdates" class="addon-control" > label="&detail.checkForUpdates.label;" > accesskey="&detail.checkForUpdates.accesskey;" > tooltiptext="&detail.checkForUpdates.tooltip;" > command="cmd_findItemUpdates"/> > <button id="detail-prefs" class="addon-control" > label="&detail.showPreferences.label;" > accesskey="&detail.showPreferences.accesskey;" > tooltiptext="&detail.showPreferences.tooltip;" > command="cmd_showItemPreferences"/> > </vbox> For add-ons that aren't installed in the profile I think we should add some text here explaining that we won't check the add-on for updates or some such.

Comment on attachment 435436 [details] extensions.js More comments please! >var gStrings = {}; >XPCOMUtils.defineLazyServiceGetter(gStrings, "bundleSvc", > "@mozilla.org/intl/stringbundle;1", > "nsIStringBundleService"); File a bug to get the string bundle service into Services.jsm and then use that >var gTypeNames = { > "recommended" : "Recommended", > "language" : "Languages", > "searchengine" : "Search Engines", > "extension" : "Features", > "theme" : "Appearance", > "plugin" : "Plugins" >}; Localize these please. Put them into the properties as header-<type> or somesuch to avoid this object at all. > initialize: function() { > var self = this; > ["onEnabling", "onEnabled", "onDisabling", "onDisabled", "onUninstalling", > "onInstalled", "onOperationCancelled", "onUpdateAvailable", > "onUpdateFinished"].forEach(function(aEvent) { > self[aEvent] = function() { > self.delegateAddonEvent(aEvent, [].splice.call(arguments, 0)); Use Array.splice.call please. > }; > }); > > ["onNewInstall", "onDownloadStarted", "onDownloadEnded", "onDownloadFailed", > "onDownloadProgress", "onInstallStarted", "onInstallEnded", > "onInstallFailed"].forEach(function(aEvent) { > self[aEvent] = function() { > self.delegateInstallEvent(aEvent, [].splice.call(arguments, 0)); Ditto. > register: function(aListener, aAddonId) { > if (!(aAddonId in this._listeners)) > this._listeners[aAddonId] = []; > else if (this._listeners[aAddonId].indexOf(aListener) != -1) > return; > this._listeners[aAddonId].push(aListener); > }, This needs a more descriptive name, maybe registerForAddonEvents or registerAddonListener. And make the registerForInstalls consistent with it. I also kind of think it makes more sense to have aAddonId first, roughly matches addEventListener then and reads right to me. > unregister: function(aListener, aAddonId) { Likewise > loadView: function(aViewId) { > if (aViewId == this.currentViewId) > return; > > var view = this.parseViewId(aViewId); > > if (!view.type || !(view.type in this.viewObjects)) { > /// XXXunf remove this warning > Components.utils.reportError("Invalid view: " + view.type); Yes please > var viewObj = this.viewObjects[view.type]; > if (!viewObj.node) { > /// XXXunf remove this warning > Components.utils.reportError("Root node doesn't exist for '" + view.type + "' view"); Again, unless there really is a case where that happens? > commands: { > cmd_restartApp: { > isEnabled: function() true, > doCommand: function() { > // Notify all windows that an application quit has been requested. > var cancelQuit = Cc["@mozilla.org/supports-PRBool;1"] > .createInstance(Ci.nsISupportsPRBool); > Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart"); > > // Something aborted the quit process. > if (cancelQuit.data) > return; > > Cc["@mozilla.org/toolkit/app-startup;1"] > .getService(Ci.nsIAppStartup) > .quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit); Application.restart() does all this for you. > cmd_findAllUpdates: { > isEnabled: function() true, This should probably be disabled if there are no add-ons installed that can be updated. Of course that can only be gotten asynchronously which kind of doesn't really work for here. Oh well. > cmd_showItemAbout: { > isEnabled: function(aAddon) { > if (!aAddon) > return false; > // XXXunf check to make sure this is an installed item Actually you should still be able to show the default about dialog for pending-install items I think. > supportsCommand: function(aCommand) { > if (aCommand in this.commands) > return true; > return false; > }, Just return (aCommand in this.commands) > isCommandEnabled: function(aCommand) { > var addon = this.currentViewObj.getSelectedAddon(); > return this.commands[aCommand].isEnabled(addon); > }, I suspect it will never crop up but put a check in that the command actually exists and return false if it doesn't. > doCommand: function(aCommand) { > var addon = this.currentViewObj.getSelectedAddon(); > var cmd = this.commands[aCommand]; > if (!cmd.isEnabled(addon)) > return; > cmd.doCommand(addon); > }, Again, just double check that the command exists. >function prettifyURL(aURL) { > if (!aURL) > return ""; > return aURL.replace(/^http(s|)\:\/\//, "") > .replace(/^www./, "") > .replace(/\/$/, ""); >} I don't like this, we need to come up with something else to display rather than a part of the url. Perhaps just the hostname would work but I'm thinking not displaying the URL at all would be better. >function $(aNodeId) { > return document.getElementById(aNodeId); >} Please don't use this. >function createItem(aAddon, aIsInstall) { Since you're passing both addons and installs to this I think you need to change the first parameter to something like aItem or. > // set only attributes needed for sorting and XBL binding, > // the binding handles the rest > item.setAttribute("value", aAddon.id); > item.setAttribute("dateUpdated", (new Date).valueOf() - Math.floor(Math.random() * 100000000000)); // XXXapi This should be fixed in the API now. > var size = Math.floor(Math.random() * 1024 * 1024 * 2); > size = ("00000000000" + size).slice(-10); // HACK: nsIXULSortService doesn't do numerical sorting Really? Can you file a bug on this A.S.A.P. and CC enn, we'll get it fixed. >var gCategories = { > node: null, > _search: null, > > initialize: function() { > this.node = document.getElementById("categories"); > this._search = this.get("moz-em://search/"); > > this.maybeHideSearch(); > > var self = this; > this.node.addEventListener("select", function() { > self.maybeHideSearch(); > gViewController.loadView(self.node.selectedItem.value); > }, false); > }, > > select: function(aId) { > if (this.node.selectedItem && > this.node.selectedItem.value == aId) > return; > > var view = gViewController.parseViewId(aId); > if (view.type == "detail") > return; > > if (view.type == "search") > aId = "moz-em://search/"; You already have the node for this cached right? > var item = this.get(aId); > if (item) { > item.hidden = false; > this.node.suppressOnSelect = true; > this.node.selectedItem = item; > this.node.suppressOnSelect = false; Does selecting it programmatically really send a select event? That seems like a bug. > this.node.ensureElementIsVisible(item); > > this.maybeHideSearch(); > } > }, > > get: function(aId) { > for (let i = 0; i < this.node.itemCount; i++) { > let item = this.node.getItemAtIndex(i); > if (item.value == aId) > return item; > } > return null; Please just use document.getElementsByAttribute for this. >var gHeader = { > _search: null, > _name: null, > _linkText: null, > _linkContainer: null, > _dest: "", > > initialize: function() { > this._name = document.getElementById("header-name"); > this._linkContainer = document.getElementById("header-link-container"); > this._linkText = document.getElementById("header-link"); > this._search = document.getElementById("header-search"); > > var self = this; > this._linkContainer.addEventListener("click", function() { > gViewController.loadView(self._dest); > }, false); > > this._search.addEventListener("command", function(aEvent) { > var query = aEvent.target.value; > gViewController.loadView("moz-em://search/" + query); Do appropriate URI encoding here. Not exactly necessary but if we're going to pretend to be a URI we should follow a URI's rules. > showBackButton: function() { > this._linkText.value = "Back to " + this._name.value; Localize this properly > this._dest = gViewController.previousViewId; Can't we just look at the selected item in the categories richlistbox to get this? Either that or have it as a parameter to showBackButton. >var gDiscoverView = { > node: null, > _browser: null, > initialize: function() { > this.node = document.getElementById("discover-view"); > this._browser = document.getElementById("discover-browser"); > this._browser.homePage = AMO_URL; > }, > show: function() { > gHeader.setName("Discover Add-ons"); Localize this properly. > // load content only if we're not already showing something on AMO > // XXXunf should only be comparing hostname > if (this._browser.currentURI.spec.indexOf(this._browser.homePage) == -1) > this._browser.goHome(); > > gViewController.updateCommands(); > }, > hide: function() { }, > getSelectedAddon: function() null >}; Please file a follow-up bug on making sure we stay secure here. The discovery pane should never show anything insecure or anything not on the same host as the original URL. >var gSearchView = { > node: null, > _sorters: null, > _listBox: null, > _emptyNotice: null, > > initialize: function() { > this.node = document.getElementById("search-view"); > this._sorters = document.getElementById("search-sorters"); > this._sorters.handler = this; > this._listBox = document.getElementById("search-list"); > this._emptyNotice = document.getElementById("search-list-empty"); > }, > > show: function(aQuery) { > gHeader.setName("Search"); Localize this. A simple option for these would be to put a title attribute on the XUL box for the view. > this.showEmptyNotice(false); > > gHeader.searchQuery = aQuery; > aQuery = aQuery.trim().toLocaleLowerCase(); > > while (this._listBox.itemCount > 0) > this._listBox.removeItemAt(0); > > var self = this; > AddonManager.getAddonsByType([], function(aAddonsList) { Pass null instead of [] for the types. > for (let i = 0; i < aAddonsList.length; i++) { > let addon = aAddonsList[i]; > let score = 0; > if (aQuery.length > 0) { > score = self.getMatchScore(addon, aQuery); > if (score == 0) > continue; > } Nit: Newline here > calculateMatchScore: function(aStr, aQuery, aMultiplier) { > var score = 0; > if (aQuery.length == 0) > return score; > > aStr = aStr.trim().toLocaleLowerCase(); > var haystack = aStr.split(/\W+/); > var needles = aQuery.split(/\W+/); > > for (let n = 0; n < needles.length; n++) { > for (let h = 0; h < haystack.length; h++) { > if (haystack[h] == needles[n]) { > // matching whole words is best > score += 1; > } else { > let i = haystack[h].indexOf(needles[n]); > if (i == 0) // matching on word boundries is also good > score += 0.6; > else if (i > 0) // substring matches not so good > score += 0.3; > } > } > } > > // give progressively higher score for longer queries, since longer queries > // are more likely to be unique and therefore more relevant. > if (needles.length > 1 && aStr.indexOf(aQuery) != -1) > score += needles.length; > > return score * aMultiplier; > }, Has this algorithm and the magic numbers within come from somewhere? Might be worth referencing if so. Please also put the magic numbers as suitably named constants. >var gListView = { > node: null, > _listBox: null, > _emptyNotice: null, > _sorters: null, > > initialize: function() { > this.node = document.getElementById("list-view"); > this._sorters = document.getElementById("list-sorters"); > this._sorters.handler = this; > this._listBox = document.getElementById("addon-list"); > this._emptyNotice = document.getElementById("addon-list-empty"); > }, Nit: newline here > show: function(aType) { > gHeader.setName(gTypeNames[aType]); > this.showEmptyNotice(false); > > var types = [aType]; > var installTypes = [aType]; > if (aType == "extension") { > types.push("bootstrapped"); > installTypes = types.concat(""); > } For the future but I think we should be able to support displaying multiple types in the same list. We're also going to get rid of the bootstrapped type so that tweak will go away. > var self = this; > var addons = null, installs = null; Nit: newline here > AddonManager.getAddonsByType(types, function(aAddonsList) { > addons = aAddonsList; > updateList(); > }); > > AddonManager.getInstalls(installTypes, function(aInstallsList) { > installs = aInstallsList; > updateList(); > gEventManager.registerForInstalls(this); > }); Just nest these, I don't believe you gain anything in performance by trying to run them at the same time. > onNewInstall: function(aInstall) { > // the event manager ensures that upgrades are filtered out > window.__install = aInstall; // XXXunf remove this! Yep >var gDetailView = { > node: null, > _addon: null, > initialize: function() { > this.node = document.getElementById("detail-view"); > }, Nit: newline here > show: function(aAddonId) { > var self = this; > this.node.setAttribute("loading", true); > setTimeout(function() { > if (self.node.hasAttribute("loading")) > self.node.setAttribute("loading-extended", true); > }, 100); Rather than using the loading attribute just cancel the timer when we get the data. > var desc = $("detail-desc"); > if (desc.firstChild) > desc.removeChild(desc.firstChild); > desc.appendChild(document.createTextNode(aAddon.description)); Use desc.textContent = aAddon.description.

Comment on attachment 435442 [details] extensions.xml Add newlines between content/implementation and each of the methods/fields/constructors please. Also comments giving a rough idea of what each binding is for. ><bindings id="addonBindings" > xmlns="http://www.mozilla.org/xbl" > xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > xmlns:xbl="http://www.mozilla.org/xbl"> > > <binding id="rating"> > <content orient="horizontal"> > <xul:label anonid="label" value="&rating.yourRating.label;"/> I think that this label should be outside of the binding to make it a more reusable ratings display. > <xul:hbox anonid="star-container" class="star-container"> > <xul:image class="star"/> > <xul:image class="star"/> > <xul:image class="star"/> > <xul:image class="star"/> > <xul:image class="star"/> > </xul:hbox> > <xul:label anonid="reviews" class="text-link"/> Again the reviews label really belongs separate from the binding. > <property name="cloudRating"> > <getter><![CDATA[ > if (this.hasAttribute("cloudrating")) > return this.getAttribute("cloudrating"); > return -1; > ]]></getter> > <setter><![CDATA[ > this.setAttribute("cloudrating", val); > if (this.showRating == "cloud") > this._updateStars(); > ]]></setter> > </property> This sounds needlessly buzzwordy. How about averageRating, since that's what this is. > <property name="showRating"> > <getter><![CDATA[ > if (this.hasAttribute("showrating")) > return this.getAttribute("showrating"); > return "cloud"; > ]]></getter> > <setter><![CDATA[ > if (val != "cloud" || val != "user") > throw new Error("Invalid value"); > this.setAttribute("showrating", val); > this._updateStars(); > ]]></setter> > </property> > <method name="_updateStars"> > <body><![CDATA[ > var stars = this._starContainer.children; > > var rating = this[this.showRating + "Rating"]; > for (let i = 0; i < stars.length; i++) { > if (rating > (i + 0.5)) > stars[i].setAttribute("on", true); > else if (stars[i].hasAttribute("on")) > stars[i].removeAttribute("on"); > } > ]]></body> > </method> This binding is all a little unclear to me, maybe I don't know how it is meant to work. Perhaps it might be better with the content as two hboxes of stars that get shown/hidden based on a :hover css rule or something. > <binding id="download-progress"> > <content> > <xul:stack flex="1"> > <xul:hbox flex="1"> > <xul:hbox class="start-cap"/> > <xul:progressmeter anonid="progress" class="progress" flex="1" min="0" max="100"/> > <xul:hbox class="end-cap"/> > </xul:hbox> Are these caps here just for image purposes? Can we use multiple-css-backgrounds to achieve the same thing? > <binding id="categories-list" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox"> > <implementation> > <!-- This needs to be overridden to allow the fancy animation while not allowing that item to be selected when hiding --> Can't you just make the item disabled when hiding it? > <binding id="category" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem"> > <content> > <xul:image anonid="icon" class="icon"/> > <xul:label anonid="name" class="name" flex="1" xbl:inherits="value=name"/> > <xul:label anonid="badge" class="badge" value="0"/> Have this inherit its value from an attribute "count" I think. > <binding id="creator-link"> > <content> > <xul:label anonid="prefix"/> > <xul:label anonid="creator-link" class="creator-link text-link"/> > <xul:label anonid="creator-name" class="creator-name"/> > <xul:label anonid="postfix"/> We should flag this as something needing accessibility attention. I think screenreaders will barf on this. > <field name="_baseString"> > gStrings.ext.GetStringFromName("createdBy"); I'd rather not rely on a variable from the window. > <binding id="install-status"> > <content> > <xul:label anonid="message"/> > <xul:progressmeter anonid="progress" class="download-progress"/> > <xul:button anonid="restart-needed" hidden="true" command="cmd_restartApp" > label="&addon.restartNow.label;" > tooltiptext="&addon.restartNow.tooltip;"/> > <xul:button anonid="restart-install" hidden="true" > label="&addon.install.label;" > tooltiptext="&addon.install.tooltip;" > oncommand="document.getBindingParent(this).restartInstall();"/> > </content> > <implementation> > <constructor><![CDATA[ > if (this.mInstall) > this.initWithInstall(this.mInstall); > else if(this.mControl.mAddon.install) Nit: space after the if > <method name="refreshState"> > <body><![CDATA[ > this._restartInstall.hidden = true; > this._restartNeeded.hidden = true; Use a variable to store these states and set them at the end rather than flickering them. > <method name="onDownloadProgress"> > <body><![CDATA[ > this._progress.maxProgress = this.mInstall.maxProgress; > this._progress.progress = this.mInstall.progress; Support undetermined mode. > <binding id="addon-base" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem"> > <implementation> > <method name="hasPermission"> > <parameter name="aPerm"/> > <body><![CDATA[ > return hasPermission(this.mAddon, aPerm); Again, don't rely on functions from the window please. > <method name="isPending"> > <parameter name="aAction"/> > <body><![CDATA[ > return isPending(this.mAddon, aAction); Ditto. > <xul:button type="checkbox" anonid="toggle-more" class="toggle-more" > label="&showMore.label;" tooltiptext="&showMore.tooltip;" > showless="&showLess.label;" showlesstooltip="&showLess.tooltip;" > showmore="&showMore.label;" showmoretooltip="&showMore.tooltip;" > oncommand="document.getBindingParent(this).toggleDetails();"/> Add an accesskey. > this._showStatus("none"); > this._updateSize(); > this._updateDates(); > this._updateState(); > > this._incompatibleContainer.hidden = this.mAddon.isCompatible; I'd rather set this as an attribute on the richlistitem and show/hide isCompatible using a style rule. This gives themes scope for making the entire listitem red or whatever when incompatible. > let desc = document.createTextNode(this.mAddon.description); > this._description.appendChild(desc); this._description.textContent = ... > if (numExtraDetails == 0) > this._toggleMore.hidden = true; > > gEventManager.register(this, this.mAddon.id); Bleh ok I guess you get away with this one. > <method name="_updateDates"> > <body><![CDATA[ > function formatDate(aDate) { > return Cc["@mozilla.org/intl/scriptabledateformat;1"] > .getService(Ci.nsIScriptableDateFormat) > .FormatDate("", > Ci.nsIScriptableDateFormat.dateFormatLong, > aDate.getFullYear(), > aDate.getMonth() + 1, > aDate.getDate() + 1 > ); > } > > //XXXunf api > //this._dateUpdated.value = formatDate(this.mAddon.dateUpdated); Should work > var showAsActive = this.mAddon.isActive; > if (this.isPending("enable")) > showAsActive = true; > else if (this.isPending("disable")) > showAsActive = false; > this.setAttribute("enabled", showAsActive); Use active as the attribute name for consistencies sake > <handler event="click" button="0" clickcount="2"> > <action><![CDATA[ > gViewController.loadView("moz-em://detail/" + this.mAddon.id); > ]]></action> > </handler> Need to support opening this from the keyboard too. > <field name="_restartBtn" readonly="true"> > document.getAnonymousElementByAttribute(this, "anonid", "restart-btn"); > </field> > <method name="undoUninstall"> Call this cancelUninstall. > <body><![CDATA[ > this.removeAttribute("restartrequired"); > this.mAddon.userDisabled = false; > this.mAddon.cancelUninstall(); > ]]></body> > </method> > <method name="refreshInfo"> > <body><![CDATA[ > this.mAddon = this.mInstall.addon; > if (this.mAddon) { > this._icon.src = this.mAddon.iconURL || this.mInstall.iconURL; > this._name.value = this.mAddon.name; > } else { > this._icon.src = this.mInstall.iconURL; > this._name.value = this.mInstall.name; //XXXunf is this always available? No it isn't. Fallback to the filename part of sourceURL if it is null I think.

Only skimmed over extensions.css, don't think it's worth me going into any depth on that just yet but it looks basically ok, these reviews should give you enough to go on for now. Overall this is looking really good. I'll need to look over all of them again once you've addressed/answered all this stuff but it's pretty close to being right already.

(In reply to comment #8) > I believe you should be able to do this with list-style-image on the label > rather than having to put an explicit image into the XUL. Also try using > text-link (or other means) to make the label focusable. I'm not aware of any (sane) way to do this - pointers? Anyway, changed this to a styled button to fix all these types of issues. Think it makes more sense as a button. > I think we need to hide the browser and display a loading throbber until the > page has actually loaded, it just sits there blank for me for a while. Can be > done in a follow-up bug. Bug 556223. > The class "sorters" doesn't sound right, how about "sort-header"? Agreed. Went with "sort-controls" - since its not really a header, but a group of controls. > > <vbox class="list-container" flex="1"> > > <hbox class="search-filter"> > > <label value="&search.filter.label;"/> > > <checkbox id="search-filter-local" label="&search.filter.installed.label;" > > checked="true" disabled="true"/> > > <checkbox id="search-filter-remote" label="&search.filter.available.label;" > > checked="false" disabled="true"/> > > </hbox> > > Could this work better in the view-header above? No, its meant to scroll with the list items, rather than be static with the header. > > > <hbox id="detail-view" flex="1" class="view-pane" hidden="true"> > > <spacer flex="1"/> > > <hbox class="loading" flex="1"> > > <image/> > > <label value="&loading.label;"/> > > </hbox> > > <vbox class="detail-view-container" flex="8"> > > These flex values seem a bit weird. I'm not sure why the top and bottom spacers > are here and I think the loading and detail should just be in a deck. Or do we > even need to care? I've never seen what the loading bit looks like, this is all > likely fast enough that I don't think we need to worry about it for now. Yea, some explaination needed here (which I'll also add as comments). The flex="8" is there to balance out the spacers. The spacers are there to add whitespace to the sides at high resolutions (and take whitespace away from the middle). And the loading screen is there because it'd need to be added in eventually anyway - the "Loading..." message doesn't show unless the provider takes longer than 100ms. I'd like to eventually put these in a stack, so the loading screen can be (quickly) faded out if its been shown for a long enough time to be noticed, with the detail view faded in (same effect can be used in bug 556223). > None of the mockups show a rating here. The newer mockups do - it moved to here (directly below the description text), from further down the page (which, incidently, I hadn't removed). > On add-ons with little/no description this makes the buttons seem way off on > their own, not sure the best way to tackle that. Maybe the problem goes away > once we have screenshots working. See above. Screenshots do help. Not sure if its a full solution yet though - we can experiment with this later. > For add-ons that aren't installed in the profile I think we should add some > text here explaining that we won't check the add-on for updates or some such. Hmm, yes. Filed bug 556228, since it needs some UX input.

(In reply to comment #10) > File a bug to get the string bundle service into Services.jsm and then use that Bug 556230. > > cmd_findAllUpdates: { > > isEnabled: function() true, > > This should probably be disabled if there are no add-ons installed that can be > updated. Of course that can only be gotten asynchronously which kind of doesn't > really work for here. Oh well. Yea :\ > >function prettifyURL(aURL) { > > I don't like this, we need to come up with something else to display rather > than a part of the url. Perhaps just the hostname would work but I'm thinking > not displaying the URL at all would be better. The design has changed somewhat to use just a link labelled "Homepage" - I've updated that now. > > var size = Math.floor(Math.random() * 1024 * 1024 * 2); > > size = ("00000000000" + size).slice(-10); // HACK: nsIXULSortService doesn't do numerical sorting > > Really? Can you file a bug on this A.S.A.P. and CC enn, we'll get it fixed. Bug 557636. > > var item = this.get(aId); > > if (item) { > > item.hidden = false; > > this.node.suppressOnSelect = true; > > this.node.selectedItem = item; > > this.node.suppressOnSelect = false; > > Does selecting it programmatically really send a select event? That seems like > a bug. It does - and looking at the code in listbox.xml, that's by design. > Please file a follow-up bug on making sure we stay secure here. The discovery > pane should never show anything insecure or anything not on the same host as > the original URL. Bug 557698. > > calculateMatchScore: function(aStr, aQuery, aMultiplier) { > Has this algorithm and the magic numbers within come from somewhere? Might be > worth referencing if so. Please also put the magic numbers as suitably named > constants. No, it's of my own design - so will need some L10n eyes on it. Filed bug 557701. > For the future but I think we should be able to support displaying multiple > types in the same list. We're also going to get rid of the bootstrapped type so > that tweak will go away. I actually had that in there at one stage, but debated whether it was needed. Pretty easy to add in again, in the future. > > AddonManager.getAddonsByType(types, function(aAddonsList) { > > addons = aAddonsList; > > updateList(); > > }); > > > > AddonManager.getInstalls(installTypes, function(aInstallsList) { > > installs = aInstallsList; > > updateList(); > > gEventManager.registerForInstalls(this); > > }); > > Just nest these, I don't believe you gain anything in performance by trying to > run them at the same time. These were originally nested, but I wanted to get away from assuming certain characteristics of the providers. At the very least, it doesn't make it worse, and I think the code is more readable broken up like this.

(In reply to comment #12) > > <binding id="download-progress"> > > <content> > > <xul:stack flex="1"> > > <xul:hbox flex="1"> > > <xul:hbox class="start-cap"/> > > <xul:progressmeter anonid="progress" class="progress" flex="1" min="0" max="100"/> > > <xul:hbox class="end-cap"/> > > </xul:hbox> > > Are these caps here just for image purposes? Can we use > multiple-css-backgrounds to achieve the same thing? Yes, appearance only. Don't think multiple backgrounds on the same hbox will work here, because each cap needs to be independantly shown depending on the value of the progress meter. > > > <binding id="categories-list" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox"> > > <implementation> > > <!-- This needs to be overridden to allow the fancy animation while not allowing that item to be selected when hiding --> > > Can't you just make the item disabled when hiding it? I've done that for this case (so the hidden attribute can be used when an animation isn't ever wanted), but the override is still needed because disabled items are normally selectable (listbox.xml's _canUserSelect() doesn't take into account the disabled attribute). > > this._name.value = this.mInstall.name; //XXXunf is this always available? > > No it isn't. Fallback to the filename part of sourceURL if it is null I think. Ugh, really wish there was an immutable version of nsIURL. That's everything from the first round of reviews - ready for the next round now.

Beginning review from http://hg.mozilla.org/projects/addonsmgr/rev/e28ec4a0c096

Comment on attachment 440604 [details] extensions.xul Just nitty stuff here, this looks good. > <!-- main header --> > <hbox id="header"> > <label id="header-name"/> > <button id="header-link"/> > <spacer flex="1"/> > <hbox id="updates-container"> > <image class="spinner"/> > <label id="updates-nonefound" hidden="true" Capitalise the F > value="&updates.noneFound.label;"/> > <button id="updates-checknow" class="button-link" And the N > <!-- search view --> > <vbox id="search-view" flex="1" class="view-pane"> > <hbox class="view-header"> > <spacer flex="1"/> Just use pack="end" on the hbox rather than the spacer. > <!-- list view --> > <vbox id="list-view" flex="1" class="view-pane"> > <hbox class="view-header"> > <spacer flex="1"/> And again > <hbox id="list-sorters" class="sort-controls" sortby="name" > ascending="true"/> > </hbox> > <vbox class="list-container" flex="1"> > <vbox id="addon-list-empty" class="empty-list-notice" > flex="1" hidden="true"> > <spacer flex="1"/> > <label value="&listEmpty.installed.label;"/> > <button label="&listEmpty.link.label;" class="addon-control" > command="cmd_goToDiscoverPane"/> listEmpty.link.label doesn't seem right, how about listEmpty.button.label? > <hbox class="detail-basicinfo fade"> > <image id="detail-icon"/> > <vbox> > <label id="detail-name"/> > <label id="detail-creator" class="creator"/> > </vbox> > <spacer flex="1"/> > <label id="detail-dateupdated"/> > </hbox> > <hbox class="detail-desc"> > <!-- XXXunf placeholder --> Put a reference to a bug here > <vbox hidden="true" class="fade"> > <image id="detail-screenshot"/> > <label value="1 of 5"/> > <spacer flex="1"/> > </vbox> > <vbox flex="1"> > <description flex="1" id="detail-desc" class="fade"/> > <hbox> > <!-- XXXunf placeholder --> Ditto > <hbox class="detail-extra"> > <vbox class="detail-prefs" flex="1"> > <checkbox id="detail-autoupdate" checked="true" camelCase again > label="&detail.updateAutomatically.label;" > accesskey="&detail.updateAutomatically.accesskey;" > tooltiptext="&detail.updateAutomatically.tooltip;"/> > <button id="detail-findupdates" class="addon-control" And again, also double check I didn't miss any > <grid class="detail-meta fade" flex="1"> > <columns> > <column flex="1"/> > <column flex="2"/> > </columns> > <rows> > <row> > <label class="meta-label" > value="&detail.version.label;"/> > <label class="meta-value" id="detail-version"/> > </row> > <row> > <label class="meta-label" > value="&detail.updated.label;"/> > <label class="meta-value" id="detail-updatedate"/> > </row> > <row> > <label class="meta-label" > value="&detail.numberOfDownloads.label;"/> > <!-- XXXunf placeholder --> Bug reference

Comment on attachment 440605 [details] extensions.js Just basically nitty stuff again here. I want to look at this with fresh eyes first thing in the morning but I think this is basically an r+. > cmd_showItemAbout: { > isEnabled: function(aAddon) { > // XXXunf double check that this will work for items pending install > return !!aAddon; Either fix this or put a bug reference to fix it. If the add-on is pending install you should still be able to load the default about dialog, just not an add-on's custom one. > var updated = "000000000000000"; // HACK: nsIXULSortService doesn't do numerical sorting (bug 379745) > if (aObj.updateDate) > updated = (updated + aObj.updateDate.valueOf()).slice(-14); > item.setAttribute("dateUpdated", updated); > > var size = Math.floor(Math.random() * 1024 * 1024 * 2); > size = ("00000000000" + size).slice(-10); // HACK: nsIXULSortService doesn't do numerical sorting (bug 379745) > item.setAttribute("size", size); // XXXapi Please make sure there is a bug on file for both API and UI to fix this and reference the UI bug here. > show: function() { > gHeader.setName(gStrings.ext.GetStringFromName("header-discover")); > // load content only if we're not already showing something on AMO > // XXXunf should only be comparing hostname > if (this._browser.currentURI.spec.indexOf(this._browser.homePage) == -1) > this._browser.goHome(); Put a bug reference for this > onSortChanged: function(aSortBy, aAscending) { > var sortService = Cc["@mozilla.org/xul/xul-sort-service;1"] > .getService(Ci.nsIXULSortService); Commonly we put the . at the end of the first line and align the getService with the Cc. > onSortChanged: function(aSortBy, aAscending) { > var sortService = Cc["@mozilla.org/xul/xul-sort-service;1"] > .getService(Ci.nsIXULSortService); Ditto >var gDetailView = { > node: null, > _addon: null, > _loadingTimer: null, > > _notificationContainer: null, > _notificationText: null, > > initialize: function() { > this.node = document.getElementById("detail-view"); > > this._notificationContainer = document.getElementById("detail-notification"); > this._notificationText = document.getElementById("detail-notification-text"); > }, > > show: function(aAddonId) { > var self = this; > this.node.setAttribute("loading", true); > this._loadingTimer = setTimeout(function() { > self.node.setAttribute("loading-extended", true); > }, 100); Please put numbers like this as constants at the top of the file. Same for the numbers in the search scoring bits.

Comment on attachment 440606 [details] extensions.xml Will go through this again tomorrow, here are some comments on the first half of the file: > <!-- Rating - displays current/average rating, allows setting user rating --> > <binding id="rating"> > <content> > <xul:hbox anonid="star-container" class="stars"> > <xul:image class="star" > onmouseover="document.getBindingParent(this)._hover(1);" > onclick="document.getBindingParent(this).userRating = 1;"/> > <xul:image class="star" > onmouseover="document.getBindingParent(this)._hover(2);" > onclick="document.getBindingParent(this).userRating = 2;"/> > <xul:image class="star" > onmouseover="document.getBindingParent(this)._hover(3);" > onclick="document.getBindingParent(this).userRating = 3;"/> > <xul:image class="star" > onmouseover="document.getBindingParent(this)._hover(4);" > onclick="document.getBindingParent(this).userRating = 4;"/> > <xul:image class="star" > onmouseover="document.getBindingParent(this)._hover(5);" > onclick="document.getBindingParent(this).userRating = 5;"/> > </xul:hbox> Why the hbox, why not just put orient="horizontal" on the <content>? > <method name="_updateStars"> > <body><![CDATA[ > var stars = this._starContainer.children; > var rating = this[this.showRating + "Rating"]; > for (let i = 0; i < stars.length; i++) > stars[i].setAttribute("on", rating > (i + 0.5)); Decrement rating by 0.5 before the loop rather than adding to i every loop and add a comment for why it is needed. > ]]></body> > </method> > > <!-- Download progress - shows graphical progress of download and any > related status message. --> > <binding id="download-progress"> > <content> > <xul:stack flex="1"> > <xul:hbox flex="1"> > <xul:hbox class="start-cap"/> > <xul:progressmeter anonid="progress" class="progress" flex="1" > min="0" max="100"/> > <xul:hbox class="end-cap"/> > </xul:hbox> > <xul:hbox class="status-container"> > <xul:button anonid="pause" class="pause" > tooltiptext="&progress.pause.tooltip;"/> > <xul:spacer flex="1"/> > <!-- XXXunf placeholder --> Bug reference

The changes here over-write the previous changes in browser.js in the initial landing on the project branch - so the only change missing here is removing the original body of the BrowserOpenAddonsMgr() function (the whole function body is replaced).

Since there's no functional changes here, I think it makes more sense to keep testsuite/litmus stuff in specific bugs.

Comment on attachment 440655 [details] [diff] [review] browser.js This looks good to me, though please add a bug reference, but I am not a browser peer. Can you make a patch of all your browser changes and request review from sdwilsh on it please

Comment on attachment 440605 [details] extensions.js r+ with the previous comments

Comment on attachment 440606 [details] extensions.xml > </xul:hbox> > <xul:hbox class="status-container"> > <xul:hbox anonid="checking-update" hidden="true"> > <xul:image class="spinner"/> > <xul:label value="&addon.checkingforUpdates.label;"/> checkingForUpdates. I think I said it before but if you could spin through and check the capitalisation is right then that'd be great, otherwise this all looks good.

Changes in browser.

(In reply to comment #22) > Commonly we put the . at the end of the first line and align the getService > with the Cc. Bah! (In reply to comment #23) > > <!-- Rating - displays current/average rating, allows setting user rating --> > Why the hbox, why not just put orient="horizontal" on the <content>? Because you need an anonymous node to get its children. Without the anonymous node, you get children of the binding instead. Unless I'm missing something? > > > <method name="_updateStars"> > > <body><![CDATA[ > > var stars = this._starContainer.children; > > var rating = this[this.showRating + "Rating"]; > > for (let i = 0; i < stars.length; i++) > > stars[i].setAttribute("on", rating > (i + 0.5)); > > Decrement rating by 0.5 before the loop rather than adding to i every loop and > add a comment for why it is needed. This was poor-man's rounding - I've just explicitly rounded the rating at the start now.

(In reply to comment #30) > (In reply to comment #22) > > Commonly we put the . at the end of the first line and align the getService > > with the Cc. > > Bah! > > (In reply to comment #23) > > > <!-- Rating - displays current/average rating, allows setting user rating --> > > Why the hbox, why not just put orient="horizontal" on the <content>? > > Because you need an anonymous node to get its children. Without the anonymous > node, you get children of the binding instead. Unless I'm missing something? document.getAnonymousNodes(this) will return you a NodeList of the anonymous children. Should be all you need I think.

Comment on attachment 440937 [details] [diff] [review] Browser changes >+ // XXXunf need to implement switching to the relevant view - bug 560449 >+ switchToTabHavingURI("about:addons", true); I think we'd normally use a TODO comment here, not XXX >+function switchToTabHavingURI(aURI, aOpenNew) { Say, would you mind adding some java-doc style comments explaining what this method does please? r=sdwilsh

(In reply to comment #31) > document.getAnonymousNodes(this) will return you a NodeList of the anonymous > children. Should be all you need I think. Ah, useful! http://hg.mozilla.org/projects/addonsmgr/rev/e1e1582220fa (In reply to comment #32) > I think we'd normally use a TODO comment here, not XXX > > >+function switchToTabHavingURI(aURI, aOpenNew) { > Say, would you mind adding some java-doc style comments explaining what this > method does please? Done and done. http://hg.mozilla.org/projects/addonsmgr/rev/a9bafa5a3689

Eep, almost forgot this. Changes to about.js (for the addon about dialog) - removing all the RDF-related stuff (yay!), using the new API, some general cleanup, and fixing being opened from a tab.

Comment on attachment 441421 [details] [diff] [review] about.js Looks good, couple of minor things: > var extensionDescription = document.getElementById("extensionDescription"); >- if (description) >- extensionDescription.appendChild(document.createTextNode(description)); >+ if (addon.description) >+ extensionDescription.appendChild(document.createTextNode(addon.description)); Could you switch this to just using extensionDescription.textContent please. >+function appendContributors(aHeaderId, aNodeId, aContributors) { Seems a bit weird to use Contributors in the name when it covers Contributors and the other two. How about calling it just appendStrings or appendList or something? Rename the parameter appropriately. >+ var header = document.getElementById(aHeaderId); >+ var node = document.getElementById(aNodeId); >+ >+ if (!aContributors || aContributors.length == 0) { >+ header.hidden = true; >+ return; >+ } >+ >+ for (let i = 0; i < aContributors.length; i++) { >+ var label = document.createElement("label"); >+ label.setAttribute("value", aContributors[i]); >+ label.setAttribute("class", "contributor"); This class seems extraneous

Adds the about:addons redirect. Apparently, this will need a security review (though I'm still not clear on why) - would like to do that in a followup, so we can get this on trunk as soon as possible.

Comment on attachment 441645 [details] [diff] [review] Docshell changes Er, lets get the right person to review this.

Theme CSS. winstripe/pinstripe/gnomestripe all have exactly the same CSS (so far).

Comment on attachment 441649 [details] extensions.properties >aboutWindowTitle=About %S >aboutWindowCloseButton=Close >aboutWindowVersionString=version %S >aboutAddon=About %S Add localization notes for these please. >header-search=Search Results >header-discover=Get Add-ons >header-language=Languages >header-searchengine=Search Engines >header-extension=Extensions >header-theme=Themes >header-plugin=Plugins > >#LOCALIZATION NOTE (header-goBack) %S is the name of the pane to go back to >header-goBack=Back to %S > >#LOCALIZATION NOTE (uninstallNotice) %S is the add-on name >uninstallNotice=The Add-on %S has been removed. > >numReviews=#1 review;#1 reviews > >dateUpdated=Updated %S Add notes for both of these please. >#LOCALIZATION NOTE (incompatibleWith) first %S is brand name, second %S is application version >incompatibleWith=Incompatible with %S %S > >incompatibleTitle2=Incompatible Add-on >#LOCALIZATION NOTE (incompatibleMessage) first %S is add-on name, second is add-on version, third is application name, fourth is application version >incompatibleMessage=%S %S could not be installed because it is not compatible with %S %S. > >installDownloading=Downloading >installDownloaded=Downloaded >installDownloadFailed=Error downloading >installVerifying=Verifying >installInstalling=Installing >installInstallPending=Add-on ready to install I think just "Ready to install" >installUpdatePending=Add-on ready to update "Ready to update" >installFailed=Error installing >installCancelled=Install cancelled > >#LOCALIZATION NOTE: First %S is the addon name, second %S is brand name >restartToEnable=The add-on %S will be enabled after you restart %S >restartToDisable=The add-on %S will be disabled after you restart %S >restartToUninstall=The add-on %S will be removed after you restart %S >restartToUpgrade=The add-on %S will be updated after you restart %S I kind of think that "The add-on" is not useful here, Boriss?

(In reply to comment #41) Rob has also suggested that it would be clearer to use the full numeric %S syntax on strings with more than one %S.

Comment on attachment 441648 [details] extensions.dtd ><!ENTITY addons.title "Add-ons Manager"> ><!ENTITY search.placeholder "Search all add-ons"> ><!ENTITY loading.label "Loadingâ¦"> This is probably bugzilla screwing up but make sure your file is UTF-8 encoded. ><!-- categories / views --> ><!ENTITY view.search.label "Search"> ><!ENTITY view.discover.label "Get Add-ons"> ><!ENTITY view.languages.label "Languages"> ><!ENTITY view.searchengines.label "Search Engines"> ><!ENTITY view.features.label "Extensions"> ><!ENTITY view.appearance.label "Themes"> ><!ENTITY view.plugins.label "Plugins"> Can you add a note that these strings should match those in the properties, and do the same in the properties file. ><!-- addon updates --> ><!ENTITY updates.updateNow.label "Update Add-ons"> ><!ENTITY updates.updating.label "Updating Add-ons"> ><!ENTITY updates.installed.label "Add-ons have been updated!"> ><!ENTITY updates.downloaded.label "Add-on updates have been downloaded!"> I don't think the exclamation mark is necessary, Boriss? ><!ENTITY cmd.contribute.label "Contribute"> ><!ENTITY cmd.contribute.accesskey "C"> ><!ENTITY cmd.contribute.tooltip "Say thanks to this add-on's developer by sending them money"> How about "Contribute to the development of this add-on". Maybe see what the add-ons team think is best.

(In reply to comment #42) > (In reply to comment #41) > Rob has also suggested that it would be clearer to use the full numeric %S > syntax on strings with more than one %S. For example, %1$S %2$S

Comment on attachment 441650 [details] theme extensions.css I want to see this again based on the comments below and these general points: There are lots of colours here that are precisely specified. How many of them need to be and how many of them can you use CSS system colours for? One thing we used to have was some rules in the content css determining which parts of hte UI to display and hide based on add-on state etc. Do you think it would make sense to move some of your rules like that here into the content? This basically reduced the breakage of custom themes as we made changes to that sort of thing. >.pane-splitter { > -moz-appearance: none; > border: none; > -moz-border-start: 1px solid #A8A8A8; > background-color: transparent; > background-image: none; > min-width: 1px; Is this really needed? >#categories > richlistitem { Give the richlistitems a custom class and just select on that. > border: none; > -moz-appearance: none; > font-size: 18px; Please use relative font sizes, here and elsewhere > padding: 10px 4px; > -moz-box-align: center; > overflow: hidden; > min-height: 0px; Is this needed? >#categories > richlistitem[disabled] { > height: 0px; > opacity: 0; > -moz-transition-property: height, opacity; > -moz-transition-duration: 1s, 0.8s; >} > >#categories > richlistitem:not([disabled]) { > height: 52px; This seems awfully specific, is it necessary? Can you use "auto"? >#categories > richlistitem > .badge { Are there other badges in the UI? If not just use .badge as the selector. If so use something more specific like .category-badge >#categories > richlistitem > .icon { .category-icon >/*** header ***/ > >#header { > background: -moz-linear-gradient(top, #EEE, #AAA); > padding: 8px; > margin: 0px; > -moz-box-align: center; > height: 50px; Use a font size relative unit here. >#header > #header-link { Just #header-link. If you have multiple elements with ID header-link then you have other problems you need to fix. >#header > #header-link > .button-box > .button-icon { How about just "#header-link .button-icon" >.sort-controls > .sorter { Again, is .sorter used elsewhere? If not the complicated selector is redundant >.sort-controls > .sorter > .button-box > .button-icon { .sorter .button-icon >.list-container { > overflow: auto; >} > >.list { > -moz-appearance: none; > margin: 0px; > border: none; >} > >.list > richlistitem[status="installed"], .list > richlistitem[status="installing"] { Give these richlistitems a custom class and just use that. >.list > richlistitem .extra-details { > height: 0px; > overflow: hidden; > opacity: 0; > text-align: right; Use "text-align: end" (or something like that, ask me again if you can't find it) rather than needing the locale-dir stuff >.list > richlistitem .icon { > margin: 7px 5px; > width: 32px; > height: 32px; > list-style-image: url("chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png") >} > >.list > richlistitem[type="theme"] .icon { > list-style-image: url("chrome://mozapps/skin/extensions/themeGeneric.png"); >} We should have a special icon for plugins here too. >.download-progress { > -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#download-progress"); This should be in the content CSS >button.addon-control { Do you use addon-control on other elements? If not button is redundant.

(In reply to comment #41) > >restartToEnable=The add-on %S will be enabled after you restart %S > >restartToDisable=The add-on %S will be disabled after you restart %S > >restartToUninstall=The add-on %S will be removed after you restart %S > >restartToUpgrade=The add-on %S will be updated after you restart %S > > I kind of think that "The add-on" is not useful here, Boriss? Hmm, I'm 50/50 on that. A bad addon name could potentially make that pretty confusing (eg, "Firefox", "Windows"). See also the uninstallNotice string ("The Add-on %S has been removed."). (In reply to comment #44) > (In reply to comment #42) > > (In reply to comment #41) > > Rob has also suggested that it would be clearer to use the full numeric %S > > syntax on strings with more than one %S. > For example, %1$S %2$S Ah! Forgot all about that - thanks. (In reply to comment #43) > This is probably bugzilla screwing up but make sure your file is UTF-8 encoded. Confirmed: bugzilla's fault. > ><!ENTITY cmd.contribute.tooltip "Say thanks to this add-on's developer by sending them money"> > > How about "Contribute to the development of this add-on". Maybe see what the > add-ons team think is best. I've added a note to bring it up in this weeks' meeting. http://hg.mozilla.org/projects/addonsmgr/rev/0ab7a03b468b

(In reply to comment #45) > (From update of attachment 441650 [details]) > There are lots of colours here that are precisely specified. How many of them > need to be and how many of them can you use CSS system colours for? Given that the visual style isn't finalized (and will most likely change), I'd like to leave those for now. > One thing we used to have was some rules in the content css determining which > parts of hte UI to display and hide based on add-on state etc. Do you think it > would make sense to move some of your rules like that here into the content? > This basically reduced the breakage of custom themes as we made changes to that > sort of thing. Good idea - there was only a few things, but I've moved them to content. > >.pane-splitter { > > -moz-appearance: none; > > border: none; > > -moz-border-start: 1px solid #A8A8A8; > > background-color: transparent; > > background-image: none; > > min-width: 1px; > > Is this really needed? The various overrides or the splitter itself? The overrides are needed, while keeping or removing the splitter is bug 553469. > > padding: 10px 4px; > > -moz-box-align: center; > > overflow: hidden; > > min-height: 0px; > > Is this needed? Yes, needed for the show/hide animation. > >#categories > richlistitem:not([disabled]) { > > height: 52px; > > This seems awfully specific, is it necessary? Can you use "auto"? Sadly, this is also needed for the show/hide animation. http://hg.mozilla.org/projects/addonsmgr/rev/f604d787d6d0

Comment on attachment 442103 [details] content extensions.css >#categories { > -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#categories-list"); >} > >.category { > -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#category"); >} > >.sort-controls { > -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#sorters"); > -moz-appearance: none; -moz-appearance should be in the themes stylesheet normally.

Comment on attachment 442102 [details] theme extensions.css Can you file a bug on switching to system colours once the theme is more finalized please.

>uninstallNotice=The Add-on %S has been removed. Should be "The add-on %S has been removed," for consistency with otherlanguage in Firefox. Note about capitalization in strings: "Add-on" should be capitalized only if it's the start of a phrase, or in the name of a category ("Get Add-ons" and "Add-ons Manager")). This is most consistent with current Firefox strings. Following the above would mean a few tweaks: (In reply to comment #43) ><!ENTITY updates.updateNow.label "Update Add-ons"> ><!ENTITY updates.updating.label "Updating Add-ons"> Should be "Update add-ons" and "Updating add-ons" A few other tweaks: (In reply to comment #41) > >#LOCALIZATION NOTE: First %S is the addon name, second %S is brand name > >restartToEnable=The add-on %S will be enabled after you restart %S > >restartToDisable=The add-on %S will be disabled after you restart %S > >restartToUninstall=The add-on %S will be removed after you restart %S > >restartToUpgrade=The add-on %S will be updated after you restart %S > > I kind of think that "The add-on" is not useful here, Boriss? I agree - let's just write %S for all the strings here, including: "%S has been removed." "%S has been disabled." "%S will be enabled after you restart." (in reply to comment #43) > ><!ENTITY updates.installed.label "Add-ons have been updated!"> > ><!ENTITY updates.downloaded.label "Add-on updates have been downloaded!"> > I don't think the exclamation mark is necessary, Boriss? I agree, let's remove it. I'll go ahead & make the above changes.

> ><!ENTITY updates.downloaded.label "Your add-on updates have been downloaded."> > ><!ENTITY updates.restart.label "Restart"> Unfocused, is updates.restart.label here what would appear after the notification string "Your add-on updates have been downloaded?" If so, it should probably be changed to "Restart now to complete installation"

string tweaks (see above)

Comment on attachment 442202 [details] extensions.dtd Already has my review though there are bits I asked for in comment 43 that Blair still needs to add. Also I noticed that the strings don't line up so lets get that fixed too.

(In reply to comment #57) > (From update of attachment 442202 [details]) > Already has my review though there are bits I asked for in comment 43 that > Blair still needs to add. Also I noticed that the strings don't line up so lets > get that fixed too. Should line up now

Ugh, its difficult to keep track of all this.

(In reply to comment #55) > > ><!ENTITY updates.downloaded.label "Your add-on updates have been downloaded."> > > ><!ENTITY updates.restart.label "Restart"> > > Unfocused, is updates.restart.label here what would appear after the > notification string "Your add-on updates have been downloaded?" If so, it > should probably be changed to "Restart now to complete installation" Yes - done. (In reply to comment #57) > (From update of attachment 442202 [details]) > Already has my review though there are bits I asked for in comment 43 that > Blair still needs to add. Yes, Boriss's changes were based on the old attached versions, not the latest in the repo. I've merged them. http://hg.mozilla.org/projects/addonsmgr/rev/5c03150c158f

(In reply to comment #50) > -moz-appearance should be in the themes stylesheet normally. Oops. http://hg.mozilla.org/projects/addonsmgr/rev/e1bdc4f12b80

(In reply to comment #35) > >+ label.setAttribute("class", "contributor"); > > This class seems extraneous It's used in theme CSS. http://hg.mozilla.org/projects/addonsmgr/rev/3c928ab347ec

Comment on attachment 442288 [details] [diff] [review] firefox.js r=sdwilsh

Comment on attachment 441645 [details] [diff] [review] Docshell changes r=bzbarsky

Comment on attachment 442102 [details] theme extensions.css >.list-container { > overflow: auto; >} Just interested in how it looks by default in Modern; awful, because you use CSS in the skin to get the scrolling to work. >.list { > -moz-appearance: none; > margin: 0px; > border: none; >} While I was here, note that we have a class="plain" that does this.

I don't now whether this is the right place for feedback for about:addons, please tell me if it isn't. First of all, I like it, that the addon manager was outsourced as a "normal" page. But some things aren't that nice yet: 1. If I click on "Get Addons" and it gets this blue background gradient, it seems strange, that the grey title bar is two pixels (estimated) greater. Imho it would create a much nicer effect, if the title bar (i'm refering to the one with the search, a.s.o.) were same height as one item in the navi, i.e. two pixels smaller. 2. When I click on "Update add-ons" the message switches to a non-link " Updating add-ons". It would be nice to have a stronger indicator, that firefox is actually doing something, for example this rotating circle often used to show that something's loading. 3. If I switch to "Extension" I see five installed addons. The strange thing about this is, that the addons only get displayed in part of the window even though there is still space underneath. This creates a scroll-bar, which isn't nice. 4. It would be nice to switch the "Remove" and "Disable" buttons (so it get's "Disable" "Remove".) This way the remove-buttons will always be in one row. (Right now it is not this way, because some add-ons are already disables and therefore the "Disable" button is missing, so the remove-button takes the place of the disable-button.) 5. Under the rating there's a date. Is this the last update date? If so, please make this clearer. (Furthermore all my Plugins have an "Unknown" there, which is irritating, because one doesn't even know, what is unknown, actually.) 6. Do not show the "Show more" button, if it would take ad much place as the information itself (only the size, for example). (Or is this intended to hide information, that the end user doesn't really care about? If so, the update-date (if it really is the update date) should be hidden, too.) 7. The text of the contribute button should be centered (it is right-aligned right now, at least it looks like that.) 8. If removing an addon in the addon list the yellow message shows "Undo? Restart now". If removing from the details-screen, it shows "Restart now Undo?" 9. If I click "Contribute" or "Check for updates" (in details) nothing happens. (But I expect, that this easily isn't yet implemented.) Hope this helps you. (If you can't understand some of the points, I can provide screen shots explaining what I want to say :)

(In reply to comment #68) > I don't now whether this is the right place for feedback for about:addons, > please tell me if it isn't. This bug is for the initial landing of that feature. Any other bug or feature request should be covered in its own bug. > 1. If I click on "Get Addons" and it gets this blue background gradient, it > seems strange, that the grey title bar is two pixels (estimated) greater. Imho > it would create a much nicer effect, if the title bar (i'm refering to the one > with the search, a.s.o.) were same height as one item in the navi, i.e. two > pixels smaller. Yes, you should file it as a new bug. > 2. When I click on "Update add-ons" the message switches to a non-link " > Updating add-ons". It would be nice to have a stronger indicator, that firefox > is actually doing something, for example this rotating circle often used to > show that something's loading. Something we can cover in bug bug 562622. Please check its design mockups and comment there. > 3. If I switch to "Extension" I see five installed addons. The strange thing > about this is, that the addons only get displayed in part of the window even > though there is still space underneath. This creates a scroll-bar, which isn't > nice. Bug 562885 > 4. It would be nice to switch the "Remove" and "Disable" buttons (so it get's > "Disable" "Remove".) This way the remove-buttons will always be in one row. > (Right now it is not this way, because some add-ons are already disables and > therefore the "Disable" button is missing, so the remove-button takes the place of the disable-button.) Please file a bug on that. > 5. Under the rating there's a date. Is this the last update date? If so, please > make this clearer. (Furthermore all my Plugins have an "Unknown" there, which > is irritating, because one doesn't even know, what is unknown, actually.) Bug 562239. > 6. Do not show the "Show more" button, if it would take ad much place as the > information itself (only the size, for example). (Or is this intended to hide > information, that the end user doesn't really care about? If so, the > update-date (if it really is the update date) should be hidden, too.) Bug 562800. > 7. The text of the contribute button should be centered (it is right-aligned > right now, at least it looks like that.) See bug 562902. > 8. If removing an addon in the addon list the yellow message shows "Undo? > Restart now". If removing from the details-screen, it shows "Restart now Undo?" Should also be covered by the above bug. > 9. If I click "Contribute" or "Check for updates" (in details) nothing happens. > (But I expect, that this easily isn't yet implemented.) Yes, those buttons are only placeholders at the moment. > Hope this helps you. Thanks for your feedback. In general please check our testplan and the top issues section: https://wiki.mozilla.org/QA/Firefox_3.next/Test_Plan:AddonsManagerRedesign#Top_Issues

With the new design, there is no way to 'Use Theme'.

Created Bug 562919 - With the new addons UI, there is no way to use a theme.

(In reply to comment #67) > (From update of attachment 442102 [details]) > >.list-container { > > overflow: auto; > >} > Just interested in how it looks by default in Modern; awful, because you use > CSS in the skin to get the scrolling to work. For things like that, we really should have some CSS in content.

Oh, and for some fun, read the L10n notes in those lines and compare the first strings after each of those: http://hg.mozilla.org/mozilla-central/file/4eb27434a297/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties#l9 http://hg.mozilla.org/mozilla-central/file/4eb27434a297/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd#l10 Could we please have the original strings follow the same advice as is given for the localizers? ;-)

Filed two new bugs: Bug 562949 (concerning height) Bug 562954 (concerning remove/disable) Does anyone know, whether were already is a bug on the fact, that one cannot select text in about:addons? (Couldn't find one.) Thanks Henrik for your patience.

http://hg.mozilla.org/mozilla-central/rev/c44d6a3f139a (browser) http://hg.mozilla.org/mozilla-central/rev/90c46ddd6c5f (docshell) http://hg.mozilla.org/mozilla-central/rev/c89f7b6ebc56 (preferences) http://hg.mozilla.org/mozilla-central/rev/eb4b899de5f1 (strings) http://hg.mozilla.org/mozilla-central/rev/3c6862a3b12f (UI) http://hg.mozilla.org/mozilla-central/rev/e3b94f3e9baa (theme)

Everything has been landed. Marking as verified fixed.