Blocking-P4; definitely wanted from a product perspective, and want to invest effort, but until we get better sizing and understanding of time required, we can't make this a higher priority in terms of blocking status.

Notes from security review: Need to be able to disable the integration pane through preferences, also to provide support for switching browser online if it is offline. Must check that the learn more link returned from AMO is a http/s url.

Also should pass xpinstall the name and icon for pretty displaying in the install confirmation dialog.

Need to decide what to do about apps not wanting the integration pane but still wanting a link to their add-ons site.

Currently add-on retrieval through the API is basically implemented, as is installing add-ons. Still need to implement comments 2, 3 and manage the display in the pane of the install progress. Main parts remaining are UI work.

--> P2, thanks Dave.

This is the bulk of the work to implement this pane, the other UI parts are spread across the bug dependencies. While this part is working we should hold off on landing until bug 400715 and bug 411650 are resolved. The pane works in a similar style to the plugins pane, I've added an additional datasource that search results are put into for display. In the proposed UI we have additional items within the richlistbox that are not list items. As such I have made a change to the view template builder. This allows us to specify different sets of binding for different elements types, other panes behave the same as previously however in the new pane the template will create a richlistitem for anything with searchResult=true and a vbox for anything with statusMessage=true. Then the status messages have xbl bindings similarly to how the add-on items do. The system is set up to support pluggable sources of add-ons through the nsIAddonRepository interface. The default one provided pulls data from AMO through the new APIs. This is an asynchronous interface so we provide callback objects to receive the data which then push it into the new datasource. In the new pane there are thumbnail images for each add-on. The images we get from AMO are of varying sizes so the binding on initialisation starts to load the image in the background then once loaded works out a standard size to display it at. Some add-ons have an EULA so there is a small dialog implemented to display that for user confirmation.

Bugzilla seems none too keen on hg patches, here is the same against CVS

These are the updated images associated with the patch Oh and ignore the eula.css in gnomestripe in the patch above, that isn't meant to be there.

Comment on attachment 297146 [details] binary files jar:https://bugzilla.mozilla.org/attachment.cgi?id=297146!/toolkit/themes/winstripe/mozapps/extensions/missingThumbnail.png This is bad for i18n. I guess all images are temporary, but we shouldn't even start with such things.

Yep sure, I should have added that all of the images are placeholders awaiting new ones that apparently faaborg is sourcing.

I'm not quite finished... just wanted to ask you to try to some up with a better name for the prefs. >Index: browser/app/profile/firefox.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v >retrieving revision 1.252 >diff -u -8 -p -r1.252 firefox.js >--- browser/app/profile/firefox.js 12 Jan 2008 22:22:03 -0000 1.252 >+++ browser/app/profile/firefox.js 15 Jan 2008 09:47:48 -0000 >@@ -63,16 +63,25 @@ pref("xpinstall.dialog.progress.type.chr > // their extensions are being updated and thus reregistered every time the app is > // started. > pref("extensions.ignoreMTimeChanges", false); > // Enables some extra Extension System Logging (can reduce performance) > pref("extensions.logging.enabled", false); > // Hides the install button in the add-ons mgr > pref("extensions.hideInstallButton", true); > >+// Preferences for the Get Add-ons pane >+pref("extensions.search.showPane", true); >+pref("extensions.search.browseAddons", "https://addons.mozilla.org/%LOCALE%/%APP%"); >+pref("extensions.search.maxResults", 5); >+pref("extensions.search.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended"); >+pref("extensions.search.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/list/featured/all/10"); >+pref("extensions.search.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%"); >+pref("extensions.search.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/search/%TERMS%"); Perhaps something along the lines of extensions.getaddons.* or something besides search.search and >Index: toolkit/mozapps/extensions/content/extensions.js >=================================================================== >... >+ var urlproperties = [ "iconURL", "homepageURL", "thumbnailURL", "xpiURL" ]; >+ var properties = [ "name", "eula", "iconURL", "homepageURL", "thumbnailURL", "xpiURL", "xpiHash" ]; >+ for (var i = 0; i < addons.length; i++) { >+ var addon = addons[i]; >+ // Strip out any items with potentially unsafe urls >+ var unsafe = false; >+ for (var j = 0; j < urlproperties.length; j++) { >+ if (!isSafeURI(addon[urlproperties[j]])) { Is there a reason to restrict xpiURL so it can't use ftp?

(In reply to comment #13) > Perhaps something along the lines of extensions.getaddons.* or something > besides search.search I considered getaddons at first but discarded it for some reason. Not sure I have a problem with it now, maybe some alternatives are: remoteAddons addonsRepository > >+ for (var j = 0; j < urlproperties.length; j++) { > >+ if (!isSafeURI(addon[urlproperties[j]])) { > Is there a reason to restrict xpiURL so it can't use ftp? I don't think so. The current set of urls being tested in this function is: iconURL homepageURL thumbnailURL xpiURL availableUpdateInfo From my estimation there isn't any need to restrict any of those from ftp so I think this is just a simple change to the isSafeURI function.

Comment on attachment 297140 [details] [diff] [review] patch rev 1 (against cvs) In addition to previous comments. note: a diff -w would be appreciated in addition to a regular diff in the future for patches with lots of whitespace changes like this one. >Index: toolkit/mozapps/extensions/content/extensions.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v >retrieving revision 1.153 >diff -u -8 -p -r1.153 extensions.js >--- toolkit/mozapps/extensions/content/extensions.js 3 Dec 2007 12:24:41 -0000 1.153 >+++ toolkit/mozapps/extensions/content/extensions.js 15 Jan 2008 09:48:23 -0000 >... >@@ -521,45 +560,305 @@ function setRestartMessage(aItem) > item.setAttribute("description", item.getAttribute("oldDescription")); > item.removeAttribute("oldDescription"); > } > } > aItem.setAttribute("oldDescription", aItem.getAttribute("description")); > aItem.setAttribute("description", restartMessage); > } > >+// Removes any assertions in the datasource about a given resource >+function cleanResource(ds, resource) >+{ >+ // Remove outward arcs >+ var arcs = ds.ArcLabelsOut(resource); >+ while (arcs.hasMoreElements()) { >+ var arc = arcs.getNext().QueryInterface(Components.interfaces.nsIRDFResource); >+ var targets = ds.GetTargets(resource, arc, true); >+ while (targets.hasMoreElements()) { >+ var value = targets.getNext().QueryInterface(Components.interfaces.nsIRDFNode); >+ if (value) >+ ds.Unassert(resource, arc, value); >+ } >+ } >+} >+ >+// Wipes the datasource clean of assertions >+function cleanDataSource(ds, rootctr) >+{ >+ // Remove old entries from the list >+ var nodes = rootctr.GetElements(); >+ while (nodes.hasMoreElements()) { >+ var node = nodes.getNext() >+ .QueryInterface(Components.interfaces.nsIRDFResource); >+ rootctr.RemoveElement(node, false); >+ cleanResource(ds, node); >+ } >+} >+ >+// Displays the search status message >+function displaySearchThrobber(aKey) { Stick with one style for curly braces (same line as function as immediately above) on function declarations in this patch... the rest can be cleaned up later. >+ var rdfCU = Components.classes["@mozilla.org/rdf/container-utils;1"] >+ .getService(Components.interfaces.nsIRDFContainerUtils); >+ var rootctr = rdfCU.MakeSeq(gSearchDS, gRDF.GetResource(RDFURI_ITEM_ROOT)); >+ >+ cleanDataSource(gSearchDS, rootctr); >+ >+ var labelNode = gRDF.GetResource("urn:mozilla:addons:search:status:header"); >+ rootctr.AppendElement(labelNode); >+ gSearchDS.Assert(labelNode, >+ gRDF.GetResource(PREFIX_NS_EM + "statusMessage"), >+ gRDF.GetLiteral("true"), >+ true); >+ gSearchDS.Assert(labelNode, >+ gRDF.GetResource(PREFIX_NS_EM + "type"), >+ gRDF.GetLiteral(aKey), >+ true); >+} >+ >+// Clears the search box and updates the result list >+function clearSearch() { nit: I'll leave this to you to decide but this name doesn't seem descriptive since it clears the search"box" and then performs a search >+ var searchbox = document.getElementById("searchbox"); >+ searchbox.value = ""; >+ performSearch(); >+} >+ >+// Searches for results >+function performSearch() { nit: same as above since this can also cancel a search? >@@ -765,32 +1071,36 @@ function Startup() >... > function Shutdown() > { >+ if (gAddonRepository && gAddonRepository.searching) >+ gAddonRepository.cancelSearch(); >+ Set gRDF to null? This is from habit and may no longer be necessary. >@@ -827,17 +1137,16 @@ XPInstallDownloadManager.prototype = { > _statusFormatKBMB : null, > _statusFormatKBKB : null, > _statusFormatMBMB : null, > > observe: function (aSubject, aTopic, aData) > { > switch (aTopic) { > case "xpinstall-download-started": >- showView("installs"); Does this break displaying the installation view if the EM window, initiates an install with the EM window already open with this change, etc.? >@@ -1817,16 +2128,44 @@ function confirmOperation(aName, aTitle, > for (var i = 0; i < aDependantItems.length; ++i) > names.push(aDependantItems[i].name + " " + aDependantItems[i].version); > > window.openDialog("chrome://mozapps/content/extensions/list.xul", "", > "titlebar,modal,centerscreen", names, params); > return params.result == "accept"; > } > >Index: toolkit/mozapps/extensions/content/extensions.xml >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v >retrieving revision 1.50 >diff -u -8 -p -r1.50 extensions.xml >--- toolkit/mozapps/extensions/content/extensions.xml 26 Oct 2007 18:07:09 -0000 1.50 >+++ toolkit/mozapps/extensions/content/extensions.xml 15 Jan 2008 09:48:24 -0000 >@@ -46,17 +46,18 @@ > <!ENTITY % extensionsDTD SYSTEM "chrome://mozapps/locale/extensions/extensions.dtd" > > %brandDTD; > %extensionsDTD; > ]> > > <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"> >+ xmlns:xbl="http://www.mozilla.org/xbl" >+ xmlns:html="http://www.w3.org/1999/xhtml"> Where is the html namespace being used? >Index: toolkit/themes/gnomestripe/mozapps/extensions/eula.css >=================================================================== >RCS file: toolkit/themes/gnomestripe/mozapps/extensions/eula.css >diff -N toolkit/themes/gnomestripe/mozapps/extensions/eula.css >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/themes/gnomestripe/mozapps/extensions/eula.css 15 Jan 2008 09:48:29 -0000 >@@ -0,0 +1,16 @@ >+#heading { >+ font-size: 120%; >+} >+ >+#scrollbox { >+ overflow-y: auto; >+ background-color: window; >+ border: 1px solid black; nit: can this use a system color instead? >+ margin: 1em 0 1em 0; >+ padding: 5px; >+} >+ >+#eula { >+ font-family: monospace; >+ white-space: -moz-pre-wrap; >+} >\ No newline at end of file nit: add a newline... other files as well >Index: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v >retrieving revision 1.1 >diff -u -8 -p -r1.1 extensions.css >--- toolkit/themes/gnomestripe/mozapps/extensions/extensions.css 21 Dec 2007 11:17:02 -0000 1.1 >+++ toolkit/themes/gnomestripe/mozapps/extensions/extensions.css 15 Jan 2008 09:48:29 -0000 >@@ -259,16 +259,144 @@ richlistitem[loading="true"] .updateBadg > font-weight: normal; > border: none; > } > > richlistitem[opType="needs-uninstall"] .notifyBadge { > display: none; > } > >+.addon-search-details { >+ margin: 5px 0 5px 6px; >+} >+ >+.addonThumbnailContainer { >+ background: white; >+ padding: 5px; >+ border: 2px black solid; nit: can this use a system color instead? Elsewhere in the patch as well. >... >+.searchbox-search, .searchbox-cancel { >+ -moz-appearance: none; >+ cursor: pointer; >+ margin: 0; >+ border: 0; >+ padding: 0; >+ width: 19px; >+ height: 19px; >+ min-width: 19px; >+ max-width: 19px; >+ min-height: 19px; >+ max-height: 19px; >+} Why? Elsewhere as well... It looks pretty good and shouldn't take long to review next time around

(In reply to comment #13) > Perhaps something along the lines of extensions.getaddons.* or something > besides search.search Gone for extensions.getAddons... > Is there a reason to restrict xpiURL so it can't use ftp? Allowed ftp for all the various url checks. (In reply to comment #15) > Stick with one style for curly braces (same line as function as immediately > above) on function declarations in this patch... the rest can be cleaned up > later. Done everywhere I think. > >+function clearSearch() { > nit: I'll leave this to you to decide but this name doesn't seem descriptive > since it clears the search"box" and then performs a search Changed to resetSearch which seems more appropriate. > >+// Searches for results > >+function performSearch() { > nit: same as above since this can also cancel a search? Changed this to retrieveRepositoryAddons which is more explicit about what it does. > Set gRDF to null? This is from habit and may no longer be necessary. Done > >- showView("installs"); > Does this break displaying the installation view if the EM window, initiates an > install with the EM window already open with this change, etc.? Yes it did. One of the goals is to not change panes when initiating an install from the Get Add-ons pane. I've implemented this by keeping a track of the xpi's getting installed from there and only changing panes if we see a url not in that list. > >+ xmlns:html="http://www.w3.org/1999/xhtml"> > Where is the html namespace being used? Good catch, left over from an earlier revision. > >+ border: 1px solid black; > nit: can this use a system color instead? Used ActiveBorder > >\ No newline at end of file > nit: add a newline... other files as well Done > >+ border: 2px black solid; > nit: can this use a system color instead? Elsewhere in the patch as well. ActiveBorder again > >+.searchbox-search, .searchbox-cancel { > >+ -moz-appearance: none; > >+ cursor: pointer; > >+ margin: 0; > >+ border: 0; > >+ padding: 0; > >+ width: 19px; > >+ height: 19px; > >+ min-width: 19px; > >+ max-width: 19px; > >+ min-height: 19px; > >+ max-height: 19px; > >+} > Why? Elsewhere as well... Ok, narrowed this down a bit. In pinstripe we need min-width and min-height because the default style for a button has definitions for them. In winstripe and gnomestripe they only have min-width set so that is all that needs ot be overridden so updated the patch to do that. A couple of other changes I felt it necessary to make in this patch: Changed the margins to use -moz-margin-start/end where appropriate. Tweaked the loading of thumbnail loads to stop errors appearing in the console. Ratings coming from AMO are actually 0-10 not 0-5 so adjusted to accommodate (only a style change). Removed that unlocalisable image and replaced with localised text. Made the preview default to 125x94 (this is the same ratio as the majority of preview images on AMO), but allowed it to expand vertically if necessary.

Comment on attachment 298024 [details] [diff] [review] patch rev 2 Will finish the remainder later. >Index: browser/app/profile/firefox.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v >retrieving revision 1.252 >diff -u -8 -p -w -r1.252 firefox.js >--- browser/app/profile/firefox.js 12 Jan 2008 22:22:03 -0000 1.252 >+++ browser/app/profile/firefox.js 19 Jan 2008 22:46:22 -0000 >@@ -63,16 +63,25 @@ pref("xpinstall.dialog.progress.type.chr > // their extensions are being updated and thus reregistered every time the app is > // started. > pref("extensions.ignoreMTimeChanges", false); > // Enables some extra Extension System Logging (can reduce performance) > pref("extensions.logging.enabled", false); > // Hides the install button in the add-ons mgr > pref("extensions.hideInstallButton", true); > >+// Preferences for the Get Add-ons pane >+pref("extensions.getAddons.showPane", true); >+pref("extensions.getAddons.browseAddons", "https://addons.mozilla.org/%LOCALE%/%APP%"); >+pref("extensions.getAddons.maxResults", 5); >+pref("extensions.getAddons.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended"); >+pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/list/featured/all/10"); >+pref("extensions.getAddons.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%"); >+pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/search/%TERMS%"); Have these urls been confirmed as correct? >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v >retrieving revision 1.28 >diff -u -8 -p -w -r1.28 extensions.dtd >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd 27 Nov 2007 02:44:50 -0000 1.28 >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd 19 Jan 2008 22:46:50 -0000 >@@ -112,16 +116,37 @@ > <!ENTITY toBeEnabled.label "This add-on will be enabled when &brandShortName; is restarted."> > <!ENTITY toBeInstalled.label "This add-on will be installed when &brandShortName; is restarted."> > <!ENTITY toBeUninstalled.label "This add-on will be uninstalled when &brandShortName; is restarted."> > <!ENTITY toBeUpdated.label "This add-on will be updated when &brandShortName; is restarted."> > > <!ENTITY getExtensions.label "Get Extensions"> > <!ENTITY getThemes.label "Get Themes"> > <!ENTITY getPlugins.label "Get Plugins"> >+ >+<!ENTITY searchAddons.label "Search All Add-ons"> >+<!ENTITY browseAddons.label "Browse All Add-ons"> >+<!ENTITY searchFailed.label "&brandShortName; couldn't retrieve add-ons"> >+<!ENTITY recommendedHeader.label "Recommended"> >+<!ENTITY recommendedThrobber.label "Retrieving recommended add-ons"> >+<!ENTITY searchThrobber.label "Searching add-ons"> >+<!ENTITY resetSearch.label "Clear Results"> >+<!ENTITY searchEmpty.label "No Matching Add-ons"> >+<!ENTITY searchEmpty.button "Ok"> >+<!ENTITY cancelSearch.button "Cancel"> >+<!ENTITY searchResultHomepage.value "Learn More"> >+<!ENTITY searchBox.label "Search All Add-ons"> >+<!ENTITY recommendedResults.label "See all Recommended Add-ons"> nit: the case of 'all' seems wrong here. >Index: toolkit/mozapps/extensions/content/extensions.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v >retrieving revision 1.154 >diff -u -8 -p -w -r1.154 extensions.js >--- toolkit/mozapps/extensions/content/extensions.js 18 Jan 2008 18:18:42 -0000 1.154 >+++ toolkit/mozapps/extensions/content/extensions.js 19 Jan 2008 22:46:52 -0000 >... >@@ -865,21 +1170,31 @@ XPInstallDownloadManager.prototype = { > var type = isTheme ? nsIUpdateItem.TYPE_THEME : nsIUpdateItem.TYPE_EXTENSION; > var item = Components.classes["@mozilla.org/updates/item;1"] > .createInstance(Components.interfaces.nsIUpdateItem); > item.init(url, " ", "app-profile", "", "", displayName, url, "", iconURL, "", "", type, ""); > items.push(item); > > // Advance the enumerator > var certName = aParams.GetString(i++); >+ >+ // Check whether the install was triggered from the Get Add-ons pane. >+ var pos = gPendingInstalls.indexOf(url); >+ if (pos >= 0) >+ gPendingInstalls.splice(pos, 1); >+ else >+ switchPane = true; > } > > gExtensionManager.addDownloads(items, items.length, aManager); > updateOptionalViews(); > updateGlobalCommands(); >+ // Only switch to the installs pane if there was an install from a website >+ if (switchPane) >+ showView("installs"); Thanks for this... this should also switch when initiating an install from a DnD operation onto the add-ons mgr. which I believe this does but the comment only calls out website. >Index: toolkit/mozapps/extensions/public/nsIAddonRepository.idl >=================================================================== >RCS file: toolkit/mozapps/extensions/public/nsIAddonRepository.idl >diff -N toolkit/mozapps/extensions/public/nsIAddonRepository.idl >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/mozapps/extensions/public/nsIAddonRepository.idl 19 Jan 2008 22:46:53 -0000 >@@ -0,0 +1,198 @@ >... >+[scriptable, uuid(a549a714-2ada-4bb9-8a47-be26e73d49a5)] >+interface nsIAddonSearchResult : nsISupports >... >+ /** >+ * And EULA that must be accepted before install. nit: A EULA >+ */ >+ readonly attribute AString eula; >+ >+ /** >+ * The type of the add-on, see nsIUpdateItem nit: The add-on type (see nsIUpdateItem) >+[scriptable, uuid(a6f70917-dd30-4eb6-8b3d-453204f96f33)] >+interface nsIAddonSearchResultsCallback : nsISupports >+{ >+ /** >+ * The search succeeded, the results are provided. nit: Called when a search has succeeded. >+ * >+ * @param aAddons an array of the add-on results. In the case of >+ * searching for specific terms this should be ordered >+ * by relevance. nit: should be? If this is up to the search provider then state something like "the order of the results is determined by the search provider". >+ * @param aAddonCount The number of add-on results nit: the length of aAddons. >+ * @param aTotalResults The total results actually available in the >+ * repository >+ */ >+ void searchSucceeded([array, size_is(aAddonCount)] in nsIAddonSearchResult aAddons, >+ in unsigned long aAddonCount, >+ in unsigned long aTotalResults); >+ >+ /** >+ * Indicates that an error occured performing the search. nit: Called when an error occured when performing a search. >+ */ >+ void searchFailed(); >+}; >+ >+/** >+ * The add-on repository is a source of add-ons that can be installed. It can >+ * be searched in two ways. One returns a list of add-ons that come highly >+ * recommended, this list should change frequently. The other way is to >+ * search for specific search terms entered by the user. >+ * In both cases the results should be filtered to only include add-ons that nit: this should just continue on the line above. >+ * are compatible with the current application and are not already installed. it isn't clear where the responsibility lies "to only include add-ons that are compatible...". >+ * Searches are always asynchronous and should be passed to the callback object nit: s/asynchronous and should be passed/asynchronous and the results should be passed/ >+ * provided. >+ */ >+[scriptable, uuid(c4d2ac29-6edc-43cd-8dc8-e4cf213aa1be)] >+interface nsIAddonRepository : nsISupports >+{ >+ /** >+ * The homepage for visiting this repository in a web browser. If empty then nit: "in a web browser" doesn't need to be called out. >+ * no way to visit the repository will be provided. "If empty..." - just note that this may be null or an empty string, etc. as other idl comments do. >+ */ >+ readonly attribute AString homepageURL; >+ >+ /** >+ * Returns whether this instance is currently performing a search. No new >+ * searche can be performed while this is the case. "No new searche..." - something like "New searches will not be performed while this is true". >+ */ >+ readonly attribute boolean searching; nit: I'm fine with this but I believe we tend to prefix booleans with 'is' so isSearching. >+ /** >+ * The url that can be visited by a webbrowser to see recommended add-ons in nit: "by a webbrowser" doesn't need to be called out. >+ * this repository. >+ */ >+ AString getRecommendedURL(); >+ >+ /** >+ * Retrieves the url that can be visited by a webbrowser to see search results >+ * for the given terms. nit: "by a webbrowser" doesn't need to be called out. >+ * >+ * @param aSearchTerms what to search the repository for nit: search terms used to search the repository. >+ */ >+ AString getSearchURL(in AString aSearchTerms); >+ >+ /** >+ * Begins a search for recommended add-ons in this repository. Results will >+ * be passed to the given callback. >+ * >+ * @param aCount the maximum number of results to return aCount doesn't seem right here... how about aMaxResults? >+ * @param aCallback the callback to pass results to >+ */ >+ void retrieveRecommendedAddons(in unsigned long aCount, >+ in nsIAddonSearchResultsCallback aCallback); >+ >+ /** >+ * Begins a search for add-ons in this repository. Results will be passed to >+ * the given callback. >+ * >+ * @param aSearchTerms the terms to search for >+ * @param aCount the maximum number of results to return Same here >+ * @param aCallback the callback to pass results to >+ */ >+ void searchAddons(in AString aSearchTerms, in unsigned long aCount, >+ in nsIAddonSearchResultsCallback aCallback); >+ >+ /** >+ * Cancels any search in progress. If there is no search in progress this >+ * does nothing. The way this is worded makes me think it may be possible to perform multiple searches at the same time. Can you reword this? >Index: toolkit/mozapps/extensions/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/Makefile.in,v >retrieving revision 1.9 >diff -u -8 -p -w -r1.9 Makefile.in >--- toolkit/mozapps/extensions/src/Makefile.in 16 Aug 2007 00:43:17 -0000 1.9 >+++ toolkit/mozapps/extensions/src/Makefile.in 19 Jan 2008 22:46:53 -0000 >@@ -40,16 +40,20 @@ topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk > > MODULE = extensions > > EXTRA_COMPONENTS = nsExtensionManager.js >-EXTRA_PP_COMPONENTS = nsBlocklistService.js >+EXTRA_PP_COMPONENTS = \ >+ nsBlocklistService.js \ >+ nsAMORepository.js \ What do you think about renaming nsAMORepository to nsAddonRepository etc. throughout? Not all consumers will use AMO.

(In reply to comment #18) > >+pref("extensions.getAddons.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended"); > >+pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/list/featured/all/10"); > >+pref("extensions.getAddons.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%"); > >+pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/search/%TERMS%"); > Have these urls been confirmed as correct? browseAddons looked wrong so I've corrected that. I'll get Laura to confirm them all though. > >+ // Only switch to the installs pane if there was an install from a website > >+ if (switchPane) > >+ showView("installs"); > Thanks for this... this should also switch when initiating an install from a > DnD operation onto the add-ons mgr. which I believe this does but the comment > only calls out website. Changed this to note that it'll switch for any install not started by the Get Add-ons pane > >+ * The add-on repository is a source of add-ons that can be installed. It can > >+ * be searched in two ways. One returns a list of add-ons that come highly > >+ * recommended, this list should change frequently. The other way is to > >+ * search for specific search terms entered by the user. > >+ * In both cases the results should be filtered to only include add-ons that > nit: this should just continue on the line above. > > >+ * are compatible with the current application and are not already installed. > it isn't clear where the responsibility lies "to only include add-ons that are > compatible...". > > >+ * Searches are always asynchronous and should be passed to the callback object > nit: s/asynchronous and should be passed/asynchronous and the results should be > passed/ Reworded that section to follow that and make some more sense. > >+ nsBlocklistService.js \ > >+ nsAMORepository.js \ > What do you think about renaming nsAMORepository to nsAddonRepository etc. > throughout? Not all consumers will use AMO. I don't have a problem with it, I was just thinking of distinguishing a repository that uses the AMO API (which any consumer using this component would have to provide), as opposed to a completely different component that could do what it liked. Fixed all the other nits.

One thing we have to consider is the version check. I've currently got it commented out but I don't think we should land with it like that. Unfortunately that leaves us not getting any results last I checked which will make the feature look broken.

One suggestion was to obey the extensions.checkCompatibility setting and return incompatible results in that case, I'm not keen on that though, especially with no visual indication that the extensions may not work.

In the process of writing a unit test for the component I discovered that there was an error where when retrieving recommended results we could end up with repeated items. This adds an extra test to avoid that. Only change is in nsAddonRepository.js

Comment on attachment 298987 [details] [diff] [review] patch rev 4 >Index: browser/app/profile/firefox.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v >retrieving revision 1.254 >diff -u -8 -p -r1.254 firefox.js >--- browser/app/profile/firefox.js 24 Jan 2008 08:22:37 -0000 1.254 >+++ browser/app/profile/firefox.js 24 Jan 2008 19:14:34 -0000 >... >@@ -603,8 +612,10 @@ pref("browser.places.migratePostDataAnno > // 0 - don't pre-populate anything > // 1 - pre-populate site URL, but don't fetch certificate > // 2 - pre-populate site URL and pre-fetch certificate > pref("browser.ssl_override_behavior", 1); > > // replace newlines with spaces when pasting into <input type="text"> fields > pref("editor.singleLine.pasteNewlines", 2); > >+// The breakpad report server to link to in about:crashes >+pref("breakpad.reportURL", "http://crash-stats.mozilla.com/report/index/"); Remove this before checking in >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v >retrieving revision 1.30 >diff -u -8 -p -r1.30 extensions.dtd >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd 23 Jan 2008 20:32:34 -0000 1.30 >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd 24 Jan 2008 19:17:09 -0000 >... >@@ -77,16 +78,19 @@ > <!ENTITY cmd.installUpdate.accesskey "I"> > <!ENTITY cmd.installUpdate.tooltip "Install an update for this Add-on"> > <!ENTITY cmd.showUpdateInfo.label "Show Information"> > <!ENTITY cmd.showUpdateInfo.accesskey "S"> > <!ENTITY cmd.showUpdateInfo.tooltip "Show more information about these updates"> > <!ENTITY cmd.hideUpdateInfo.label "Hide Information"> > <!ENTITY cmd.hideUpdateInfo.accesskey "H"> > <!ENTITY cmd.hideUpdateInfo.tooltip "Hide information about these updates"> >+<!ENTITY cmd.installSearchResult.label "Add to &brandShortName;…"> >+<!ENTITY cmd.installSearchResult.accesskey "A"> >+<!ENTITY cmd.installSearchResult.tooltip "Download and install this Add-on"> Please check the case here since everywhere else that add-on is used it is typically in lowercase. >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v >retrieving revision 1.46 >diff -u -8 -p -r1.46 extensions.properties >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties 5 Dec 2007 05:03:55 -0000 1.46 >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties 24 Jan 2008 19:17:10 -0000 >@@ -98,15 +98,20 @@ updateAvailableMsg=Version %S is availab > > xpinstallDisabledMsgLocked=Software installation has been disabled by your system administrator. > xpinstallDisabledMsg=Software installation is currently disabled. Click Enable and try again. > safeModeMsg=All add-ons have been disabled by safe mode. > disabledCompatMsg=Add-on compatibility checking is disabled. You may have incompatible add-ons. > disabledUpdateSecurityMsg=Add-on update security checking is disabled. You may be compromised by updates. > noUpdatesMsg=No updates were found. > offlineUpdateMsg=%S is currently in offline mode and is unable to update Add-ons. Click Go Online and try again. >+offlineSearchMsg=%S is currently in offline mode and is unable to search for Add-ons. Click Go Online and try again. Wonder if Add-on should be in lowercase here? Could you check with Madhava and if so please file a new bug. >Index: toolkit/mozapps/extensions/content/extensions.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v >retrieving revision 1.154 >diff -u -8 -p -r1.154 extensions.js >--- toolkit/mozapps/extensions/content/extensions.js 18 Jan 2008 18:18:42 -0000 1.154 >+++ toolkit/mozapps/extensions/content/extensions.js 24 Jan 2008 19:17:14 -0000 >... >@@ -1657,16 +1973,17 @@ function updateOptionalViews() { > var updateableArc = rdfs.GetResource(PREFIX_NS_EM + "updateable"); > var updateable = ds.GetTarget(e, updateableArc, true); > updateable = updateable.QueryInterface(Components.interfaces.nsIRDFLiteral); > if (updateable.Value == "true") > showUpdates = true; > } > } > } >+ document.getElementById("search-view").hidden = !gShowGetAddonsPane; You only check the pref value that sets gShowGetAddonsPane during startup so just set whether search-view is hidden during startup. >Index: toolkit/mozapps/extensions/src/nsAddonRepository.js >=================================================================== >RCS file: toolkit/mozapps/extensions/src/nsAddonRepository.js >diff -N toolkit/mozapps/extensions/src/nsAddonRepository.js >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/mozapps/extensions/src/nsAddonRepository.js 24 Jan 2008 19:17:16 -0000 >@@ -0,0 +1,365 @@ >... >+ var addon = new AddonSearchResult(); >+ addon.id = guid[0].textContent; >+ addon.rating = -1; >+ var node = element.firstChild; >+ while (node) { >+ if (node instanceof Ci.nsIDOMElement) { >+ switch (node.localName) { >+ case "name": >+ case "version": >+ case "summary": >+ case "description": >+ case "eula": >+ addon[node.localName] = node.textContent; >+ break; >+ case "rating": >+ if (node.textContent.length > 0) >+ addon.rating = parseInt(node.textContent); >+ break; >+ case "thumbnail": >+ addon.thumbnailURL = node.textContent; >+ break; >+ case "icon": >+ addon.iconURL = node.textContent; >+ break; >+ case "learnmore": >+ addon.homepageURL = node.textContent; >+ break; >+ case "type": >+ if (node.getAttribute("id") == 2) >+ addon.type = Ci.nsIUpdateItem.TYPE_THEME; >+ else >+ addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION; This is a tad confusing for me. The node's value for id is the add-on type? Won't there be multiple nodes of the same type and hence duplicate id's? A value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is 4? With an answer or fix for the above r=me

(In reply to comment #25) > >... > >+<!ENTITY cmd.installSearchResult.tooltip "Download and install this Add-on"> > Please check the case here since everywhere else that add-on is used it is > typically in lowercase. nevermind... this looks fine in its context. We should probably do a review of the usage at some point.

(In reply to comment #25) Yeah, when in a sentence, we should go with "add-on" rather than "Add-on". > (From update of attachment 298987 [details] [diff] [review]) > >Index: browser/app/profile/firefox.js > >=================================================================== > >RCS file: /cvsroot/mozilla/browser/app/profile/firefox.js,v > >retrieving revision 1.254 > >diff -u -8 -p -r1.254 firefox.js > >--- browser/app/profile/firefox.js 24 Jan 2008 08:22:37 -0000 1.254 > >+++ browser/app/profile/firefox.js 24 Jan 2008 19:14:34 -0000 > >... > >@@ -603,8 +612,10 @@ pref("browser.places.migratePostDataAnno > > // 0 - don't pre-populate anything > > // 1 - pre-populate site URL, but don't fetch certificate > > // 2 - pre-populate site URL and pre-fetch certificate > > pref("browser.ssl_override_behavior", 1); > > > > // replace newlines with spaces when pasting into <input type="text"> fields > > pref("editor.singleLine.pasteNewlines", 2); > > > >+// The breakpad report server to link to in about:crashes > >+pref("breakpad.reportURL", "http://crash-stats.mozilla.com/report/index/"); > Remove this before checking in > > >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd > >=================================================================== > >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v > >retrieving revision 1.30 > >diff -u -8 -p -r1.30 extensions.dtd > >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd 23 Jan 2008 20:32:34 -0000 1.30 > >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd 24 Jan 2008 19:17:09 -0000 > >... > >@@ -77,16 +78,19 @@ > > <!ENTITY cmd.installUpdate.accesskey "I"> > > <!ENTITY cmd.installUpdate.tooltip "Install an update for this Add-on"> > > <!ENTITY cmd.showUpdateInfo.label "Show Information"> > > <!ENTITY cmd.showUpdateInfo.accesskey "S"> > > <!ENTITY cmd.showUpdateInfo.tooltip "Show more information about these updates"> > > <!ENTITY cmd.hideUpdateInfo.label "Hide Information"> > > <!ENTITY cmd.hideUpdateInfo.accesskey "H"> > > <!ENTITY cmd.hideUpdateInfo.tooltip "Hide information about these updates"> > >+<!ENTITY cmd.installSearchResult.label "Add to &brandShortName;…"> > >+<!ENTITY cmd.installSearchResult.accesskey "A"> > >+<!ENTITY cmd.installSearchResult.tooltip "Download and install this Add-on"> > Please check the case here since everywhere else that add-on is used it is > typically in lowercase. > > >Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties > >=================================================================== > >RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v > >retrieving revision 1.46 > >diff -u -8 -p -r1.46 extensions.properties > >--- toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties 5 Dec 2007 05:03:55 -0000 1.46 > >+++ toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties 24 Jan 2008 19:17:10 -0000 > >@@ -98,15 +98,20 @@ updateAvailableMsg=Version %S is availab > > > > xpinstallDisabledMsgLocked=Software installation has been disabled by your system administrator. > > xpinstallDisabledMsg=Software installation is currently disabled. Click Enable and try again. > > safeModeMsg=All add-ons have been disabled by safe mode. > > disabledCompatMsg=Add-on compatibility checking is disabled. You may have incompatible add-ons. > > disabledUpdateSecurityMsg=Add-on update security checking is disabled. You may be compromised by updates. > > noUpdatesMsg=No updates were found. > > offlineUpdateMsg=%S is currently in offline mode and is unable to update Add-ons. Click Go Online and try again. > >+offlineSearchMsg=%S is currently in offline mode and is unable to search for Add-ons. Click Go Online and try again. > Wonder if Add-on should be in lowercase here? Could you check with Madhava and > if so please file a new bug. > > >Index: toolkit/mozapps/extensions/content/extensions.js > >=================================================================== > >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v > >retrieving revision 1.154 > >diff -u -8 -p -r1.154 extensions.js > >--- toolkit/mozapps/extensions/content/extensions.js 18 Jan 2008 18:18:42 -0000 1.154 > >+++ toolkit/mozapps/extensions/content/extensions.js 24 Jan 2008 19:17:14 -0000 > >... > >@@ -1657,16 +1973,17 @@ function updateOptionalViews() { > > var updateableArc = rdfs.GetResource(PREFIX_NS_EM + "updateable"); > > var updateable = ds.GetTarget(e, updateableArc, true); > > updateable = updateable.QueryInterface(Components.interfaces.nsIRDFLiteral); > > if (updateable.Value == "true") > > showUpdates = true; > > } > > } > > } > >+ document.getElementById("search-view").hidden = !gShowGetAddonsPane; > You only check the pref value that sets gShowGetAddonsPane during startup so > just set whether search-view is hidden during startup. > > >Index: toolkit/mozapps/extensions/src/nsAddonRepository.js > >=================================================================== > >RCS file: toolkit/mozapps/extensions/src/nsAddonRepository.js > >diff -N toolkit/mozapps/extensions/src/nsAddonRepository.js > >--- /dev/null 1 Jan 1970 00:00:00 -0000 > >+++ toolkit/mozapps/extensions/src/nsAddonRepository.js 24 Jan 2008 19:17:16 -0000 > >@@ -0,0 +1,365 @@ > >... > >+ var addon = new AddonSearchResult(); > >+ addon.id = guid[0].textContent; > >+ addon.rating = -1; > >+ var node = element.firstChild; > >+ while (node) { > >+ if (node instanceof Ci.nsIDOMElement) { > >+ switch (node.localName) { > >+ case "name": > >+ case "version": > >+ case "summary": > >+ case "description": > >+ case "eula": > >+ addon[node.localName] = node.textContent; > >+ break; > >+ case "rating": > >+ if (node.textContent.length > 0) > >+ addon.rating = parseInt(node.textContent); > >+ break; > >+ case "thumbnail": > >+ addon.thumbnailURL = node.textContent; > >+ break; > >+ case "icon": > >+ addon.iconURL = node.textContent; > >+ break; > >+ case "learnmore": > >+ addon.homepageURL = node.textContent; > >+ break; > >+ case "type": > >+ if (node.getAttribute("id") == 2) > >+ addon.type = Ci.nsIUpdateItem.TYPE_THEME; > >+ else > >+ addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION; > This is a tad confusing for me. The node's value for id is the add-on type? > Won't there be multiple nodes of the same type and hence duplicate id's? A > value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is > 4? > > With an answer or fix for the above r=me >

(In reply to comment #25) > >+ case "type": > >+ if (node.getAttribute("id") == 2) > >+ addon.type = Ci.nsIUpdateItem.TYPE_THEME; > >+ else > >+ addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION; > This is a tad confusing for me. The node's value for id is the add-on type? > Won't there be multiple nodes of the same type and hence duplicate id's? A > value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is > 4? See f.e. https://services.addons.mozilla.org/en-US/firefox/api/search/vista, the type comes as <type id="2">Theme</type>. The content is a string representation. The id of the type is the id in AMO's database, these unfortunately do not match the nsIUpdateItem values. Will address the other bits. Going to wait for an AMO push that is scheduled for tonight before landing because the feature will appear broken without it. Madhava has also mentioned that it would be good to get bug 408116 reviewed to land at the same time if this is possible?

(In reply to comment #28) > (In reply to comment #25) > > >+ case "type": > > >+ if (node.getAttribute("id") == 2) > > >+ addon.type = Ci.nsIUpdateItem.TYPE_THEME; > > >+ else > > >+ addon.type = Ci.nsIUpdateItem.TYPE_EXTENSION; > > This is a tad confusing for me. The node's value for id is the add-on type? > > Won't there be multiple nodes of the same type and hence duplicate id's? A > > value of 2 for the node id denotes a theme whereas nsIUpdateItem.TYPE_THEME is > > 4? > > See f.e. https://services.addons.mozilla.org/en-US/firefox/api/search/vista, > the type comes as <type id="2">Theme</type>. The content is a string > representation. The id of the type is the id in AMO's database, these > unfortunately do not match the nsIUpdateItem values. Could you please add a comment here stating that this is the case? > Will address the other bits. Going to wait for an AMO push that is scheduled > for tonight before landing because the feature will appear broken without it. Sounds like a plan > Madhava has also mentioned that it would be good to get bug 408116 reviewed to > land at the same time if this is possible? Will do

The AMO push didn't happen last night so I won't land this in full. Going to land the strings before the freeze though.

Comment on attachment 299139 [details] [diff] [review] strings for checkin [checked in] Landed strings. Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd; /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd,v <-- extensions.dtd new revision: 1.31; previous revision: 1.30 done Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties; /cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v <-- extensions.properties new revision: 1.47; previous revision: 1.46 done

(In reply to comment #30) > Created an attachment (id=299139) [details] > strings for checkin > > The AMO push didn't happen last night so I won't land this in full. Going to > land the strings before the freeze though. I just noticed that offlineUpdateMsg uses 'Add-ons' with a capital, whereas all the other uses are lowercase. Should I file a spin-off?

(In reply to comment #33) > (In reply to comment #30) > > Created an attachment (id=299139) [details] [details] > > strings for checkin > > > > The AMO push didn't happen last night so I won't land this in full. Going to > > land the strings before the freeze though. > > I just noticed that offlineUpdateMsg uses 'Add-ons' with a capital, whereas all > the other uses are lowercase. Should I file a spin-off? Yes please

(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #30) > > > Created an attachment (id=299139) [details] [details] [details] > > > strings for checkin > > > > > > The AMO push didn't happen last night so I won't land this in full. Going to > > > land the strings before the freeze though. > > > > I just noticed that offlineUpdateMsg uses 'Add-ons' with a capital, whereas all > > the other uses are lowercase. Should I file a spin-off? > > Yes please Filed bug 414083.

Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.259; previous revision: 1.258 done Checking in toolkit/themes/gnomestripe/mozapps/extensions/extensions.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css,v <-- extensions.css new revision: 1.4; previous revision: 1.3 done Checking in toolkit/themes/pinstripe/mozapps/jar.mn; /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/jar.mn,v <-- jar.mn new revision: 1.25; previous revision: 1.24 done RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/eula.css,v done Checking in toolkit/themes/pinstripe/mozapps/extensions/eula.css; /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/eula.css,v <-- eula.css initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensionIcons.png,v done Checking in toolkit/themes/pinstripe/mozapps/extensions/extensionIcons.png; /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensionIcons.png,v <-- extensionIcons.png initial revision: 1.1 done Checking in toolkit/themes/pinstripe/mozapps/extensions/extensions.css; /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/extensions.css,v <-- extensions.css new revision: 1.34; previous revision: 1.33 done RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/ratings.png,v done Checking in toolkit/themes/pinstripe/mozapps/extensions/ratings.png; /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/ratings.png,v <-- ratings.png initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/searchIcons.png,v done Checking in toolkit/themes/pinstripe/mozapps/extensions/searchIcons.png; /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/extensions/searchIcons.png,v <-- searchIcons.png initial revision: 1.1 done Checking in toolkit/themes/winstripe/mozapps/jar.mn; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v <-- jar.mn new revision: 1.22; previous revision: 1.21 done RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/eula.css,v done Checking in toolkit/themes/winstripe/mozapps/extensions/eula.css; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/eula.css,v <-- eula.css initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensionIcons.png,v done Checking in toolkit/themes/winstripe/mozapps/extensions/extensionIcons.png; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensionIcons.png,v <-- extensionIcons.png initial revision: 1.1 done Checking in toolkit/themes/winstripe/mozapps/extensions/extensions.css; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/extensions.css,v <-- extensions.css new revision: 1.38; previous revision: 1.37 done RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/ratings.png,v done Checking in toolkit/themes/winstripe/mozapps/extensions/ratings.png; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/ratings.png,v <-- ratings.png initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/searchIcons.png,v done Checking in toolkit/themes/winstripe/mozapps/extensions/searchIcons.png; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/searchIcons.png,v <-- searchIcons.png initial revision: 1.1 done Checking in toolkit/themes/winstripe/mozapps/extensions/viewButtons.png; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/extensions/viewButtons.png,v <-- viewButtons.png new revision: 1.5; previous revision: 1.4 done Checking in toolkit/mozapps/extensions/jar.mn; /cvsroot/mozilla/toolkit/mozapps/extensions/jar.mn,v <-- jar.mn new revision: 1.6; previous revision: 1.5 done RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.js,v done Checking in toolkit/mozapps/extensions/content/eula.js; /cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.js,v <-- eula.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.xul,v done Checking in toolkit/mozapps/extensions/content/eula.xul; /cvsroot/mozilla/toolkit/mozapps/extensions/content/eula.xul,v <-- eula.xul initial revision: 1.1 done Checking in toolkit/mozapps/extensions/content/extensions.css; /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.css,v <-- extensions.css new revision: 1.18; previous revision: 1.17 done Checking in toolkit/mozapps/extensions/content/extensions.js; /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v <-- extensions.js new revision: 1.156; previous revision: 1.155 done Checking in toolkit/mozapps/extensions/content/extensions.xml; /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xml,v <-- extensions.xml new revision: 1.51; previous revision: 1.50 done Checking in toolkit/mozapps/extensions/content/extensions.xul; /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v <-- extensions.xul new revision: 1.69; previous revision: 1.68 done Checking in toolkit/mozapps/extensions/public/Makefile.in; /cvsroot/mozilla/toolkit/mozapps/extensions/public/Makefile.in,v <-- Makefile.in new revision: 1.5; previous revision: 1.4 done RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIAddonRepository.idl,v done Checking in toolkit/mozapps/extensions/public/nsIAddonRepository.idl; /cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIAddonRepository.idl,v <-- nsIAddonRepository.idl initial revision: 1.1 done Checking in toolkit/mozapps/extensions/src/Makefile.in; /cvsroot/mozilla/toolkit/mozapps/extensions/src/Makefile.in,v <-- Makefile.in new revision: 1.10; previous revision: 1.9 done RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsAddonRepository.js,v done Checking in toolkit/mozapps/extensions/src/nsAddonRepository.js; /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsAddonRepository.js,v <-- nsAddonRepository.js initial revision: 1.1 done Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in; /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v <-- nsExtensionManager.js.in new revision: 1.272; previous revision: 1.271 done

Forgot to add, I made a very minor tweak to this to deal with the proto landing. Just dropped the pane image and added a border at the bottom of the search bar in line with that at the bottom of the richlistbox.

Bah, forgot to add the component to packages-static, this should fix it up.

(In reply to comment #38) > Bah, forgot to add the component to packages-static, this should fix it up. While you're at it, could you please add this entry to suite/installer/*/packages as well?

Comment on attachment 299793 [details] [diff] [review] add to packages-static r=mano.

(In reply to comment #39) > (In reply to comment #38) > > Bah, forgot to add the component to packages-static, this should fix it up. > > While you're at it, could you please add this entry to > suite/installer/*/packages as well? I'll look at separate apps in separate bugs probably, they won't work without the prefs set as it is anyway.

Checked in the follow up fix, should work properly now Checking in browser/installer/unix/packages-static; /cvsroot/mozilla/browser/installer/unix/packages-static,v <-- packages-static new revision: 1.145; previous revision: 1.144 done Checking in browser/installer/windows/packages-static; /cvsroot/mozilla/browser/installer/windows/packages-static,v <-- packages-static new revision: 1.148; previous revision: 1.147 done

"See all results (X)" link is broken due to URL encoding. Example: A search for Tab Mix Plus is parsed to the AMO search engine as: Tab%20Mix%20Plus and returns zero results. Also it appears this feature automatically limits search results to Add-Ons that are compatible with the version Firefox performing the search. This is sensible for most people but some might want to know a extension exists, it just hasn't been updated yet.

The tool also fails to display items even when there are results. Example: Search for: net usage item zero results are displayed however the "See all ...(X)" total is 1. Clearly a result seems to have been found but is not displayed. This may be what I was interpreting in my previous comment as 'limiting to compatible add ons'.

(In reply to comment #45) This is because we're filtering out add-ons that are (a) incompatible with your version of firefox and/or (b) already installed. You're right - there should be a message explaining this. What we're considering is "All compatible results are already installed" (still subject to some change - suggestions?). The number in the link refers to the unfiltered result set - the link should take you to the search results page in AMO where you can see that full list. > The tool also fails to display items even when there are results. Example: > > Search for: > > net usage item > > zero results are displayed however the "See all ...(X)" total is 1. Clearly a > result seems to have been found but is not displayed. This may be what I was > interpreting in my previous comment as 'limiting to compatible add ons'. >

Filtering out add-ons that are already installed sounds confusing. How about showing them with a check mark?

The homepage of add-ons won't open. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre Error: uri is not defined Source File: chrome://mozapps/content/extensions/extensions.js Line: 2446 cmd_homepage: function (aSelectedItem) { if (!aSelectedItem) return; var homepageURL = aSelectedItem.getAttribute("homepageURL"); // only allow http(s) homepages if (isSafeURI(homepageURL)) - openURL(uri.spec); + openURL(homepageURL); },

Comment on attachment 300133 [details] [diff] [review] fix for: homepage does not open (uri is not defined) [checked in] Nice catch, thanks

Comment on attachment 300133 [details] [diff] [review] fix for: homepage does not open (uri is not defined) [checked in] Checking in toolkit/mozapps/extensions/content/extensions.js; /cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v <-- extensions.js new revision: 1.159; previous revision: 1.158 done

marking this original bug as Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012904 Minefield/3.0b3pre.

Two remarks: 1. The 'restart firefox' button is gone, so one cannot easily restart FF if for example I uninstall a theme or extension, as it is now only shown after installing a new extension or theme. 2. Themes and extensions already installed are not shown in the list, only a link is shown saying 'See all results(1)'. This is ugly and confusing.

(In reply to comment #52) > 1. The 'restart firefox' button is gone, so one cannot easily restart FF if for > example I uninstall a theme or extension, as it is now only shown after > installing a new extension or theme. That was bug 408116. I do miss it. :(

(In reply to comment #52) > Two remarks: > 1. The 'restart firefox' button is gone, so one cannot easily restart FF if for > example I uninstall a theme or extension, as it is now only shown after > installing a new extension or theme. The notification bar with the restart button should be coming up after any action that requires a restart -- installations, updates, and uninstalls. It's working for me in the case of uninstalling -- can you provide steps to reproduce this? > 2. Themes and extensions already installed are not shown in the list, only a > link is shown saying 'See all results(1)'. This is ugly and confusing. >

I was expecting (and was needing the restart button to test something else) that the restart button would also appear when uninstalling an unused theme.

Unused themes can be uninstalled without a restart.

The 'see all results (19)' link and the 'Clear results' button should not be in the scroll pane, but in the real footer: #commandBarBottom. See bug 415276.

Bug 417606 added some testcases for this.