Closed
Bug 553169
Opened 14 years ago
Closed 14 years ago
Initial review of API rewrite
Categories
(Toolkit :: Add-ons Manager, defect, P1)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a5
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Whiteboard: [rewrite])
Attachments
(9 files, 17 obsolete files)
21.93 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
95.18 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
100.50 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
153.45 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
14.67 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
55.66 KB,
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
153.62 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
6.94 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
41.18 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
Time for review, attached are a set of patches (I couldn't quite make them independent but I have gone for separation by area) of the rewrite. Sorry, it's kinda big. These are patches of http://hg.mozilla.org/projects/addonsmgr/rev/9c8eaad44ef0 against a recent m-c changeset with the following breakdown: updatechecker toolkit/mozapps/extensions/UpdateChecker.jsm toolkit toolkit/xre/nsAppRunner.cpp toolkit/components/startup/public/nsIAppStartup.idl toolkit/components/startup/src/nsAppStartup.cpp toolkit/components/startup/src/nsAppStartup.h base toolkit/mozapps/extensions/AddonManager.jsm toolkit/mozapps/extensions/extIManager.idl toolkit/mozapps/extensions/extManager.js toolkit/mozapps/extensions/Makefile.in xpinstall toolkit/mozapps/extensions/extContentHandler.js toolkit/mozapps/extensions/extIInstallTrigger.idl toolkit/mozapps/extensions/extIWebInstallListener.idl toolkit/mozapps/extensions/extInstallTrigger.cpp toolkit/mozapps/extensions/extInstallTrigger.h toolkit/mozapps/extensions/extWebInstallListener.js toolkit/mozapps/xpinstall/content/xpinstallConfirm.js lwtheme toolkit/mozapps/extensions/LightweightThemeManager.jsm xpiprovider toolkit/mozapps/extensions/XPIProvider.jsm browsertests toolkit/mozapps/extensions/test/browser xpcshelltests toolkit/mozapps/extensions/test/xpcshell toolkit/mozapps/extensions/test/addons xpinstalltests toolkit/mozapps/extensions/test/xpinstall
Assignee | ||
Comment 1•14 years ago
|
||
Implements a standalone JSM for retrieving information for an add-on's update manifest
Assignee | ||
Comment 2•14 years ago
|
||
Adjusts how the platform talks to the add-ons manager during startup.
Assignee | ||
Updated•14 years ago
|
Attachment #433273 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 3•14 years ago
|
||
The basic AddonManager implementation. This has no real functionality without providers which are in later patches.
Attachment #433275 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 4•14 years ago
|
||
The big one, implements a provider for XPI based extensions.
Attachment #433277 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 5•14 years ago
|
||
Implements a basic provider adding lightweight themes.
Attachment #433278 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 6•14 years ago
|
||
This is basically the replacement for xpinstall, implements InstallTrigger, the application/x-xpinstall content handler and wires up the install confirmation dialog and blocking UI.
Attachment #433279 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 7•14 years ago
|
||
Reimplements the tests for bug 435743 on the new API
Attachment #433280 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 8•14 years ago
|
||
The main automated tests for the new API. I have not yet gone through and checked which of the old tests need to be ported across, that will come later in the week as an additional patch hopefully.
Attachment #433281 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 9•14 years ago
|
||
Not requesting review just yet since a couple of these are failing for unknown reasons but these are the xpinstall automated tests ported to the new API.
Assignee | ||
Comment 10•14 years ago
|
||
In addition the following files are to be removed: toolkit/mozapps/extensions/nsExtensionManager.js toolkit/mozapps/extensions/nsIExtensionManager.idl toolkit/mozapps/extensions/test/bug435743.rdf toolkit/mozapps/extensions/test/bug435743.sjs toolkit/mozapps/extensions/test/bug435743.xpi toolkit/mozapps/extensions/test/test_bug435743_1.xul toolkit/mozapps/extensions/test/test_bug435743_2.xul
Assignee | ||
Comment 11•14 years ago
|
||
Note that there are still some known issues and things to be implemented, these are listed as blockers to bug 461973 and will have their own patches filed.
Comment 12•14 years ago
|
||
If you think I can lend a hand here on any particular part of this, let me know. I may not have been active on extmgr in a while, but I do care, and would make some weekend time available for this.
Comment 13•14 years ago
|
||
Comment on attachment 433273 [details] [diff] [review] updatechecker rev 1 Quick initial review to get things started... many of the comments below will likely refer to all files for this review >diff --git a/toolkit/mozapps/extensions/UpdateChecker.jsm b/toolkit/mozapps/extensions/UpdateChecker.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/UpdateChecker.jsm >@@ -0,0 +1,629 @@ >+/* >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is the Extension Manager. >+# >+# The Initial Developer of the Original Code is >+# Mozilla Corporation. Per Gerv http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html >... >+/** >+ * The UpdateChecker is responsible for retrieving the update information from >+ * an add-on's update manifest. nit: does the update checker really retrieve update information 'from' an add-on's update manifest or is it from the update's remote update rdf? >+ */ >+ >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+ >+var EXPORTED_SYMBOLS = [ "UpdateChecker" ]; I'm thinking this should be more specific since we have a couple of different update checkers in the app. Perhaps AddonUpdateChecker? >+ /** >+ * Serializes all em:* (see EM_NS) properties of an RDF resource except for >+ * the em:signature property. As this serialization is to be compared against >+ * the manifest signature it cannot contain the em:signature property itself. >+ * @param ds >+ * The datasource holding the data nit: 'holding the data' doesn't provide additional understanding here and elsewhere... datasources by their very definition hold data. Might want to just have "The RDF datasource" >+ * @param resource >+ * The RDF resource to output the properties of nit: of what? >+ /** >+ * Recursively serializes an RDF resource and all resources it links to. >+ * This will only output EM_NS properties and will ignore any em:signature >+ * property. >+ * @param ds >+ * The datasource holding the data >+ * @param resource >+ * The RDF resource to serialize >+ * @param indent >+ * The current level of indent for pretty-printing. Leave undefined >+ * for no indent nit: If undefined no indent will be added >+ * @returns a string containing the serialized resource. >+ * @throws if the RDF data contains multiple references to the same resource. >+ */ >+ serializeResource: function RDFSerializer_serializeResource(ds, resource, indent) { >+ if (this.resources.indexOf(resource) != -1 ) { >+ // We cannot output multiple references to the same resource. >+ throw new Error("Cannot serialize multiple references to "+resource.Value); space before / after + >+ let item = id; >+ switch (type) { >+ case "extension": >+ item = PREFIX_EXTENSION + item; >+ break; >+ case "theme": >+ item = PREFIX_THEME + item; >+ break; >+ default: >+ item = PREFIX_ITEM + item; >+ break; >+ } Seems like this should just be + id and refactored accordingly. I am also of the opinion that using let when it is the equivelent of var is not a good thing since it is done without any though and often leads to let declarations outside of a block at the top level where it is for all practical purposes the same as a var just to use let within a block where just declaring var in the block would provide the same functionality. >+ while (items.hasMoreElements()) { >+ let item = items.getNext().QueryInterface(Ci.nsIRDFResource); >+ let version = getRequiredProperty(ds, item, "version"); >+ LOG("Found an update entry for version " + version); I suspect this will get fixed in the improve logging bug but just in case this should at least state the id for the item here and elsewhere so they can be easily associated to the item. >+ let xml = request.responseXML; >+ if (!xml || xml.documentElement.namespaceURI == XMLURI_PARSE_ERROR) { >+ WARN("Update manifest was not valid XML"); >+ this.notifyError(UpdateChecker.ERROR_PARSE_ERROR); >+ return; >+ } >+ >+ // We currently only know about RDF update manifests >+ if (xml.documentElement.namespaceURI == PREFIX_NS_RDF) { nit: I think this would be cleaner if you checked if it wasn't PREFIX_NS_RDF and have success at the end so everything before the end is failure case. >+ /** >+ * Called when the manifest failed to load. >+ */ >+ onError: function() { >+ if (!this.timer) >+ return; When does the code end up here without a timer? Please add a comment for that case. >+ /** >+ * Helper method to notify the observer that an error occured. >+ */ >+ notifyError: function(status) { >+ if ("onUpdateCheckFailed" in this.observer) >+ this.observer.onUpdateCheckFailed(status); I think this should be onUpdateCheckError for consistency with this being called when the xhr returns an error, etc. >+function matchesApplication(update, appVersion, platformVersion) { >+ let result = false; >+ for (let i = 0; i < update.targetApplications.length; i++) { There have been various cases where we don't jit when using let statements especially in loops. The following provides info on getting a log for the aborts http://hacks.mozilla.org/2009/07/tracemonkey-overview/
Comment 14•14 years ago
|
||
(In reply to comment #13) > >... > >+/** > >+ * The UpdateChecker is responsible for retrieving the update information from > >+ * an add-on's update manifest. > nit: does the update checker really retrieve update information 'from' an > add-on's update manifest or is it from the update's remote update rdf? add-on's remote update rdf that is. I think I am just confusing manifest since I'm used to the previous term.
Comment 15•14 years ago
|
||
Comment on attachment 433273 [details] [diff] [review] updatechecker rev 1 >+/** >+ * A serialisation method for RDF data that produces an identical string >+ * provided that the RDF assertions match. >+ * The serialisation is not complete, only assertions stemming from a given >+ * resource are included, multiple references to the same resource are not >+ * permitted, and the RDF prolog and epilog are not included. >+ * RDF Blob and Date literals are not supported. nit: this last sentence could be worded better. >+ */ >+function RDFSerializer() { >+ this.cUtils = Cc["@mozilla.org/rdf/container-utils;1"]. >+ getService(Ci.nsIRDFContainerUtils); >+ this.resources = []; >+} >+ >+RDFSerializer.prototype = { >+ INDENT: " ", // The indent used for pretty-printing >+ resources: null, // Array of the resources that have been found >+ >+ /** >+ * Escapes characters from a string that should not appear in XML. >+ * @param string >+ * The string to be escaped >+ * @returns a string with all characters invalid in XML character data >+ * converted to entity references. >+ */ >+ escapeEntities: function RDFSerializer_escapeEntities(string) { >+ string = string.replace(/&/g, "&"); >+ string = string.replace(/</g, "<"); >+ string = string.replace(/>/g, ">"); >+ string = string.replace(/"/g, """); >+ return string; nit: should just be return string.replace(/"/g, """); though I could go either way on this >+ }, >+ >+ /** >+ * Serializes all the elements of an RDF container. >+ * @param ds >+ * The datasource holding the data >+ * @param container >+ * The RDF container to output the child elements of >+ * @param indent >+ * The current level of indent for pretty-printing >+ * @returns a string containing the serialized elements. >+ */ >+ serializeContainerItems: function RDFSerializer_serializeContainerItems(ds, container, indent) { >+ var result = ""; >+ var items = container.GetElements(); >+ while (items.hasMoreElements()) { >+ var item = items.getNext().QueryInterface(Ci.nsIRDFResource); >+ result += indent + "<RDF:li>\n" >+ result += this.serializeResource(ds, item, indent + this.INDENT); >+ result += indent + "</RDF:li>\n" >+ } >+ return result; >+ }, nit: before the verification of falling off trace it would be a good thing if let and var were used consistently in this and other files. I tend to ask myself why did the developer use let vs. var when I see them used and when they are used inconsistently it makes it harder to answer that question. >+ /** >+ * Serializes all em:* (see EM_NS) properties of an RDF resource except for >+ * the em:signature property. As this serialization is to be compared against >+ * the manifest signature it cannot contain the em:signature property itself. >+ * @param ds >+ * The datasource holding the data >+ * @param resource >+ * The RDF resource to output the properties of >+ * @param indent >+ * The current level of indent for pretty-printing >+ * @returns a string containing the serialized properties. should include a comment for the case where it throws >... >+/** >+ * Parses an RDF style update manifest into an array of update objects. >+ * @param id >+ * The ID of the add-on being checked for updates >+ * @param type >+ * The type of the add-on being checked for updates >+ * @param updateKey >+ * An optional update key for the add-on >+ * @param The XMLHttpRequest that has retrieved the update manifest >+ * @returns an array of update objects should include a comment for the case where it throws... please verify this is present for this and other patches >... >+ if (result.updateURL && checkSecurity && >+ result.updateURL.substring(0, 6) != "https:" && >+ (!result.updateHash || result.updateHash.substring(0, 3) != "sha")) { >+ WARN("updateLink " + result.updateURL + " is not secure and is not verified" + >+ " by a strong enough hash."); Please add the minimum hash required for it to be strong enough in WARN >+ delete result.updateURL; >+ delete result.updateHash; >+ } >+ results.push(result); >+ } >+ } >+ return results; >+} >+ >+/** >+ * Starts downloading an update manifest and then passes it to an appropriate >+ * parser to convert to an array of update objects >+ * @param id >+ * The ID of the add-on being checked for updates >+ * @param type >+ * The type of the add-on being checked for updates nit: The type of add-on being checked for updates >+ * @param updateKey >+ * An optional update key for the add-on >+ * @param url >+ * The URL of the update manifest >+ * @param observer >+ * An observer to pass results to >+ */ >+function UpdateParser(id, type, updateKey, url, observer) { >+ this.id = id; >+ this.type = type; >+ this.updateKey = updateKey; >+ this.observer = observer; >+ >+ this.timer = Cc["@mozilla.org/timer;1"]. >+ createInstance(Ci.nsITimer); >+ this.timer.initWithCallback(this, TIMEOUT, Ci.nsITimer.TYPE_ONE_SHOT); >+ >+ LOG("Requesting " + url); >+ this.request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]. >+ createInstance(Ci.nsIXMLHttpRequest); >+ this.request.open("GET", url, true); >+ this.request.channel.notificationCallbacks = new BadCertHandler(); >+ this.request.overrideMimeType("text/xml"); Perhaps don't remove the following which was added bug 318793 as well or an explanation as to why it is no longer needed? this.request.channel.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE; >+ var self = this; >+ this.request.onload = function(event) { self.onLoad() }; >+ this.request.onerror = function(event) { self.onError() }; >+ this.request.send(null); >+} >+ >+UpdateParser.prototype = { >+ id: null, >+ type: null, >+ updateKey: null, >+ observer: null, >+ request: null, >+ timer: null, >+ >+ /** >+ * Called when the manifest has been successfully loaded. >+ */ >+ onLoad: function() { >+ this.timer.cancel(); >+ this.timer = null; >+ let request = this.request; >+ this.request = null; >+ >+ try { >+ checkCert(request.channel); >+ } >+ catch (e) { >+ this.notifyError(UpdateChecker.ERROR_DOWNLOAD_ERROR); >+ return; >+ } >+ >+ if (request.status != 200 && request.status != 0) { Will isSuccessCode work here? >+/** >+ * Tests if an update matches a version of the application/platform nit: might as well just be application or platform since there is room >+ * @param update >+ * The available update >+ * @param appVersion >+ * The application version to use >+ * @param platformVersion >+ * The platform version to use >+ * @returns true if the update is compatible with the application/platform >+ */ >+function matchesApplication(update, appVersion, platformVersion) { nit: this could be better named since it checks whether it is compatible with the application or the platform which is quite different than matching an application. >+ let result = false; >+ for (let i = 0; i < update.targetApplications.length; i++) { >+ let app = update.targetApplications[i]; >+ if (app.id == Services.appinfo.ID) { >+ return (Services.vc.compare(appVersion, app.minVersion) >= 0) && >+ (Services.vc.compare(appVersion, app.maxVersion) <= 0); >+ } >+ if (app.id == TOOLKIT_ID) { >+ result = (Services.vc.compare(platformVersion, app.minVersion) >= 0) && >+ (Services.vc.compare(platformVersion, app.maxVersion) <= 0); >+ } nit: seems like there is a mix of using braces when you don't have to and not using them.
Attachment #433273 -
Flags: review?(robert.bugzilla) → review-
Comment 16•14 years ago
|
||
Also, per java-doc spec @return should be used instead of @returns
Comment 17•14 years ago
|
||
I see that in at least one other patch you use intitals to prefix the anonymous functions... might want to do that in the first file I reviewed as well. For example, RDFS_
Comment 18•14 years ago
|
||
Comment on attachment 433275 [details] [diff] [review] base rev 1 First go at the review >diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/AddonManager.jsm >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+ >+const PREF_EM_UPDATE_ENABLED = "extensions.update.enabled"; >+ >+Components.utils.import("resource://gre/modules/Services.jsm"); >+ >+var EXPORTED_SYMBOLS = [ "AddonManager", "AddonManagerInternal" ]; AddonManagerInternal being exported seems wrong... I understand what you are trying to do but I think it should be renamed since it isn't internal since it is exported. >+/** >+ * Calls a method on a provider if it exists and consumes any thrown exception. >+ * Any parameters after the dflt parameter are passed to the provider's method. >+ * @param provider >+ * The provider to call >+ * @param method >+ * The method name to call >+ * @param dflt >+ * A default return value of the provider does not implement the named nit: if it does not? >+ * method or throws an error. >+ * @returns The nsIRDFContainer, initialized at the root. nit: or dflt if it does not implement the named method or throws an error >+ */ >+function callProvider(provider, method, dflt) { >+ if (!(method in provider)) >+ return dflt; >+ var args = Array.slice(arguments, 3); >+ try { >+ return provider[method].apply(provider, args); >+ } >+ catch (e) { >+ ERROR("Exception calling provider." + method + ": " + e); >+ return dflt; >+ } >+} >+ >+/** >+ * This is the real manager, kept here rather than in AddonManager to keep its >+ * contents hidden from API users. >+ */ >+var AddonManagerPrivate = { >+ installListeners: null, >+ addonListeners: null, >+ providers: [], >+ started: false, >+ >+ /** >+ * Initialises the AddonManager, loading any known providers and initialising nit: Initializes - 398 matching lines in 253 files Initialises - 12 matching lines in 10 files >+ * them. >+ */ >+ startup: function AM_startup() { >+ if (this.started) >+ return; >+ >+ this.installListeners = []; >+ this.addonListeners = []; >+ >+ let appChanged = true; >+ try { >+ appChanged = Services.appinfo.version != Services.prefs.getCharPref("extensions.lastAppVersion"); >+ } >+ catch (e) { } >+ if (appChanged) { nit: your other if statements are preceded by an empty line... might as well be consistent >+ LOG("Application has been upgraded"); >+ Services.prefs.setCharPref("extensions.lastAppVersion", Services.appinfo.version); >+ } >+ >+ // Ensure all known providers have had a chance to register themselves nit: should probably be "Ensure all default providers" etc. since that is how they are called out previously >+ PROVIDERS.forEach(function(url) { >+ Components.utils.import(url, {}); >+ }); >+ >+ let needsRestart = false; >+ this.providers.forEach(function(provider) { >+ callProvider(provider, "startup"); >+ if (callProvider(provider, "checkForChanges", false, appChanged)) >+ needsRestart = true; >+ }); >+ this.started = true; >+ >+ // Flag to the platform that a restart is necessary >+ if (needsRestart) { >+ let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]. >+ getService(Ci.nsIAppStartup2); >+ appStartup.needsRestart = needsRestart; >+ } >+ }, >+ >+ /** >+ * Registers a new AddonProvider. >+ * @param provider >+ * The provider to register >+ */ >+ registerProvider: function AM_registerProvider(provider) { >+ this.providers.push(provider); >+ >+ // If we're registering after startup call this provider's startup immediately. >+ if (this.started) >+ callProvider(provider, "startup"); >+ }, >+ >+ /** >+ * Shuts down the addon manager and all registered providers, this must clean >+ * up everything in order for automated tests to fake restarts. >+ */ >+ shutdown: function AM_shutdown() { >+ this.providers.forEach(function(provider) { >+ callProvider(provider, "shutdown"); >+ }); >+ >+ this.installListeners = null; >+ this.addonListeners = null; >+ this.started = false; >+ }, >+ >+ /** >+ * Performs a background update check by starting an update for all add-ons >+ * that are available to update. >+ */ >+ backgroundUpdateCheck: function AM_backgroundUpdateCheck() { >+ if (!Services.prefs.getBoolPref(PREF_EM_UPDATE_ENABLED)) >+ return; >+ this.getAddonsByType(null, function(addons) { >+ addons.forEach(function(addon) { >+ if (addon.permissions & AddonManager.PERM_CANUPGRADE) { >+ addon.findUpdates({ >+ onUpdateAvailable: function(addon, install) { nit: probably should name these anonymous functions >+ install.install(); >+ } >+ }, AddonManager.UPDATE_WHEN_PERIODIC_UPDATE); >+ } >+ }); >+ }); >+ }, >+ >+ /** >+ * Calls all registered InstallListeners with an event. Any parameters after >+ * the additional parameter are passed to the listener. >+ * @param method >+ * The method on the listeners to call >+ * @param additional >+ * An array of InstallListeners to also call >+ * @returns false if any of the listeners returned false, true otherwise >+ */ >+ callInstallListeners: function AM_callInstallListeners(method, additional) { >+ let result = true; >+ let listeners = this.installListeners; >+ if (additional) >+ listeners = additional.concat(listeners); >+ let args = Array.slice(arguments, 2); >+ listeners.forEach(function(listener) { >+ try { >+ if (method in listener) { >+ if (listener[method].apply(listener, args) === false) >+ result = false; >+ } >+ } >+ catch (e) { } Would it be worth logging if a listener throws? >+ }); >+ return result; >+ }, >+ >+ /** >+ * Calls all registered AddonListeners with an event. Any parameters after >+ * the method parameter are passed to the listener. >+ * @param method >+ * The method on the listeners to call >+ */ >+ callAddonListeners: function AM_callInstallListeners(method) { >+ var args = Array.slice(arguments, 1); >+ this.addonListeners.forEach(function(listener) { >+ try { >+ if (method in listener) >+ listener[method].apply(listener, args); >+ } >+ catch (e) { } >+ }); >+ }, >+ >+ /** >+ * Notifies all providers that an add-on has been enabled when that type of >+ * add-on only supports a single add-on being enabled at a time. This allows >+ * the providers to disable theirs if necessary. notifyAddonSelected for an add-on that has been enabled is somewhat confusing... can you come up with a better name? It might be a good thing to provide a concrete example such as themes if they are applicable. Perhaps notifyAddonActivated and addonActivated?
Comment 19•14 years ago
|
||
(In reply to comment #12) > If you think I can lend a hand here on any particular part of this, let me > know. I may not have been active on extmgr in a while, but I do care, and > would make some weekend time available for this. I am going to try to get the reviews done over the weekend... perhaps you could review the tests?
Comment 20•14 years ago
|
||
Comment on attachment 433280 [details] [diff] [review] browsertests rev 1 Well, I'm not competent in the certificate parts of this, so I'll have to leave that review to him. I'll do what I can here. There's a lot of repeated lines here, and room for a future developer to really mess it up. For instance, >+TESTS = [ >+ // Tests that a simple update.rdf retrieval works as expected. >+ "http://example.com/" + xpi, >+ "https://example.com/" + xpi, >+ "https://nocert.example.com/" + xpi, >+ "https://self-signed.example.com/" + xpi, >+ "https://untrusted.example.com/" + xpi, >+ "https://expired.example.com/" + xpi, ... >+function run_test_1() { >+ run_install_tests(run_test_2, [ >+ [0, "Should have seen no failure for a http install url"], >+ [AddonManager.DOWNLOADERROR_NETWORK, "Should have seen a failure for a https install url from a non built-in CA"], >+ [AddonManager.DOWNLOADERROR_NETWORK, "Should have seen a failure for nocert https install url"], >+ [AddonManager.DOWNLOADERROR_NETWORK, "Should have seen a failure for self-signed https install url"], >+ [AddonManager.DOWNLOADERROR_NETWORK, "Should have seen a failure for untrusted https install url"], >+ [AddonManager.DOWNLOADERROR_NETWORK, "Should have seen a failure for expired https install url"], >+ >+ [0, "Should have seen no failure for a http to http redirect"], >+ [0, "Should have seen no failure for a http to https redirect for a non built-in CA"], ===== If I wasn't REALLY careful, I might accidentally add a test line at index 32 in TESTS, and at line 33 at this unnamed test-matches array. I think this is highly fragile and hard to maintain. It'd be better to have a constructor function: function IndividualAddonTest(redirectBase, xpiBase, expectedResult, testMsg) { const xpi = "browser/toolkit/mozapps/extensions/test/browser/browser_installssl.xpi"; const redirect = "browser/toolkit/mozapps/extensions/test/browser/redirect.sjs?"; var url = xpiBase + xpi; if (redirectBase) url = redirectBase + redirect + url; this.url = url; this.expectedResult = expectedResult; this.testMsg = testMsg; } IndividualAddonTest.prototype = { toString: function() { // something } } function addToTests(redirectBase, xpiBase, expectedResult, testMsg) { var test = new IndividualAddonTest(redirectBase, xpiBase, expectedResult, testMsg); TESTS.push(test); } (Looking a little later in run_test_2, it seems the expected results are slightly different. Which means you can just add two arguments to the constructor for "second pass" results, or define an ExpectedResults() constructor.) >+function test() { >+ waitForExplicitFinish(); >+ >+ run_test_1(); >+} >+ >+function end_test() { ... >+} >+ >+function badCertListener(host, bits) { >+ this.host = host; >+ this.bits = bits; >+} >+ >+badCertListener.prototype = { ... >+} >+ >+// Add overrides for the bad certificates >+function addCertOverrides() { ALL of this is repeated in browser_updatessl.js. Suggest writing a head_AOM.js file to capture the common parts, >+ var req = new XMLHttpRequest(); >+ try { >+ req.open("GET", "https://nocert.example.com/" + xpi, false); >+ req.channel.notificationCallbacks = new badCertListener("nocert.example.com", >+ Ci.nsICertOverrideService.ERROR_MISMATCH); >+ req.send(null); >+ } >+ catch (e) { } Uh, no. Empty catch blocks are useless, dangerous, and can sometimes hide exceptions you really want to propagate. At the very least, there should be a comment saying "// do nothing", indicating the empty catch is deliberate. Far better to specify the types of exceptions you expect: try { } catch (e if (e instanceof Ci.nsIException) && (e.result == NS_ERROR_WHATEVER)) { // do nothing, this is expected } catch (e) { // uh oh! abort(); } Also, how about: const COS = Ci.nsICertOverrideService; var callbacks = new badCertListener("nocert.example.com", COS.ERROR_MISMATCH); req.channel.notificationCallbacks = callbacks; That'll shorten the lines significantly. addCertOverrides() looks like it can be broken up into two functions: function requestCertOverrides(URL, ERRTYPE) { var req = new XMLHttpRequest(); try { req.open("GET", "https://" + URL + xpi, false); req.channel.notificationCallbacks = new badCertListener(URL, ERRTYPE); req.send(null); } catch (e) { // do nothing, we expect all errors here, move along } } function addCertOverrides() { const COS = Ci.nsICertOverrideService; requestCertOverrides("nocert.example.com/", COS.ERROR_MISMATCH); requestCertOverrides("self-signed.example.com/", COS.ERROR_UNTRUSTED); // ... } >+ [0, "Should have seen no failure for a http install url"], >+ [AddonManager.DOWNLOADERROR_NETWORK, "Should have seen a failure for a https install url from a non built-in CA"], I really hate magic numbers. Can we explicitly define a value, AddonManager.DOWNLOAD_SUCCESS = 0? browser_updatessl.js is so similar to this that I wonder if we can combine that file with this one. (But don't go out of your way to do it.)
Attachment #433280 -
Flags: feedback-
Comment 21•14 years ago
|
||
Comment on attachment 433281 [details] [diff] [review] xpcshelltests rev 1 This is way too big for me to review all in one swallow. I think I'll need a few days to go through all of this. >+++ b/toolkit/mozapps/extensions/test/addons/test_bootstrap1/bootstrap.js >@@ -0,0 +1,9 @@ >+Components.utils.import("resource://gre/modules/Services.jsm"); >+ >+function enable() { >+ Services.prefs.setIntPref("bootstraptest.version", 1); >+} >+ >+function disable() { >+ Services.prefs.setIntPref("bootstraptest.version", 0); >+} Can you write a brief comment here explaining where the bootstrap part of the API is, and who calls enable/disable? >+++ b/toolkit/mozapps/extensions/test/addons/test_bootstrap1/install.rdf >+ <em:type>64</em:type> What does em:type 64 mean? A XML comment here explaining the type would be very helpful. >+++ b/toolkit/mozapps/extensions/test/addons/test_install1/install.rdf >+ <em:id>addon1@tests.mozilla.org</em:id> >+ <em:version>1.0</em:version> >+++ b/toolkit/mozapps/extensions/test/addons/test_install2/install.rdf >+ <em:id>addon2@tests.mozilla.org</em:id> >+ <em:version>2.0</em:version> >+++ b/toolkit/mozapps/extensions/test/addons/test_install3/install.rdf >+ <em:id>addon2@tests.mozilla.org</em:id> >+ <em:version>3.0</em:version> This is a little confusing. Why does test_install3 have the same ID as test_install2? If it's deliberate, a comment to that effect would be very helpful. >+++ b/toolkit/mozapps/extensions/test/addons/test_install4/install.rdf >+ <em:targetApplication> >+ <Description> >+ <em:id>xpcshell@tests.mozilla.org</em:id> >+ <em:minVersion>0</em:minVersion> >+ <em:maxVersion>0</em:maxVersion> >+ </Description> >+ </em:targetApplication> Add a XML comment explaining what's expected here? I think you mean "This extension should never be installed." >+++ b/toolkit/mozapps/extensions/test/xpcshell/data/test_migrate.rdf This whole file looks auto-generated. Could you safely add XML comments to each section, explaining what it means? >+++ b/toolkit/mozapps/extensions/test/xpcshell/data/test_updatecheck.rdf >+ <em:updateLink>https://localhost:4444/addons/test1.xpi</em:updateLink> ... >+ <em:updateLink>https://localhost:4444/addons/test2.xpi</em:updateLink> ... >+ <em:updateLink>https://localhost:4444/addons/test2.xpi</em:updateLink> ... >+ <em:updateLink>https://localhost:4444/addons/test1.xpi</em:updateLink> You mention the same URL for different updateLinks on updatecheck1@tests.mozilla.org more than once. XML comments explaining why? >+ <em:signature>MIGTMA0GCSqGSIb3DQEBBQUAA4GBAMO1O2gwSCCth1GwYMgscfaNakpN40PJfOWt >+ ub2HVdg8+OXMciF8d/9eVWm8eH/IxuxyZlmRZTs3O5tv9eWAY5uBCtqDf1WgTsGk >+ jrgZow1fITkZI7w0//C8eKdMLAtGueGfNs2IlTd5P/0KH/hf1rPc1wUqEqKCd4+L >+ BcVq13ad</em:signature> The first one of these might warrant a comment explaining why it's there (and not in preceding RDF blocks) and what it means. >+ <em:updateHash>sha1:78fc1d2887eda35b4ad2e3a0b60120ca271ce6e6</em:updateHash> Again, a comment explaining why it's here and not in other RDF blocks. >+++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js >+ * Portions created by the Initial Developer are Copyright (C) 2009 Wrong year. >+var gInternalManager = null Nit: Missing semicolon. >+function createAppInfo(id, name, version, platformVersion) { >+ gAppInfo = { >+ invalidateCachesOnRestart: function invalidateCachesOnRestart() { }, Why the empty function? What interface does it come from? (Actually, it'd be really nice to specify what values here belong to which interfaces, with a quick one-line comment and a blank line separating blocks of interface-required properties.) >+ QueryInterface: function QueryInterface(iid) { >+ if (iid.equals(Components.interfaces.nsIXULAppInfo) >+ || iid.equals(Components.interfaces.nsIXULRuntime) >+ || iid.equals(Components.interfaces.nsISupports)) >+ return this; >+ >+ throw Components.results.NS_ERROR_NO_INTERFACE; >+ } Earlier, you defined: >+const AM_Cc = Components.classes; >+const AM_Ci = Components.interfaces; Reuse those constants here? Or better yet, use XPCOMUtils to generate the QI? >+function startupManager(expectedRestarts, appChanged) { This function is reused in a number of places. Please JavaDoc, particularly since the implementation suggests each argument is optional. >+ if (restart) { >+ if (expectedRestarts !== undefined) >+ restartManager(expectedRestarts - 1); I strongly suggest checking the arguments (and the typeof each argument) before executing any code here. If isNaN(expectedResults), you're going to hit a stack overflow error (startupManager calls restartManager(NaN), which calls startupManager(NaN)...) >+ else if (expectedRestarts !== undefined) { >+ if (expectedRestarts > 0) >+ do_throw("Expected to need to restart " + expectedRestarts + " more times"); >+ else if (expectedRestarts < 0) >+ do_throw("Restarted " + (-expectedRestarts) + " more times than expected"); >+ } >+} Again, it's just good to check that typeof expectedRestarts == "number" somewhere. (Current tests might not violate that rule, but tests as-yet unwritten could.) >+function restartManager(expectedRestarts, newVersion) { This function is reused in a number of places. Please JavaDoc, particularly since the implementation suggests each argument is optional. Check your arguments again, please. >+function loadRegistry(appChanged) { >+ function readDirectories(parser, section) { ... >+ var parser = factory.createINIParser(file); >+ gRegistry.extensions = readDirectories(parser, "ExtensionDirs"); >+ gRegistry.themes = readDirectories(parser, "ThemeDirs"); >+} Any particular reason why you have readDirectories inside loadRegistry, and pass in the parser value as an argument? One or the other, but not both. >+function writeInstallRDFToDir(data, dir) { This function definitely needs JavaDoc'ing. >+ var rdf = "<?xml version=\"1.0\"?>\n"; >+ rdf += "<RDF xmlns=\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\" xmlns:em=\"http://www.mozilla.org/2004/em-rdf#\">\n"; >+ rdf += "<Description about=\"urn:mozilla:install-manifest\">\n"; Have you thought about using single quotes, instead of escaped double quotes? >+ ["id", "version", "type", "internalName", "updateURL", "updateKey", >+ "optionsURL", "aboutURL", "iconURL"].forEach(function(prop) { >+ if (prop in data) >+ rdf += "<em:" + prop + ">" + data[prop] + "</em:" + prop + ">\n"; >+ }); >+ rdf += writeLocaleStrings(data); >+ if ("targetApplications" in data) { New line just before this line, please. >+ data.targetApplications.forEach(function(app) { >+ rdf += "<em:targetApplication><Description>\n"; >+ ["id", "minVersion", "maxVersion"].forEach(function(prop) { >+ if (prop in app) >+ rdf += "<em:" + prop + ">" + app[prop] + "</em:" + prop + ">\n"; >+ }); >+ rdf += "</Description></em:targetApplication>\n"; >+ }); >+ } >+ if ("requires" in data) { New line just before this line, please. >+ data.requires.forEach(function(app) { >+ rdf += "<em:requires><Description>\n"; >+ ["id", "minVersion", "maxVersion"].forEach(function(prop) { >+ if (prop in app) >+ rdf += "<em:" + prop + ">" + app[prop] + "</em:" + prop + ">\n"; >+ }); >+ rdf += "</Description></em:requires>\n"; >+ }); >+ } New line just before this line, please. >+ fos.init(file, 0x02 | 0x08 | 0x20, 0644, 0); If you could add a /* PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE */ line here, that would help. (That's one thing I hate about nsIFileOutputStream: you always have to look up the codes to know what they mean.) >+var gDirSvc = AM_Cc["@mozilla.org/file/directory_service;1"]. >+ getService(AM_Ci.nsIProperties); const, perhaps? Or something to get from Services.jsm? Also, there's a number of places you use let, and some places you use var. I don't yet understand the semantic differences between them. All I ask is that you check for consistency's sake that let is used where let should be, and var is used where var should be. >+function registerDirectory(key, dir) { ... >+ QueryInterface: function(iid) { >+ if (iid.equals(AM_Ci.nsIDirectoryServiceProvider) || >+ iid.equals(AM_Ci.nsISupports)) { >+ return this; >+ } >+ throw Components.results.NS_ERROR_NO_INTERFACE; >+ } Another candidate for XPCOMUtils.generateQI? >+var AddonListener = { ... >+var InstallListener = { These sound important enough to be constants. >+function hasFlag(bits, flag) { >+ return (bits & flag) != 0; >+} Suggestion: function hasFlag(bits, flag) { return Boolean(bits & flag); } >+// Verifies that all the expected events for all add-ons were seen >+function check_test() { The name of this function is awfully generic. (I misunderstood what it was when I saw it later.) How about ensure_test_completed? >+// Need to create and register a profile folder. >+var gProfD = do_get_profile().QueryInterface(AM_Ci.nsILocalFile); const? >+var gPrefs = AM_Cc["@mozilla.org/preferences;1"]. >+ getService(AM_Ci.nsIPrefBranch); >+// Enable more extensive EM logging >+gPrefs.setBoolPref("extensions.logging.enabled", true); Services.jsm? >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js >+var profileDir = gProfD.clone(); >+profileDir.append("extensions"); const? >+function check_test_1() { >+ AddonManager.getAddon("bootstrap1@tests.mozilla.org", function(b1) { >+ do_check_neq(b1, null); typeof b1 != "undefined"? >+ do_check_eq(b1.version, "1.0"); >+ do_check_false(b1.appDisabled); >+ do_check_false(b1.userDisabled); >+ do_check_true(b1.isActive); >+ do_check_eq(getActivatedVersion(), 1); >+ do_check_true(b1.hasResource("install.rdf")); >+ do_check_true(b1.hasResource("bootstrap.js")); >+ do_check_false(b1.hasResource("foo.bar")); >+ >+ let dir = profileDir.clone(); >+ dir.append("bootstrap1@tests.mozilla.org"); >+ dir.append("bootstrap.js"); >+ let uri = Services.io.newFileURI(dir).spec; >+ do_check_eq(b1.getResourceURL("bootstrap.js"), uri); Is it worth checking that bootstrap.js exists and won't throw an exception for a syntax error? Actually, I'd like to see tests written for bootstrap.js failure conditions: * where bootstrap.js throws an exception in enable, * where bootstrap.js throws an exception in disable, * where bootstrap.js just has a syntax error and can't be parsed * where bootstrap.js throws an exception before enable and disable are defined * where bootstrap.js throws an exception after enable and disable are defined * where bootstrap.js throws an exception after enable is defined, but before disable is defined. Let's make sure enable and disable will both function (in a sandbox?) before installing or enabling the add-on. >+// Tests that a restart shuts down and restarts the add-on >+function run_test_5() { >+ AddonManager.getAddon("bootstrap1@tests.mozilla.org", function(b1) { ... >+ do_check_false(isExtensionInRegistry(profileDir, b1.id)); Huh? Why would this be false? If the extension's enabled and in-place after a restart, why would it not be in the registry somewhere? (Note: I may simply be misunderstanding the state of the add-on at this point.) >+// Test that a bootstrapped extension dropped into the profile loads properly >+// on startup and doesn't cause an EM restart >+function run_test_8() { ... >+ AddonManager.getAddon("bootstrap1@tests.mozilla.org", function(b1) { ... >+ run_test_9(); >+ }); >+} At this point, I wonder what the point of having all these "numbered" tests is. If you ever have to introduce something between test 8 and test 9, either there's renumbering involved, or a run_test_8_1. Why not just have a run_next_test() generic call, and register each of these tests in a sequence, with less generic function names? (This might also allow reuse of a couple test functions: if test 3 and test 4 cancel each other out, test 2 might be re-run to ensure we're still in a good state.) >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_checkcompatibility.js >+ * Portions created by the Initial Developer are Copyright (C) 2009 Wrong year. >+// This verifies that the extensions.checkCompatibility.* preferences work. >+ >+var ADDONS = [{ >+ id: "addon1@tests.mozilla.org", >+ version: "1.0", >+ name: "Test 1", >+ targetApplications: [{ >+ id: "unknown@tests.mozilla.org", >+ minVersion: "1", >+ maxVersion: "1" >+ }] >+}, { Each of these add-ons needs a comment line indicating whether it would be normally enabled or not. >+ AddonManager.getAddons(["addon1@tests.mozilla.org", >+ "addon2@tests.mozilla.org", >+ "addon3@tests.mozilla.org", >+ "addon4@tests.mozilla.org", >+ "addon5@tests.mozilla.org"],function(addons) { ... >+ AddonManager.getAddons(["addon1@tests.mozilla.org", >+ "addon2@tests.mozilla.org", >+ "addon3@tests.mozilla.org", >+ "addon4@tests.mozilla.org", >+ "addon5@tests.mozilla.org"],function(addons) { ... >+ AddonManager.getAddons(["addon1@tests.mozilla.org", >+ "addon2@tests.mozilla.org", >+ "addon3@tests.mozilla.org", >+ "addon4@tests.mozilla.org", >+ "addon5@tests.mozilla.org"],function(addons) { ... >+ AddonManager.getAddons(["addon1@tests.mozilla.org", >+ "addon2@tests.mozilla.org", >+ "addon3@tests.mozilla.org", >+ "addon4@tests.mozilla.org", >+ "addon5@tests.mozilla.org"],function(addons) { >+ do_test_finished(); You shouldn't need to indent four times, and to have four-deep inline functions like this. In addition: >+ let addon = addons.shift(); >+ do_check_neq(addon, null); >+ do_check_false(addon.isActive); >+ do_check_false(addon.isCompatible); >+ addon = addons.shift(); >+ do_check_neq(addon, null); >+ do_check_false(addon.isActive); >+ do_check_false(addon.isCompatible); >+ addon = addons.shift(); >+ do_check_neq(addon, null); >+ do_check_false(addon.isActive); >+ do_check_false(addon.isCompatible); >+ addon = addons.shift(); >+ do_check_neq(addon, null); >+ do_check_true(addon.isActive); >+ do_check_true(addon.isCompatible); >+ addon = addons.shift(); >+ do_check_neq(addon, null); >+ do_check_true(addon.isActive); >+ do_check_true(addon.isCompatible); It's confusing what the expected results after each pass on each addon should be. This test is ugly. Unfortunately, my suggestion (below) is not much prettier. Store properties on each member of ADDONS what the expected result should be after the first pass, then after the second pass, etc: const ADDONS = [ { id: "addon1@tests.mozilla.org", version: "1.0", name: "Test 1", targetApplications: [{ id: "unknown@tests.mozilla.org", minVersion: "1", maxVersion: "1" }], expectedResultsAfterGetAddons: [ {isActive: false, isCompatible: false}, /* ... */ ] }, { /* ... */ } ]; var TEST_CALLBACKS = [ function() { // Disable compatibility checks gPrefs.setBoolPref("extensions.checkCompatibility.2.2", false); restartManager(0); }, function() { gPrefs.setBoolPref("extensions.checkCompatibility.2.1a", false); restartManager(1, "2.1a4"); }, function() { gPrefs.setBoolPref("extensions.checkCompatibility.2.1a", true); restartManager(0); }, do_test_finished ]; function run_test() { do_test_pending(); createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "2.2.3", "2"); ADDONS.forEach(function(a) { let dest = profileDir.clone(); dest.append(a.id); writeInstallRDFToDir(a, dest); }); startupManager(1); function run_addon_checks(addonsList, testIndex) { for (var j = 0; j < ADDONS.length; j++) { let expectedResults = ADDONS[j].expectedResultsAfterGetAddons[testIndex]; let addon = addonsList[j]; do_check_neq(addon, null); // check addon ID? do_check_eq(addon.isActive, expectedResults.isActive, "some message with testIndex, j here"); do_check_eq(addon.isCompatible, expectedResults.isCompatible, "some message with testIndex, j here"); } } const addonsByID = ["addon1@tests.mozilla.org", "addon2@tests.mozilla.org", "addon3@tests.mozilla.org", "addon4@tests.mozilla.org", "addon5@tests.mozilla.org"]; for (var i = 0; i < TEST_CALLBACKS.length; i++) { AddonManager.getAddons(addonsByID, function(addonsList) { run_addon_checks(addonsList, i); TEST_CALLBACKS[i](); }); } } There's a tradeoff, in that you sacrifice the line numbers in your version (which point to a specific test failure) for storing expected results in the ADDONS object collection directly in my version. Perhaps a happy medium exists between the two versions. My main gripes are that you shouldn't need four-deep inline functions for this test, and it's hard to tell what each add-on's individual state should be at any given time. I really think you should rewrite this whole test for added clarity on each add-on's state machine. >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_disable.js >+ * Portions created by the Initial Developer are Copyright (C) 2009 Wrong year. >+var addon1 = { >+ id: "addon1@tests.mozilla.org", >+ version: "1.0", >+ name: "Test 1", >+ iconURL: "chrome://foo/content/icon.png", >+ targetApplications: [{ >+ id: "xpcshell@tests.mozilla.org", >+ minVersion: "1", >+ maxVersion: "1" >+ }] >+}; >+ >+var profileDir = gProfD.clone(); >+profileDir.append("extensions"); >+ >+// Sets up the profile by installing an add-on. >+function run_test() { >+ do_check_true(hasFlag(a1.permissions, AddonManager.PERM_CANDISABLE)); >+ do_check_false(hasFlag(a1.permissions, AddonManager.PERM_CANENABLE)); Nit: Suggest renaming these variables to AddonManager.PERM_CAN_DISABLE, AddonManager.PERM_CAN_ENABLE. >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_general.js >+ * Portions created by the Initial Developer are Copyright (C) 2009 Wrong year. >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_install.js >+ * Portions created by the Initial Developer are Copyright (C) 2009 Wrong year. >+function run_test() { >+ // Make sure we only register once despite multiple calls >+ AddonManager.addInstallListener(InstallListener); >+ AddonManager.addAddonListener(AddonListener); >+ AddonManager.addInstallListener(InstallListener); >+ AddonManager.addAddonListener(AddonListener); How are you you testing that here? addInstallListener doesn't throw NS_ERROR_ALREADY_INITIALIZED, I presume. Does it return a special value that you're not checking here? Without poking into the internals of AddonManager, you really have no way of knowing InstallListener is in the AddonManager's registry exactly one time. >+// Checks that an install proceeds as expected >+function run_test_1() { >+ AddonManager.getInstallForFile(do_get_addon("test_install1"), function(install) { ... >+ AddonManager.getInstalls(null, function(installs) { >+ do_check_eq(installs.length, 1); >+ do_check_eq(installs[0], install); ... >+ install.install(); >+ }); >+ }); Does anyone here think it might be possible to confuse install with installs, or installs[0], or install.install? Give your function arguments more distinct names, please. >+function check_test_1() { >+ check_test(); >+ AddonManager.getAddon("addon1@tests.mozilla.org", function(a1) { ... >+ AddonManager.getAddonsWithPendingOperations(null, function(list) { ... >+ AddonManager.getInstalls(null, function(installs) { >+ AddonManager.getAddon("addon1@tests.mozilla.org", function(a1) { ... Here we go again with the four-levels-deep nesting. Also, a1 is now defined twice in this scope - once by the innermost function (where we're executing now), and once by the function just inside check_test_1. I recommend renaming one of these variables - even though JS is smart enough to know what you mean. A developer reading this might not be that smart. >+ // Should have been installed sometime in the last second. >+ do_check_true((Date.now() - a1.installDate.getTime()) < 1000); >+ do_check_false((Date.now() - a1.installDate.getTime()) < 0); Date.now() could conceivably change between these two test lines. (When we run these xpcshell tests several times a day... this makes random failures more probable.) var timeElapsed = Date.now() - a1.installDate.getTime(); do_check_true(timeElapsed < 1000); do_check_false(timeElapsed < 0); >+function check_test_2(install) { >+ // Pause the install here and start it again in run_test_3 >+ do_execute_soon(function() { run_test_3(install) }); >+ return false Nit: missing semicolon. Looking over this file, I can't help but think: what would happen if a test failed at check_test_7 in this file? The do_throw stack trace would be VERY long indeed. Other super-deeply-nested, multi-functions-deep tests could easily spew dozens of lines of xpcshell stack traces. Heck, if you keep this up, you might find yourself accidentally discovering how deep a JS stack can be - and how far is too far. do_execute_soon looks like an escape hatch for excessively long stack traces. I think you might consider using it a lot more - both in this file and in other tests I've already reviewed here. You don't have to - if anything, this allows for one of the heavier JSENG tests on "how deep can we go" - but when it comes time to debug, ouch. >+function check_test_3() { ... >+ AddonManager.getAddon("addon2@tests.mozilla.org", function(a2) { ... >+ // Should have been installed sometime in the last second. >+ do_check_true((Date.now() - a2.updateDate.getTime()) < 1000); >+ do_check_false((Date.now() - a2.updateDate.getTime()) < 0); var timeElapsed = Date.now() - a1.installDate.getTime(); do_check_true(timeElapsed < 1000); do_check_false(timeElapsed < 0); >+function run_test_4() { >+ AddonManager.getInstallForURL(url, function(install) { >+ AddonManager.getInstalls(null, function(installs) { >+ do_check_eq(installs.length, 1); >+ do_check_eq(installs[0], install); >+ do_check_eq(install.existingAddon, null); >+ >+ prepare_test({}, [ >+ "onDownloadStarted", >+ "onDownloadEnded", >+ ], check_test_4); >+ install.install(); >+ }); Give your function arguments more distinct names, please. >+function check_test_5() { >+ AddonManager.getAddon("addon2@tests.mozilla.org", function(a2) { >+ AddonManager.getInstalls(null, function(installs) { >+ AddonManager.getAddon("addon2@tests.mozilla.org", function(a2) { Recommend renaming one of the "a2" variables here. >+function run_test_6() { >+ AddonManager.getInstallForURL(url, function(install) { >+ do_check_neq(install, null); >+ do_check_eq(install.version, "1.0"); >+ do_check_eq(install.name, "Real Test 4"); >+ do_check_eq(install.state, AddonManager.STATE_AVAILABLE); >+ AddonManager.getInstalls(null, function(installs) { >+ do_check_eq(installs.length, 1); >+ do_check_eq(installs[0], install); >+ >+ prepare_test({}, [ >+ "onDownloadStarted", >+ "onDownloadEnded", >+ ], check_test_6); >+ install.install(); >+ }); Give your function arguments more distinct names, please. >+function check_test_7() { >+ AddonManager.getAddon("addon3@tests.mozilla.org", function(a3) { >+ AddonManager.getInstalls(null, function(installs) { >+ do_check_eq(installs, 0); >+ AddonManager.getAddon("addon3@tests.mozilla.org", function(a3) { Recommend renaming one of the "a3" variables here. >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_manifest.js I'm going to stop here for now. I'm guessing this is a little short of the half-way mark.
Comment 22•14 years ago
|
||
Regarding use of single quotes https://developer.mozilla.org/en/JavaScript_style_guide Prefer double quotes, except in in-line event handlers or when quoting double quotes.
Comment 23•14 years ago
|
||
Dave, those are all patches collected from other existing bugs?
Assignee | ||
Comment 24•14 years ago
|
||
(In reply to comment #23) > Dave, those are all patches collected from other existing bugs? Other bugs and the initial work that wasn't tracked in any particular bug. Bugs that have been fixed since then are marked [needs-review]
Comment 25•14 years ago
|
||
Ok, as talked on IRC we will have to scan the UI work for creating Litmus tests. For now lets add the in-litmus? flag so we don't forget that one.
Status: NEW → ASSIGNED
Flags: in-testsuite?
Flags: in-litmus?
Comment 26•14 years ago
|
||
Comment on attachment 433275 [details] [diff] [review] base rev 1 >+ >+ /** >+ * Gets an AddonInstall for an nsIFile. >+ * @param file >+ * the file the add-on is located at >+ * @param callback >+ * A callback to pass the AddonInstall to >+ * @param mimetype >+ * An optional mimetype hint for the add-on >+ */ >+ getInstallForFile: function AM_getInstallForFile(file, callback, mimetype) { >+ if (!file || !callback) >+ throw new TypeError("Invalid arguments"); >+ >+ // Freeze the list of providers then call each in turn till one gives a >+ // valid Install >+ let providers = this.providers.slice(0); >+ >+ function callNextProvider() { >+ if (providers.length == 0) >+ return safeCall(callback, null); >+ >+ let provider = providers.shift(); >+ if ("getInstallForFile" in provider) { >+ callProvider(provider, "getInstallForFile", null, file, function(install) { >+ if (install) >+ safeCall(callback, install); >+ else >+ callNextProvider(); >+ }); >+ } >+ else { >+ callNextProvider(); >+ } >+ } >+ >+ callNextProvider(); >+ }, nit: as we discussed, this pattern repeats itself a lot. Not sure if there is a better way though but you could likely create a helper that created a copy of all of the providers you are interested in outside of callNextProvider. >+ /** >+ * Checkes whether a particular source is allowed to install add-ons of a >+ * given mimetype. >+ * @param mimetype >+ * The mimetype of the add-on >+ * @param uri >+ * The uri of the source, may be null >+ * @returns true if the source is allowed to install these types of add-on >+ */ >+ isInstallAllowed: function AM_isInstallAllowed(mimetype, uri) { >+ for (let i = 0; i < this.providers.length; i++) { >+ if (callProvider(this.providers[i], "supportsMimetype", false, mimetype)) >+ return callProvider(this.providers[i], "isInstallAllowed", null, uri); >+ } >+ }, >+ >+ /** >+ * Starts installation of a set of AddonInstalls notifying the registered >+ * web install listener of blocked or started installs. nit: what is a 'set'? Is it really an array? >+ * @param datasource >+ * The datasource the container is in >+ * @param root >+ * The RDF Resource which is the root of the container. >+ * @returns The nsIRDFContainer, initialized at the root. Really? >+ */ >+ startInstallation: function AM_startInstallation(mimetype, source, uri, installs) { >+ if (!("@mozilla.org/extensions/web-install-listener;1" in Cc)) { >+ WARN("No web installer available, cancelling all installs"); >+ installs.forEach(function(install) { >+ install.cancel(); >+ }); >+ return; >+ } >+ >+ try { >+ let weblistener = Cc["@mozilla.org/extensions/web-install-listener;1"]. >+ getService(Ci.extIWebInstallListener); when would this fail with the previous check in Cc? >+ >+ if (!this.isInstallAllowed(mimetype, uri)) { >+ if (weblistener.onWebInstallBlocked(source, uri, installs, installs.length)) { nit: is there a reason this if isn't part of the parent if? >+ installs.forEach(function(install) { nit: anonymous funtions should be named here and elsewhere >+ install.install(); >+ }); >+ } >+ } >+ else if (weblistener.onWebInstallRequested(source, uri, installs, installs.length)) { >+ installs.forEach(function(install) { >+ install.install(); >+ }); >+ } >+ } >+ catch (e) { >+ WARN("Failure calling web installer: " + e); >+ installs.forEach(function(install) { >+ install.cancel(); >+ }); >+ return; >+ } >+ }, This try block is covering a lot of cases without it being clear how it would fail. Can it be broken up to only cover the cases where it would fail or at least commented to state when / how it would throw? >+ >+ /** >+ * Adds a new InstallListener. nit: should also note that it removes it if it exists... same in other parts of this patch. Is there a reason you don't check if it is already present and then do nothing? >+ * @param listener >+ * The InstallListener to add >+ */ >+ addInstallListener: function AM_addInstallListener(listener) { >+ this.removeInstallListener(listener); >+ this.installListeners.push(listener); >+ }, >+ >+ /** >+ * Removes an InstallListener. >+ * @param listener >+ * The InstallListener to remove >+ */ >+ removeInstallListener: function AM_removeInstallListener(listener) { >+ this.installListeners = this.installListeners.filter(function(i) { >+ return i != listener; >+ }); >+ }, >+ >+ /** >+ * Gets an add-on with a specific ID. >+ * @param id >+ * The ID of the add-on to retrieve >+ * @param callback >+ * The callback to pass the retrieved add-on to >+ */ >+ getAddon: function AM_getAddon(id, callback) { >+ if (!id || !callback) >+ throw new TypeError("Invalid arguments"); >+ >+ // Freeze the list of providers then call each in turn till one gives a >+ // valid Install >+ let providers = this.providers.slice(0); >+ >+ function callNextProvider() { >+ if (providers.length == 0) >+ return safeCall(callback, null); >+ >+ try { >+ providers.shift().getAddon(id, function(addon) { >+ if (addon) >+ safeCall(callback, addon); >+ else >+ callNextProvider(); >+ }); >+ } >+ catch (e) { >+ ERROR("Exception calling provider.getAddon: " + e); >+ callNextProvider(); >+ } >+ } >+ >+ callNextProvider(); >+ }, >+ >+ /** >+ * Gets a set of add-ons. nit: what is a 'set'? Is it really an array? Here and elsewhere. >+ * @param ids >+ * An array of IDs to retrieve >+ * @param callback >+ * The callback to pass an array of Addons to >+ */ >+ getAddons: function AM_getAddon(ids, callback) { >... >+var AddonManager = { >+ STATE_AVAILABLE: 0, >+ STATE_DOWNLOADING: 1, >+ STATE_CHECKING: 2, >+ STATE_DOWNLOADED: 3, >+ STATE_DOWNLOAD_FAILED: 4, >+ STATE_INSTALLING: 5, >+ STATE_INSTALLED: 6, >+ STATE_INSTALL_FAILED: 7, >+ STATE_CANCELLED: 8, >+ >+ INSTALLERROR_INVALID_VERSION: -1, >+ INSTALLERROR_INVALID_GUID: -2, >+ INSTALLERROR_INCOMPATIBLE_VERSION: -3, >+ INSTALLERROR_INCOMPATIBLE_PLATFORM: -5, >+ INSTALLERROR_BLOCKLISTED: -6, >+ INSTALLERROR_INSECURE_UPDATE: -7, >+ INSTALLERROR_INVALID_MANIFEST: -8, >+ INSTALLERROR_RESTRICTED: -9, >+ INSTALLERROR_SOFTBLOCKED: -10, >+ >+ DOWNLOADERROR_NETWORK: -1, >+ DOWNLOADERROR_INCORRECT_HASH: -2, >+ DOWNLOADERROR_CORRUPT_FILE: -3, >+ >+ UPDATE_WHEN_USER_REQUESTED: 1, >+ UPDATE_WHEN_NEW_APP_DETECTED: 2, >+ UPDATE_WHEN_NEW_APP_INSTALLED: 3, >+ UPDATE_WHEN_PERIODIC_UPDATE: 16, >+ UPDATE_WHEN_ADDON_INSTALLED: 17, >+ >+ PENDING_ENABLE: 1, >+ PENDING_DISABLE: 2, >+ PENDING_UNINSTALL: 4, >+ PENDING_INSTALL: 8, >+ >+ PERM_CANUNINSTALL: 1, >+ PERM_CANENABLE: 2, >+ PERM_CANDISABLE: 4, >+ PERM_CANUPGRADE: 8, It isn't clear whether comments for their meaning will be added elsewhere or if they should be added here. If the comments are added elsewhere then there should be a comment here pointing to the location for the comments. How about prefixing all params with a in all patches? Several of the lines in this patch are over 80 and can easily be continued on the next line. safeCall which is utilized heavily by the get* functions doesn't return a value if it throws in the try catch. get* functions should return what they claim to get yet safeCall doesn't always return a value and the get* functions don't have a @return comment... found this very confusing to grok
Comment 27•14 years ago
|
||
Comment on attachment 433275 [details] [diff] [review] base rev 1 >+/** >+ * This interface is used to allow various parts of the platform to talk to >+ * AddonManager. >+ */ >+[scriptable, uuid(e9ea95bb-096f-4158-a8da-bedb2a52f1b0)] >+interface extIManager : nsISupports >+{ >+ /** >+ * Checks if installation is enabled for a source. Please elaborate what a source is >+ * @param mimetype >+ * The mimetype of add-on to be installed mimetype for the addon? >+ * @param referer >+ * The URL of the source trying to install an add-on >+ * @returns true if installation is enabled >+ */ >+ boolean isInstallEnabled(in AString mimetype, in nsIURI referer); >+ >+ /** >+ * Starts installing a new add-on Is 'new' superfluous or can only add-ons not already installed be installed? singular yet everywhere else plural >+ * @param mimetype >+ * The mimetype of the add-ons >+ * @param source >+ * The window installing the add-ons >+ * @param referer >+ * The URI of the site installing the add-ons for the site >+ * @param uris >+ * The URIs of add-ons to be installed for the add-ons >+ * @param hashes >+ * The hashes of add-ons to be installed for the add-ons >+ * @param names >+ * The names of add-ons to be installed of the add-ons >+ * @param icons >+ * The icons of add-ons to be installed for the add-ons >+ * @param callback >+ * A callback to notify about installation success and failure I believe this should denote that this param is optional >+ * @param count >+ * The number of add-ons to install I believe this should denote that this param is optional >+ * @returns true if the installation was successfully started >+ */ >+ boolean startInstall(in AString mimetype, in nsIDOMWindowInternal source, in nsIURI referer, referer on the next line >+ [array, size_is(count)] in wstring uris, >+ [array, size_is(count)] in wstring hashes, >+ [array, size_is(count)] in wstring names, >+ [array, size_is(count)] in wstring icons, >+ [optional] in extIInstallCallback callback, >+ [optional] in PRUint32 count); >+}; start at first glance seems superfluous in startInstall. Can this be better named? nit: how about installCount instead of count?
Updated•14 years ago
|
Attachment #433275 -
Flags: review?(robert.bugzilla) → review-
Comment 28•14 years ago
|
||
Comment on attachment 433275 [details] [diff] [review] base rev 1 Many of the files have an ext prefix... should they be prefixed with addonmgr or something similar now that it handles more than just extensions? >+ >+/** >+ * This component serves as integration between the platform and AddonManager. >+ * It is responsible for initialisation and shutdown as well as a couple of >+ * other things. >+ */ >+ >+ const Cc = Components.classes; extra space >+const Ci = Components.interfaces; >+const Cr = Components.results; >+ >+const PREF_EM_UPDATE_INTERVAL = "extensions.update.interval"; >+ >+// The old XPInstall error codes >+const UNSUPPORTED_TYPE = -244; >+const CANT_READ_ARCHIVE = -207; >+const USER_CANCELLED = -210; >+const DOWNLOAD_ERROR = -228; >+const SUCCESS = 0; >+ >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); >+ >+var gSingleton = null; >+ >+function extManager() { >+ Components.utils.import("resource://gre/modules/AddonManager.jsm"); >+} >+ >+extManager.prototype = { >+ observe: function AMC_observe(subject, topic, data) { >+ var os = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); >+ >+ switch (topic) { >+ case "profile-after-change": >+ os.addObserver(this, "xpcom-shutdown", false); >+ AddonManagerInternal.startup(); >+ break; >+ case "xpcom-shutdown": >+ os.removeObserver(this, "xpcom-shutdown"); >+ AddonManagerInternal.shutdown(); >+ break; >+ } >+ }, >+ >+ /** >+ * @see extIManager.idl >+ */ >+ isInstallEnabled: function AMC_isInstallEnabled(type) { >+ return AddonManager.isInstallEnabled(type); >+ }, >+ >+ /** >+ * @see extIManager.idl >+ */ >+ startInstall: function AMC_startInstall(type, window, referer, uris, hashes, >+ names, icons, callback) { How about mimetype instead of type? >+ if (uris.length == 0) >+ return false; >+ >+ let retval = true; >+ if (!AddonManager.isInstallAllowed(type, referer)) { >+ callback = null; >+ retval = false; >+ } >+ >+ let loadgroup = null; >+ try { >+ if (window) { Since the idl requires this param and it will throw below if it is invalid is the check for window really necessary? >+ loadgroup = window.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIDocumentLoader).loadGroup; >+ } >+ } >+ catch (e) { >+ } >+ >+ let installs = []; >+ function buildNextInstall() { >+ if (uris.length == 0) { >+ AddonManager.startInstallation(type, window, referer, installs); >+ return; >+ } >+ let uri = uris.shift(); >+ AddonManager.getInstallForURL(uri, function(install) { >+ if (install) { >+ installs.push(install); >+ if (callback) { >+ install.addListener({ >+ onDownloadCancelled: function(install) { >+ callback.callback(uri, USER_CANCELLED); >+ }, callback.callback? >+ >+ onDownloadFailed: function(install, error) { >+ if (error == AddonManager.DOWNLOADERROR_CORRUPT_FILE) >+ callback.callback(uri, CANT_READ_ARCHIVE); >+ else >+ callback.callback(uri, DOWNLOAD_ERROR); >+ }, >+ >+ onInstallFailed: function(install, error) { >+ }, Why doesn't the callback get called for the install failed case? >+ >+ onInstallEnded: function(install, status) { >+ callback.callback(uri, SUCCESS); >+ } >+ }); >+ } >+ } >+ else if (callback) { >+ callback.callback(uri, UNSUPPORTED_TYPE); >+ } nit: brace consistency
Comment 29•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+/** >+ * Reads an AddonInternal object from an RDF stream. >+ * @param uri >+ * The URI that the manifest is being read from >+ * @param stream >+ * An open stream to read the RDF from >+ * @returns an AddonInternal object >+ */ >+function loadManifestFromRDF(uri, stream) { >... >+ addon.targetApplications = []; >+ targets = ds.GetTargets(root, EM_R("targetApplication"), true); >+ while (targets.hasMoreElements()) { >+ let target = targets.getNext().QueryInterface(Ci.nsIRDFResource); >+ let appinfo = {}; nit: there is already XULAppInfo... perhaps targetAppInfo? >... >+/** >+ * Creates a jar: URI for a file inside a ZIP file. >+ * @param jarfile >+ * The ZIP file >+ * @param path >+ * The path inside the ZIP file >+ * @returns an nsIURI for the file >+ */ >+function buildJarURI(jarfile, path) { >+ let uri = Services.io.newFileURI(jarfile); >+ uri = "jar:" + uri.spec + "!/" + path; >+ return NetUtil.newURI(uri); >+} >+ >+/** >+ * Extracts a ZIP's files into a directory. nit: Extracts files from a ZIP file into a directory. >+ * @param zipFile >+ * The source XPI file that contains the item. >+ * @param dir >+ * The directory to extract to. nit: state that this is an nsIFile here and elsewhere >+ */ >+function extractFiles(zipFile, dir) { >... >+ >+/** >+ * Replaces %...% strings in an addon url (update and updateInfo) with >+ * appropriate values. >+ * @param addon >+ * The AddonInternal representing the item >+ * @param appVersion >+ * The application version to retrieve information for retrieve is the wrong term >+ * @param uri >+ * The uri to escape Need to add a comment for updateType It would be good to comment on under what conditions any of these params are not present (e.g. updateType could be missing according to the code). >+ * >+ * @returns the appropriately escaped uri. >+ */ >+function escapeAddonURI(addon, appVersion, uri, updateType) >+{ >+ var itemStatus = addon.userDisabled ? "userEnabled" : "userDisabled"; This seems wrong... if userDisabled itemStatus is "userEnabled"? >+ >+ if (!addon.isCompatible) >+ itemStatus += ",incompatible"; >+ if (addon.blocklistState > 0) >+ itemStatus += ",blocklisted"; >+ >+ let xpcomABI; >+ try { >+ xpcomABI = Services.appinfo.XPCOMABI; >+ } catch (ex) { >+ xpcomABI = UNKNOWN_XPCOM_ABI; >+ } >+ >+ uri = uri.replace(/%ITEM_ID%/g, addon.id); >+ uri = uri.replace(/%ITEM_VERSION%/g, addon.version); >+ uri = uri.replace(/%ITEM_STATUS%/g, itemStatus); >+ uri = uri.replace(/%APP_ID%/g, Services.appinfo.ID); >+ uri = uri.replace(/%APP_VERSION%/g, appVersion ? appVersion : Services.appinfo.version); >+ uri = uri.replace(/%REQ_VERSION%/g, REQ_VERSION); >+ uri = uri.replace(/%APP_OS%/g, Services.appinfo.OS); >+ uri = uri.replace(/%APP_ABI%/g, xpcomABI); >+ uri = uri.replace(/%APP_LOCALE%/g, getLocale()); >+ uri = uri.replace(/%CURRENT_APP_VERSION%/g, Services.appinfo.version); >+ if (updateType) >+ uri = uri.replace(/%UPDATE_TYPE%/g, updateType); >+ let app = addon.matchingTargetApplication; >+ if (app) >+ uri = uri.replace(/%ITEM_MAXAPPVERSION%/g, app.maxVersion); This was always present before... under what conditions will it not exist now? >+ >+ // Replace custom parameters (names of custom parameters must have at >+ // least 3 characters to prevent lookups for something like %D0%C8) >+ var catMan = null; >+ uri = uri.replace(/%(\w{3,})%/g, function(match, param) { >+ if (!catMan) { >+ catMan = Cc["@mozilla.org/categorymanager;1"]. >+ getService(Ci.nsICategoryManager); >+ } >+ >+ try { >+ var contractID = catMan.getCategoryEntry(CATEGORY_UPDATE_PARAMS, param); >+ var paramHandler = Cc[contractID]. >+ getService(Ci.nsIPropertyBag2); >+ return paramHandler.getPropertyAsAString(param); >+ } >+ catch(e) { >+ return match; >+ } >+ }); >+ >+ // escape() does not properly encode + symbols in any embedded FVF strings. >+ return uri.replace(/\+/g, "%2B"); >+}
Comment 30•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+/** >+ * Copies properties from one object to another. If no target object is passed >+ * a new object will be created and returned. >+ * @param object >+ * An object to copy from >+ * @param properties >+ * An array of properties to be copied >+ * @param target >+ * An optional target object to bopy the properties to I have a new favorite word! bopy! Here and below >+/** >+ * A helpful wrapper around the prefs service that allows for default values >+ * when requested values aren't set. >+ */ >+var Prefs = { >+ /** >+ * Gets a preference from the default branch ignoring user-set values. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @returns the default value of the preference or defaultValue if there is none >+ */ >+ getDefaultCharPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getDefaultBranch("").getCharPref(name); >+ } >+ catch (e) { >+ return defaultValue; >+ } iirc for strict the last return should be outside of the try catch here and elsewhere >+ }, >+ >+ /** >+ * Gets a string preference. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @returns the value of the preference or defaultValue if there is none >+ */ >+ getCharPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getCharPref(name); >+ } >+ catch (e) { >+ return defaultValue; >+ } >+ }, >+ >+ /** >+ * Gets a boolean preference. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @returns the value of the preference or defaultValue if there is none >+ */ >+ getBoolPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getBoolPref(name); >+ } >+ catch (e) { >+ return defaultValue; >+ } >+ }, >+ >+ /** >+ * Gets an integer preference. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @returns the value of the preference or defaultValue if there is none >+ */ >+ getIntPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getIntPref(name); >+ } >+ catch (e) { >+ return defaultValue; >+ } >+ } >+} Consider something along the lines of the following for the three functions above though I am fine with it getPref: function(aFunc, aPrefName, aDefaultValue) { try { return Services.prefs[aFunc](aPrefName); } catch (e) { } return aDefaultValue; }
Comment 31•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+var XPIProvider = { >+ // An array of known install locations >+ installLocations: null, >+ // A dictionary of known install locations by name >+ installLocationsByName: null, >+ // An array of currently running AddonInstalls is running the proper term here? >+ installs: null, >+ // The default skin for the application >+ defaultSkin: "classic/1.0", >+ // The currently skin that is in use or will be switched to after a restart nit: 'The skin that is in use or' or 'The currently selected skin or'? >+ selectedSkin: null, >+ // The name of the checkCompatibility preference The name of the checkCompatibility preference for the application version? >+ checkCompatibilityPref: null, >+ // The value of the checkCompatibility preference >+ checkCompatibility: true, >+ // The value of the checkUpdateSecurity preference >+ checkUpdateSecurity: true, >+ // A dictionary of the file descriptors for bootstrappable add-ons by ID >+ bootstrappedAddons: {}, >+ // A dictionary of JS scopes of loaded bootstrappable add-ons by ID >+ bootstrapScopes: {}, >+ >+ /** >+ * Startups the XPI provider initialising the install locations and prefs. nit: Starts the XPI provider and initializes the install locations and prefs. >+ */ >+ startup: function XPI_startup() { >+ >+ /** >+ * Builds the current install state. This is a JS object that describes the >+ * known add-ons in the install locations and their modification times. Any >+ * change to the installed set should yield a change in this object. >+ * >+ * The returned object is an array. Each item in the array is an object >+ * representing an install location with its name and items which is a >+ * dictionary of installed items by ID. Each installed item is an object with >+ * descriptor and mtime properties. nit: I think 'name and items' should be 'name and installed items' to be consistent with 'Each installed item' or vice versa. I'd prefer a java-doc comment here I think getInstallState should be more descriptive. Perhaps getInstallLocationsItemStates or something similar though it isn't clear that id, persistentDescriptor, and lastModifiedTime represents a state. >+ */ >+ getInstallState: function XPI_getInstallState() { >+ let state = []; >+ this.installLocations.forEach(function(location) { >+ let items = location.itemLocations; >+ if (items.length == 0) >+ return; >+ let locationState = { name: location.name, items: {} }; >+ location.itemLocations.forEach(function(dir) { >+ let id = location.getIDForLocation(dir); >+ locationState.items[id] = { >+ descriptor: dir.persistentDescriptor, >+ mtime: dir.lastModifiedTime >+ }; >+ }); >+ state.push(locationState); >+ }); >+ return state; >+ }, >+ >+ /** >+ * Check the staging directories of install locations for any new items to be >+ * installed or items to be uninstalled. >+ * @param manifests >+ * A dictionary to add detected install manifests to for the purpose >+ * of passing through uypdated compatibility information nit: uypdated >+ * @returns true if an item was installed or uninstalled Seems strange for a function named processPendingInstalls to deal with uninstalled items. Please add details in the comment for when this condition occurs. >+ */ >+ processPendingInstalls: function XPI_processPendingInstalls(manifests) { >+ // TODO maybe this should be passed off to the install locations to handle? >+ let changed = false; >+ this.installLocations.forEach(function(location) { >+ manifests[location.name] = {}; >+ // We can't install anything into locked locations >+ if (location.locked) >+ return; >+ let dir = location.getStagingDir(); >+ if (!dir || !dir.exists()) >+ return; >+ let entries = dir.directoryEntries; >+ while (entries.hasMoreElements()) { >+ let stageDir = entries.getNext().QueryInterface(Ci.nsILocalFile); >+ if (!stageDir.isDirectory()) >+ continue; >+ let id = stageDir.leafName; >+ if (!gIDTest.test(id)) >+ continue; Seems like the cases where this continues should log what happened.
Comment 32•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Processes any changes to the installed set of add-ons. 'set' here and elsewhere... it isn't clear what a set is. >+ * @param state >+ * The current install state looking at state here I find it even more confusing since it is iterated in reverse order to get install location names. >+ * @param manifests >+ * A dictionary of cached AddonInstalls for items that have been >+ * installed >+ * @param updateCompatibility >+ * True if appDisabled should be re-calculated for items since an >+ * application version change has occurred. >+ * @returns true if a change requiring a restart was detected >+ */ >+ processFileChanges: function XPI_processFileChanges(state, manifests, >+ updateCompatibility, migrateData) { >+ let changed = false; >+ let knownLocations = XPIDatabase.getInstallLocations(); >+ >+ // It is important that we are iterating through the install locations >+ // in reverse order of priority. visibleItems tracks the first seen item >+ // for an ID. >+ let visibleItems = {}; >+ state.reverse().forEach(function(st) { >+ let installLocation = this.installLocationsByName[st.name]; >+ let items = st.items; >+ let pos = knownLocations.indexOf(installLocation.name); >+ if (pos >= 0) { >+ knownLocations.splice(pos, 1); >+ let addons = XPIDatabase.getAddonsInLocation(installLocation.name); nit: I think getAddonsInLocation would be better as getAddonsForLocation >+ addons.forEach(function(oldAddon) { >+ if (oldAddon.id in items) { >+ let item = items[oldAddon.id]; >+ delete items[oldAddon.id]; >+ if (oldAddon.id in manifests[installLocation.name] || >+ oldAddon.updateDate != item.mtime || >+ oldAddon._descriptor != item.descriptor) { >+ extra new line >+ // This item has changed >+ LOG("Item " + oldAddon.id + " modified in " + installLocation.name); >+ try { >+ let newAddon = manifests[installLocation.name][oldAddon.id]; >+ if (!newAddon) >+ newAddon = loadManifestFromDir(installLocation.getLocationForID(oldAddon.id)); >+ if (newAddon.id != oldAddon.id) >+ throw new Error("Incorrect id in install manifest"); Seems like the id's would be handy here and elsewhere >... >+ // No change to this item but it might have become visible >+ if (!(oldAddon.id in visibleItems)) { >+ visibleItems[oldAddon.id] = oldAddon; >+ if (!oldAddon.visible) { >+ XPIDatabase.makeAddonVisible(oldAddon); >+ if (oldAddon.type == "bootstrapped") { >+ oldAddon.active = true; >+ XPIDatabase.updateAddonActive(oldAddon); >+ this.bootstrappedAddons[oldAddon.id] = item.descriptor; >+ } >+ else { >+ changed = true; >+ } >+ } >+ } >+ >+ // App version changed, we may need to update the appDisabled bit It isn't a bit is it? ;) // App version changed, we may need to update appDisabled for the add-on >+ if (updateCompatibility) { >+ let appDisabled = !isUsableItem(oldAddon); >+ if (appDisabled != oldAddon.appDisabled) { >+ LOG("Item " + oldAddon.id + " changed appDisabled state to " + appDisabled); >+ XPIDatabase.setAddonState(oldAddon, oldAddon.userDisabled, >+ appDisabled, oldAddon.pendingUninstall); >+ if (oldAddon.visible && !oldAddon.userDisabled) { >+ if (oldAddon.type == "bootstrapped") { >+ oldAddon.active = !oldAddon.appDisabled; >+ XPIDatabase.updateAddonActive(oldAddon); >+ if (oldAddon.active) >+ this.bootstrappedAddons[oldAddon.id] = item.descriptor; >+ else >+ delete this.bootstrappedAddons[oldAddon.id]; >+ } >+ else { >+ changed = true; >+ } >+ } >+ } >+ } >+ } >+ } >+ else { >+ // This item has disappeared >+ LOG("Item " + oldAddon.id + " removed from " + installLocation.name); >+ XPIDatabase.removeAddonMetadata(oldAddon); >+ if (oldAddon.active) { I think the previous review comment about using activated vs. selected is reinforced by .active >+ >+ // We don't know about these install locations so remove the items there >+ // from the database. remove its items from the database or remove the items from these install locations from the database. It is confusing that the comment states that "We don't know about these install locations" and it is checking knownLocations. >+ knownLocations.forEach(function(location) { >+ let addons = XPIDatabase.getAddonsInLocation(location); >+ addons.forEach(function(oldAddon) { >+ LOG("Item " + oldAddon.id + " removed from " + location); >+ XPIDatabase.removeAddonMetadata(oldAddon); >+ // If this addon was previously activated then we must restart >+ if (oldAddon.active) { >+ if (oldAddon.type == "theme") >+ this.enableDefaultTheme(); >+ if (oldAddon.type == "bootstrapped") >+ delete this.bootstrappedAddons[oldAddon.id]; >+ else >+ changed = true; >+ } >+ }, this); >+ }, this); >+ >+ cache = JSON.stringify(this.getInstallState()); >+ Services.prefs.setCharPref(PREF_INSTALL_CACHE, cache); >+ return changed; >+ },
Comment 33•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Checks for any changes that have occurred since the application was last >+ * running. since the last time the application (was launched|ran) that requires a restart. >+ * @param appChanged >+ * True if the application has changed version number since the last >+ * run launch >+ * @returns true if a change requiring a restart was detected >+ */ >+ checkForChanges: function XPI_checkForChanges(appChanged) { >+ LOG("checkForChanges"); >+ >+ // First install any new items into the locations, we'll detect these when >+ // we read the install state >+ let manifests = {}; >+ let changed = this.processPendingInstalls(manifests); >+ >+ // We have to hold the DB scheme in prefs so we don't need to load the >+ // database to see if we need to migrate data >+ let schema = Prefs.getIntPref(PREF_DB_SCHEMA, 0); >+ >+ let migrateData = null; >+ let cache = null; >+ if (schema != DB_SCHEMA) { >+ // If the schema is wrong we need to read the data necessary for migration 'If the schema has changed' it should never be 'wrong' nit: 'we need to read the data necessary for migration' seems a bit off. Perhaps something along the lines of need to migrate data. >+ migrateData = XPIDatabase.migrateData(schema); >+ } >+ else { >+ // Otherwise if the database exists we can trust the previous file cache >+ let db = FileUtils.getFile(KEY_PROFILEDIR, [FILE_DATABASE], true); >+ if (db.exists()) >+ cache = Prefs.getCharPref(PREF_INSTALL_CACHE, null); >+ } >+ >+ // Load the list of bootstrapped add-ons first so processFileChanges can >+ // modify it >+ this.bootstrappedAddons = JSON.parse(Prefs.getCharPref(PREF_BOOTSTRAP_ITEMS, "{}")); >+ let state = this.getInstallState(); >+ if (appChanged || changed || cache == null || cache != JSON.stringify(state)) >+ changed = this.processFileChanges(state, manifests, appChanged, migrateData); >+ >+ // If the application crashed before completing any pending activations >+ // then we should perform them now. Is this just for pending activations or is it for any pending operations? >+ if (changed || Prefs.getBoolPref(PREF_PENDING_OPERATIONS)) { >+ LOG("Restart necessary"); >+ XPIDatabase.updateActiveAddons(); >+ Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, false); >+ return true; >+ } >+ >+ LOG("No changes found"); >+ >+ // Check that the extensions list still exists xpi add-ons list? >+ let registry = FileUtils.getFile(KEY_PROFILEDIR, [FILE_EXTENSION_REGISTRY], true); nit: seems like FILE_EXTENSION_REGISTRY would be better called FILE_XPI_ADDONS_LIST ir FILE_ADDONS_LIST per the previous comment and it is less ambiguous than registry. >+ /** >+ * Called to test whether this provider supports installing a particular >+ * mimetype. >+ * @param mimetype >+ * The mimetype to check for >+ * @returns True if the mimetype is application/x-xpinstall >+ */ >+ supportsMimetype: function XPI_supportsMimetype(type) { >+ return type == "application/x-xpinstall"; >+ }, s/type/mimetype/ >+ /** >+ * Called to test whether installing XPI add-ons is enabled. >+ * @returns True if installing is enabled nit: I believe this should be lowercase true here and elsewhere >+ /** >+ * Called to test whether installing XPI add-ons from a URI is allowed. >+ * @param uri >+ * The URI being installed from >+ * @returns True if installing is allowed >+ */ >+ isInstallAllowed: function XPI_isInstallAllowed(uri) { >+ if (!this.isInstallEnabled()) >+ return false; >+ >+ if (!uri) >+ return true; >+ >+ // file: and chrome: don't need whitelisted hosts >+ if (uri.schemeIs("chrome") || uri.schemeIs("file")) >+ return true; >+ >+ let requireWhitelist = Prefs.getBoolPref(PREF_XPI_WHITELIST_REQUIRED, true); nit: move let requireWhitelist to just before first use below. >+ >+ let permission = Services.perms.testPermission(uri, XPI_PERMISSION); >+ >+ if (permission == Ci.nsIPermissionManager.DENY_ACTION) >+ return false; >+ if (requireWhitelist && (permission != Ci.nsIPermissionManager.ALLOW_ACTION)) >+ return false; >+ return true; >+ },
Comment 34•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Called to get an AddonInstall for a URL. >+ * @param url >+ * The URL to be installed >+ * @param hash >+ * A hash for the install >+ * @param name >+ * A name for the install >+ * @param iconURL >+ * An icon for the install An icon url for the install >+ * @param version >+ * A version for the install >+ * @param loadgroup >+ * A loadgroup to associate requests with >+ * @param callback >+ * A callback to pass the AddonInstall to >+ */ >+ getInstallForURL: function XPI_getInstallForURL(url, hash, name, iconURL, >+ version, loadgroup, callback) { >+ AddonInstall.createDownload(function(install) { >+ callback(install.wrapper); >+ }, url, hash, name, iconURL, version, loadgroup); >+ }, >+ >+ /** >+ * Called to get Addons that have pending operations. >+ * @param types >+ * An array of types to fetch. Can be null to get all types >+ * @param callback >+ * A callback to pass an array of Addons to >+ */ >+ getAddonsWithPendingOperations: function XPI_getAddonsWithPendingOperations(types, callback) { >+ XPIDatabase.getVisibleAddonsWithPendingOperations(types, function(addons) { >+ let results = [createWrapper(a) for each (a in addons)]; >+ XPIProvider.installs.forEach(function(install) { >+ if (install.state == AddonManager.STATE_INSTALLED && >+ !(install.addon instanceof DBAddonInternal)) { >+ results.push(createWrapper(install.addon)); >+ } >+ }); >+ callback(results); >+ }); >+ }, >+ >+ /** >+ * Called to get the current AddonInstalls, optionally restricting by type. I seem to recall another comment that read 'optionally limiting by type'. Either one for consistency. >+ * @param types >+ * An array of types or null to get all types This is why I'd like the cases where the param is a mimetype to be mimetype instead of type. >+ * @param callback >+ * A callback to pass the array of AddonInstalls to >+ */ >+ getInstalls: function XPI_getInstalls(types, callback) { >+ callback([i.wrapper for each (i in this.installs)]); >+ }, >+ >+ /** >+ * Called when a new add-on has been enabled when only one add-on of that type >+ * can be enabled. >+ * @param id >+ * The ID of the newly selected add-on >+ * @param type >+ * The type of the newly selected add-on >+ * @param pendingRestart >+ * True if the newly enabled add-on will only become enabled after a >+ * restart >+ */ >+ addonSelected: function XPI_addonSelected(id, type, pendingRestart) { addonActivatesd or something similar as previously commented? >+ // We only care about themes in this provider >+ if (type != "theme") >+ return; >+ >+ if (!id) { >+ // Fallback to the default theme when no theme was selected >+ this.enableDefaultTheme(); >+ return; >+ } >+ >+ // Look for the previously selected theme and find the internalName of the >+ // currently selected theme >+ let previous = null; >+ let newSkin = this.defaultSkin; >+ let addons = XPIDatabase.getAddonsByType("theme"); >+ addons.forEach(function(a) { >+ if (!a.visible) >+ return; >+ if (a.id == id) >+ newSkin = a.internalName; >+ else if (a.userDisabled == false && !a.pendingUninstall) >+ previous = a; >+ }, this); >+ >+ if (pendingRestart) { >+ Services.prefs.setBoolPref(PREF_DSS_SWITCHPENDING, true); >+ Services.prefs.setCharPref(PREF_DSS_SKIN_TO_SELECT, newSkin); >+ } >+ else { >+ Services.prefs.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, newSkin); >+ } >+ this.selectedSkin = newSkin; >+ >+ // Mark the previous theme as disabled. This won't cause recursion since >+ // only enabled calls notifyAddonSelected. >+ if (previous) >+ this.updateAddonDisabledState(previous, true); >+ }, >+ >+ /** >+ * Finds and enabled the default theme, used when there is no longer any theme >+ * selected for some reason. >+ */ >+ enableDefaultTheme: function XPI_enableDefaultTheme() { >+ LOG("Activating default theme"); >+ let addons = XPIDatabase.getAddonsByType("theme"); >+ addons.forEach(function(a) { >+ if (a.visible && a.internalName == this.defaultSkin) nit: checking a.visible second would likely have less checks >+ this.updateAddonDisabledState(a, false); >+ }, this); >+ }, >+ >+ /** >+ * Notified when a preference we're interested in has changed. >+ * @see nsIObserver >+ */ >+ observe: function XPI_observe(subject, topic, data) { >+ switch (data) { >+ case this.checkCompatibilityPref: >+ case PREF_EM_CHECK_UPDATE_SECURITY: >+ this.checkCompatibility = Prefs.getBoolPref(this.checkCompatibilityPref, true); >+ this.checkUpdateSecurity = Prefs.getBoolPref(PREF_EM_CHECK_UPDATE_SECURITY, true); >+ this.updateAllAddonDisabledStates(); >+ break; >+ } >+ }, >+ >+ /** >+ * Tests whether enabling an add-on will require a restart. >+ * @param addon >+ * The add-on to test >+ * @returns true if the operation would require a restart nit: @returns true if the operation requires a restart >+ */ >+ enableRequiresRestart: function XPI_enableRequiresRestart(addon) { >+ // If the theme we're enabling is the skin currently selected then it doesn't >+ // require a restart to enable it. >+ if (addon.type == "theme") >+ return addon.internalName != Prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN); >+ return addon.type != "bootstrapped"; >+ }, Does this also get called when an add-on that is in the to be disabled state and is then enabled? >+ /** >+ * Calls a method in a bootstrap.js loaded scope logging any exceptions thrown. and logging... >+ * @param id >+ * The ID of the add-on being bootstrapped >+ * @param scope >+ * The loaded JS scope to call into >+ * @param methods >+ * An array of methods. The first method in the array that exists in >+ * the scope will be called >+ */ >+ callBootstrapMethod: function XPI_callBootstrapMethod(id, scope, methods) { >+ for (let i = 0; i < methods.length; i++) { >+ if (methods[i] in scope) { >+ LOG("Calling bootstrap method " + methods[i] + " on " + id); >+ try { >+ scope[methods[i]](id); >+ } >+ catch (e) { >+ WARN("Exception running bootstrap " + methods[i] + " on " + id + ": " + e); >+ } >+ return; >+ } >+ } >+ }, >+ >+ /** >+ * Activates a bootstrapped add-on by loading its JS scope and calling the >+ * appropriate method on it. Adds the add-on and its scope to the bootstrapScopes >+ * and bootstrappedAddons dictionaries. >+ * @param id >+ * The ID of the add-on being bootstrapped being activated. Might also mention bootstrapped >+ * @param dir >+ * The directory containing the add-on >+ * @param startup >+ * True if the add-on is being activated during startup >+ * @param install >+ * True if the add-on is being activated during installation >+ */ >+ activateAddon: function XPI_activateAddon(id, dir, startup, install) { >+ let methods = ["enable"]; >+ if (startup) >+ methods.unshift("startup"); >+ if (install) >+ methods.unshift("install"); >+ let bootstrap = dir.clone(); >+ bootstrap.append("bootstrap.js"); >+ if (bootstrap.exists()) { >+ let uri = Services.io.newFileURI(bootstrap); >+ let scope = new Components.utils.Sandbox("chrome://mozapps/content/extensions"); >+ try { >+ let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"]. >+ createInstance(Ci.mozIJSSubScriptLoader); >+ loader.loadSubScript(uri.spec, scope); >+ this.callBootstrapMethod(id, scope, methods); >+ this.bootstrappedAddons[id] = dir.persistentDescriptor; >+ this.bootstrapScopes[id] = scope; Seems like this try catch has a bit more in it than it needs. Isn't mozIJSSubScriptLoader always going to be available? Can't bootstrappedAddons and bootstrapScopes be added after the catch? >+ } >+ catch (e) { >+ WARN("Error loading bootstrap.js for " + id + ": " + e); >+ return; >+ } >+ } >+ else { >+ WARN("Bootstrap missing for " + id); Will all jetpack add-ons have a bootstrap? If not this should just be a LOG >+ } >+ }, >+ >+ /** >+ * Dectivates a bootstrapped add-on by by calling the appropriate method on >+ * the cached JS scope. Removes the add-on and its scope from the >+ * bootstrapScopes and bootstrappedAddons dictionaries. >+ * @param id >+ * The ID of the add-on being bootstrapped being deactivated >+ >+ /** >+ * Updates the appDisabled property for all add-ons. >+ */ >+ updateAllAddonDisabledStates: function XPI_updateAllAddonDisabledStates() { >+ let addons = XPIDatabase.getAddons(); >+ addons.forEach(function(addon) { >+ this.updateAddonDisabledState(addon); >+ }, this); >+ }, >+
Comment 35•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Updates the disabled state for an add-on. Its appDisabled property will be >+ * calculated and if the add-on is changed appropriate notifications will be >+ * sent out to the registered AddonListeners. >+ * @param addon >+ * The AddonInternal to update Should this be DBAddonInternal here and elsewhere? >+ * @param userDisabled >+ * A new value for the userDisabled property or undefined to not change Value for the userDisabled property. If undefined the value will not change. >+ */ >+ updateAddonDisabledState: function XPI_updateAddonDisabledState(addon, userDisabled) { >+ if (!(addon instanceof DBAddonInternal)) >+ throw new Error("Can only update addon states for installed addons."); >+ >+ let appDisabled = !isUsableItem(addon); Having this immediately before the userDisabled can play tricks on the eyes. How about moving it to before first use? >+ if (userDisabled === undefined) >+ userDisabled = addon.userDisabled; >+ >+ // No change means nothing to do here >+ if (addon.userDisabled == userDisabled && addon.appDisabled == appDisabled) >+ return; >+ >+ let wasDisabled = addon.userDisabled || addon.appDisabled; >+ let isDisabled = userDisabled || appDisabled; >+ // Update the properties in the database >+ XPIDatabase.setAddonState(addon, userDisabled, appDisabled, addon.pendingUninstall); A note about why pendingUninstall is passed when the add-on isn't being uninstalled would be helpful. >+ >+ // If the add-on is not visible or the add-on is not changing state then >+ // there is no need to do anything else >+ if (!addon.visible || (wasDisabled == isDisabled)) >+ return; >... >+var XPIDatabase = { >+ // True if the database connection has been opened >+ initialised: false, >+ // A cache of statements that are used and need to be finalized on shutdown >+ statements: {}, >+ // A cache of weak references DBAddonInternals so we can reuse objects where >+ // possible nit: weak reference DBAddonInternals? Also, using the term cache is a tad generic for the var especially since there is also the statements cache. >+ cache: [], >+
Comment 36•14 years ago
|
||
Dave, it has been a few years since I've spent any significant time on sql... what do you think of getting someone like sdwilsh to review that part?
Comment 37•14 years ago
|
||
Dave, I wanted to mention that though it is a little difficult to wrap my head around all of the changes in such a short time I really like the direction you have taken with this rewrite. I'd like to get the comments and naming polished before focusing more on the code since it is easier to understand then. In regards to using activated instead of selected, I've been looking at this code for so long that I am no longer sure that activated is better. So, please don't hesitate to push back on any comments I've made you think are wrong and I'll re-evaluate the comment.
Comment 38•14 years ago
|
||
(In reply to comment #35) > (From update of attachment 433277 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm > >new file mode 100644 > >--- /dev/null > >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm > >... > >+ /** > >+ * Updates the disabled state for an add-on. Its appDisabled property will be > >+ * calculated and if the add-on is changed appropriate notifications will be > >+ * sent out to the registered AddonListeners. > >+ * @param addon > >+ * The AddonInternal to update > Should this be DBAddonInternal here and elsewhere? Sorry about that... should have looked a bit deeper.
Comment 39•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 I skipped a bit so I can get more familiar with the code before reviewing it. >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+/** >+ * The AddonWrapper wraps an Addon to provide the data visible to public callers >+ * through the API. >+ */ >+function AddonWrapper(addon) { >+ ["id", "version", "type", "optionsURL", "aboutURL", "isCompatible", >+ "providesUpdatesSecurely", "blocklistState", "appDisabled", >+ "userDisabled"].forEach(function(prop) { >+ this.__defineGetter__(prop, function() addon[prop]); >+ }, this); >+ >+ ["installDate", "updateDate"].forEach(function(prop) { >+ this.__defineGetter__(prop, function() new Date(addon[prop])); >+ }, this); >+ >+ this.__defineGetter__("iconURL", function() { >+ return addon.active ? addon.iconURL : null; >+ }); Does this mean disabled add-ons won't display their icon which was fixed in bug 511091? >... >+ this.uninstall = function() { >+ if (!(addon instanceof DBAddonInternal)) >+ throw new Error("Cannot uninstall an item that isn't installed"); >+ XPIProvider.uninstallAddon(addon); >+ }; >+ >+ this.cancelUninstall = function() { >+ if (!(addon instanceof DBAddonInternal)) >+ throw new Error("Cannot uninstall an item that isn't installed"); The error thrown here is the same for uninstall >+ if (!addon.pendingUninstall) >+ throw new Error("Item is not marked to be uninstalled"); Does it make sense to have a similar check in uninstall to see if it is already pendingUninstall? >... >+AddonWrapper.prototype = { >+ get screenshots() { >+ return []; >+ } >+}; >+ >+/** >+ * An object which identifies an Install Location for items, where the location >+ * relationship is each item living in a directory named with its GUID under >+ * the directory used when constructing this object. It would be great if there were no mentions of GUID after the rewrite except perhaps as a subset of possible formats for ID since this supports more than just the HUID format http://en.wikipedia.org/wiki/Globally_Unique_Identifier >+ * >+ * e.g. <location>\{GUID1} >+ * <location>\{GUID2} >+ * <location>\{GUID3} >+ * ... >+ * >+ * @param name >+ * The string identifier of this Install Location. >+ * @param location >+ * The directory that contains the items. nit: typically you have not ended the sentences for @params with a period though you started to off an on below. Either is correct but consistency would be good http://java.sun.com/j2se/javadoc/writingdoccomments/ >+ * @constructor >+ */ >+function DirectoryInstallLocation(name, location, locked) { >+ this._name = name; >+ if (location.exists() && !location.isDirectory()) >+ throw new Error("Location must be a directory."); >+ >+ this.locked = locked; >+ this._location = location; >+ this._IDToDirMap = {}; >+ this._DirToIDMap = {}; >+ >+ if (this._location.exists() && this._location.isDirectory()) >+ this._readAddons(); You already check if it is a directory when it exists above and then throw so all you should have to check here is if it exists. >+ /** >+ * Finds all the items installed in this location. >+ */ >+ _readAddons: function DirInstallLocation__readAddons() { >+ try { >+ let entries = this._location.directoryEntries >+ .QueryInterface(Ci.nsIDirectoryEnumerator); >+ while (true) { >+ let entry = entries.nextFile; >+ if (!entry) >+ break; >+ entry instanceof Ci.nsILocalFile; >+ let id = entry.leafName; >+ if (id == DIR_STAGE) >+ continue; >+ if (!gIDTest.test(id)) >+ continue; Might be a good thing to log this. >+ entry = this._location.clone().QueryInterface(Ci.nsILocalFile); >+ entry.append(id); Why are you recreating the entry? At a glance it looks to be the same as the original entry. >+ if (entry.isFile()) { >+ entry = this._readDirectoryFromFile(entry); >+ if (!entry || !entry.exists()) >+ continue; Might be a good thing to log this. >+ } >+ if (!entry.isDirectory()) >+ continue; Might be a good thing to log this. >+ this._IDToDirMap[id] = entry; >+ this._DirToIDMap[entry.path] = id; >+ } >+ entries.close(); >+ } >+ catch (e) { >+ } I know that a good portion of this is from the original implementation but this is an awfully big try catch from way back when. Can you make it more specific? >... >+ /** >+ * Gets the staging directory to put items that are pending install into. >+ * @returns an nsIFile >+ */ >+ getStagingDir: function DirInstallLocation_getStagingDir() { >+ let dir = this._location.clone(); >+ dir.append(DIR_STAGE); >+ return dir; >+ }, >+ >+ /** >+ * Installs an item from a directory into the install location. >+ * @param id >+ * The ID of the item to install >+ * @param source >+ * The directory to install from >+ */ >+ installItem: function DirInstallLocation_installItem(id, source) { >+ let dir = this._location.clone().QueryInterface(Ci.nsILocalFile); >+ dir.append(id); >+ if (dir.exists()) >+ dir.remove(true); >+ source = source.clone(); Is cloning really necessary here? Seems like all of the call sites are already using a clone. >+ source.moveTo(this._location, id); >+ this._DirToIDMap[dir.path] = id; >+ this._IDToDirMap[id] = dir; >+ return dir; >+ }, You return a dir so add a java-doc @return comment >+ >+ /** >+ * Uninstalls an item from this location. >+ * @param id >+ * The ID of the item to uninstall >+ */ >+ uninstallItem: function DirInstallLocation_uninstallItem(id) { >+ let dir = this._location.clone(); >+ dir.append(id); >+ if (!dir.exists()) >+ throw new Error("Attempt to uninstall unknown item " + id); >+ dir.remove(true); >+ delete this._DirToIDMap[dir.path]; >+ delete this._IDToDirMap[id]; Could it exist in _DirToIDMap or _IDToDirMap and if it could should it be deleted from them if the dir doesn't exist? I suspect not. >+ }, >+ >+ /** >+ * Gets the ID of the item installed in the given directory. >+ * @param file Perhaps aDir since it is a directory? >+ * The directory to look in >+ * @returns the ID >+ */ >+ getIDForLocation: function DirInstallLocation_getIDForLocation(file) {
Comment 40•14 years ago
|
||
(In reply to comment #39) > (From update of attachment 433277 [details] [diff] [review]) >... > >+AddonWrapper.prototype = { > >+ get screenshots() { > >+ return []; > >+ } > >+}; > >+ > >+/** > >+ * An object which identifies an Install Location for items, where the location > >+ * relationship is each item living in a directory named with its GUID under > >+ * the directory used when constructing this object. > It would be great if there were no mentions of GUID after the rewrite except > perhaps as a subset of possible formats for ID since this supports more than > just the HUID format GUID format
Comment 41•14 years ago
|
||
(In reply to comment #30) > >+ getDefaultCharPref: function(name, defaultValue) { > >+ try { > >+ return Services.prefs.getDefaultBranch("").getCharPref(name); > >+ } > >+ catch (e) { > >+ return defaultValue; > >+ } > iirc for strict the last return should be outside of the try catch here and > elsewhere Seems fine as is. I suppose you're referring to the "does not always return a value" warning, but that function does always return a value.
Comment 42•14 years ago
|
||
(In reply to comment #41) > (In reply to comment #30) > > >+ getDefaultCharPref: function(name, defaultValue) { > > >+ try { > > >+ return Services.prefs.getDefaultBranch("").getCharPref(name); > > >+ } > > >+ catch (e) { > > >+ return defaultValue; > > >+ } > > iirc for strict the last return should be outside of the try catch here and > > elsewhere > > Seems fine as is. I suppose you're referring to the "does not always return a > value" warning, but that function does always return a value. That is what I am referring to and iirc making that very slight change which is for practical purposes the same as the existing code silences the warning.
Comment 43•14 years ago
|
||
In the error console I'm getting a warning here, as expected: function x() { try { return 1 } catch (e) {} } But not here: function x() { try { return 1 } catch (e) { return 1 } }
Assignee | ||
Comment 44•14 years ago
|
||
Going through all these comments now, will post new patches once I'm done. (In reply to comment #13) > >+/** > >+ * The UpdateChecker is responsible for retrieving the update information from > >+ * an add-on's update manifest. > nit: does the update checker really retrieve update information 'from' an > add-on's update manifest or is it from the update's remote update rdf? I'd like to move to using the terms "install manifest" and "update manifest" to describe the content install.rdf and update.rdf files. They apply equally as well when we look at switching to install.xml/update.xml or whatever in the future. > >+ let xml = request.responseXML; > >+ if (!xml || xml.documentElement.namespaceURI == XMLURI_PARSE_ERROR) { > >+ WARN("Update manifest was not valid XML"); > >+ this.notifyError(UpdateChecker.ERROR_PARSE_ERROR); > >+ return; > >+ } > >+ > >+ // We currently only know about RDF update manifests > >+ if (xml.documentElement.namespaceURI == PREFIX_NS_RDF) { > nit: I think this would be cleaner if you checked if it wasn't PREFIX_NS_RDF > and have success at the end so everything before the end is failure case. I think the way it is now will be cleaner in the future if we add support for other types of update manifest, but if you still want me to switch it around for now then I can do. > >+function matchesApplication(update, appVersion, platformVersion) { > >+ let result = false; > >+ for (let i = 0; i < update.targetApplications.length; i++) { > There have been various cases where we don't jit when using let statements > especially in loops. The following provides info on getting a log for the > aborts > http://hacks.mozilla.org/2009/07/tracemonkey-overview/ Going to run the unit tests with that after I've addressed all the comments and fix up any places I can where tracing is abandoned.
Comment 45•14 years ago
|
||
Then it has likely been fixed to no longer log an error. The two functions are equivalent though. I could go either way though I think it is clearer for functions that return a value do so at the end of the function and not within a block in a function.
Assignee | ||
Comment 46•14 years ago
|
||
(In reply to comment #15) > >+ serializeContainerItems: function RDFSerializer_serializeContainerItems(ds, container, indent) { > >+ var result = ""; > >+ var items = container.GetElements(); > >+ while (items.hasMoreElements()) { > >+ var item = items.getNext().QueryInterface(Ci.nsIRDFResource); > >+ result += indent + "<RDF:li>\n" > >+ result += this.serializeResource(ds, item, indent + this.INDENT); > >+ result += indent + "</RDF:li>\n" > >+ } > >+ return result; > >+ }, > nit: before the verification of falling off trace it would be a good thing if > let and var were used consistently in this and other files. I tend to ask > myself why did the developer use let vs. var when I see them used and when they > are used inconsistently it makes it harder to answer that question. In the new code I've generally preferred let mainly because people often talk about it as a good thing and it also slightly better fits my mental model of variables (I learned programming with block-scoped variables). The RDFSerializer code however is just a straight copy from the old nsExtensionManager.js which is why it doesn't match up so well. I'm going to sweep through the let/var issue after the rest of the comments are addressed. > >+ LOG("Requesting " + url); > >+ this.request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]. > >+ createInstance(Ci.nsIXMLHttpRequest); > >+ this.request.open("GET", url, true); > >+ this.request.channel.notificationCallbacks = new BadCertHandler(); > >+ this.request.overrideMimeType("text/xml"); > Perhaps don't remove the following which was added bug 318793 as well or an > explanation as to why it is no longer needed? > this.request.channel.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE; Addressed in follow-up bug 553385 > >+ * @param update > >+ * The available update > >+ * @param appVersion > >+ * The application version to use > >+ * @param platformVersion > >+ * The platform version to use > >+ * @returns true if the update is compatible with the application/platform > >+ */ > >+function matchesApplication(update, appVersion, platformVersion) { > nit: this could be better named since it checks whether it is compatible with > the application or the platform which is quite different than matching an > application. Went with matchesVersions which seems more descriptive without being overly long. > >+ if (app.id == Services.appinfo.ID) { > >+ return (Services.vc.compare(appVersion, app.minVersion) >= 0) && > >+ (Services.vc.compare(appVersion, app.maxVersion) <= 0); > >+ } > >+ if (app.id == TOOLKIT_ID) { > >+ result = (Services.vc.compare(platformVersion, app.minVersion) >= 0) && > >+ (Services.vc.compare(platformVersion, app.maxVersion) <= 0); > >+ } > nit: seems like there is a mix of using braces when you don't have to and not > using them. Although braces aren't necessary when there is only one statement I think it is more readable to include them when that statement spans multiple lines.
Comment 47•14 years ago
|
||
Comment on attachment 433277 [details] [diff] [review] xpiprovider rev 1 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+#ifdef XP_WIN >+/** >+ * An object that identifies the location of installed items based on entries >+ * in the Windows registry. For each application a subkey is defined that >+ * contains a set of values, where the name of each value is a GUID and the >+ * contents of the value is a filesystem path identifying a directory >+ * containing an installed item. GUID here and elsewhere >+ * @param name >+ * The string identifier of this Install Location. >+ * @param rootKey >+ * The root key (one of the ROOT_KEY_ values from nsIWindowsRegKey). >+ * @constructor >+ */ >+function WinRegInstallLocation(name, rootKey) { >+ this.locked = true; >+ this._name = name; >+ this._rootKey = rootKey; >+ this._IDToDirMap = {}; >+ this._DirToIDMap = {}; >+ >+ // Reading the registry may throw an exception, and that's ok. In error >+ // cases, we just leave ourselves in the empty state. >+ try { >+ let path = this._appKeyPath + "\\Extensions"; >+ let key = Cc["@mozilla.org/windows-registry-key;1"]. >+ createInstance(Ci.nsIWindowsRegKey); >+ key.open(this._rootKey, path, Ci.nsIWindowsRegKey.ACCESS_READ); >+ this._readAddons(key); >+ } catch (e) { >+ if (key) >+ key.close(); >+ } This should just close the key when it is successfully opened whether it throws or not. Looks like more old code that wasn't really correct. Looking back it appears that the original implementation in bug 293548 didn't close the key at all and then close was added for the case where it throws in bug 296566... meh Probably be a good thing to move at least path and key outside of the try catch. >+} >+ >+WinRegInstallLocation.prototype = { >+ _name : "", >+ _rootKey : null, >+ _IDToDirMap : null, // mapping from ID to directory object >+ _DirToIDMap : null, // mapping from directory path to ID >+ >+ /** >+ * Retrieves the path of this Application's data key in the registry. >+ */ >+ get _appKeyPath() { >+ let appVendor = Services.appinfo.vendor; >+ let appName = Services.appinfo.name; >+ >+ // XULRunner-based apps may intentionally not specify a vendor: No reason for the - or to end this with a : >+ if (appVendor != "") >+ appVendor += "\\"; Did you intentionally remove the workaround for Thundebird not providing vendor? Lanikai still doesn't provide vendor but they do add their registry keys under Mozilla.
Comment 48•14 years ago
|
||
Dave, I'm fine with your replies to comments so far... in regards to bracing I'm fine with that as long as it is consistent
Comment 49•14 years ago
|
||
btw: I suggested to Dave to hold off on checking where this code falls off of trace until after this has landed.
Assignee | ||
Comment 50•14 years ago
|
||
(In reply to comment #18) > >+const Cc = Components.classes; > >+const Ci = Components.interfaces; > >+ > >+const PREF_EM_UPDATE_ENABLED = "extensions.update.enabled"; > >+ > >+Components.utils.import("resource://gre/modules/Services.jsm"); > >+ > >+var EXPORTED_SYMBOLS = [ "AddonManager", "AddonManagerInternal" ]; > AddonManagerInternal being exported seems wrong... I understand what you are > trying to do but I think it should be renamed since it isn't internal since it > is exported. Do you think it would make sense to switch the names of AddonManagerInternal and AddonManagerPrivate? Then the one that is truly internal to the module is AddonManagerInternal while AddonManagerPrivate is the one exported that really non-platform callers should not use.
Comment 51•14 years ago
|
||
Neither is ideal though it *might* be more obvious to add-on authors if the names were switched. I don't think there is a good solution for this without removing the export but there were a couple of calls in the em code previously that were scriptable and should never be called by add-ons. So, document and name to make it clear that they shouldn't be used should suffice.
Assignee | ||
Comment 52•14 years ago
|
||
(In reply to comment #18) > >+ /** > >+ * Initialises the AddonManager, loading any known providers and initialising > nit: > Initializes - 398 matching lines in 253 files > Initialises - 12 matching lines in 10 files Bah, Americans ;) > >+ /** > >+ * Notifies all providers that an add-on has been enabled when that type of > >+ * add-on only supports a single add-on being enabled at a time. This allows > >+ * the providers to disable theirs if necessary. > notifyAddonSelected for an add-on that has been enabled is somewhat > confusing... can you come up with a better name? It might be a good thing to > provide a concrete example such as themes if they are applicable. Perhaps > notifyAddonActivated and addonActivated? I've gone with "notifyAddonChanged" since the concept is you're changing from one activated to another, however it only applies in the cases where only one add-on of a type can be enabled at a time. However I've come to the conclusion that it's a bit of a kludge and can be replaced with making the providers just listen for onEnabling etc. and work accordingly. I'd like to just keep it as-is for this review though and do that in a follow-up since I've been banging my head against trying to get that right for too long.
Assignee | ||
Comment 53•14 years ago
|
||
(In reply to comment #26) > >+ function callNextProvider() { > >+ if (providers.length == 0) > >+ return safeCall(callback, null); > >+ > >+ let provider = providers.shift(); > >+ if ("getInstallForFile" in provider) { > >+ callProvider(provider, "getInstallForFile", null, file, function(install) { > >+ if (install) > >+ safeCall(callback, install); > >+ else > >+ callNextProvider(); > >+ }); > >+ } > >+ else { > >+ callNextProvider(); > >+ } > >+ } > >+ > >+ callNextProvider(); > >+ }, > nit: as we discussed, this pattern repeats itself a lot. Not sure if there is a > better way though but you could likely create a helper that created a copy of > all of the providers you are interested in outside of callNextProvider. I've implemented a helper object that I think makes this marginally nicer, though I think the name of it is pretty sucky so take a look when I post the updated patch and let me know what you thing. > >+ /** > >+ * Checkes whether a particular source is allowed to install add-ons of a > >+ * given mimetype. > >+ * @param mimetype > >+ * The mimetype of the add-on > >+ * @param uri > >+ * The uri of the source, may be null > >+ * @returns true if the source is allowed to install these types of add-on > >+ */ > >+ isInstallAllowed: function AM_isInstallAllowed(mimetype, uri) { > >+ for (let i = 0; i < this.providers.length; i++) { > >+ if (callProvider(this.providers[i], "supportsMimetype", false, mimetype)) > >+ return callProvider(this.providers[i], "isInstallAllowed", null, uri); > >+ } > >+ }, > >+ > >+ /** > >+ * Starts installation of a set of AddonInstalls notifying the registered > >+ * web install listener of blocked or started installs. > nit: what is a 'set'? Is it really an array? > > >+ * @param datasource > >+ * The datasource the container is in > >+ * @param root > >+ * The RDF Resource which is the root of the container. > >+ * @returns The nsIRDFContainer, initialized at the root. > Really? > > >+ */ > >+ startInstallation: function AM_startInstallation(mimetype, source, uri, installs) { > >+ if (!("@mozilla.org/extensions/web-install-listener;1" in Cc)) { > >+ WARN("No web installer available, cancelling all installs"); > >+ installs.forEach(function(install) { > >+ install.cancel(); > >+ }); > >+ return; > >+ } > >+ > >+ try { > >+ let weblistener = Cc["@mozilla.org/extensions/web-install-listener;1"]. > >+ getService(Ci.extIWebInstallListener); > when would this fail with the previous check in Cc? It could fail if the component implementing it throws when being constructed or it doesn't implement extIWebInstallListener. > >+ > >+ if (!this.isInstallAllowed(mimetype, uri)) { > >+ if (weblistener.onWebInstallBlocked(source, uri, installs, installs.length)) { > nit: is there a reason this if isn't part of the parent if? If it was then the subsequent "else if" would be used in the wrong cases. > This try block is covering a lot of cases without it being clear how it would > fail. Can it be broken up to only cover the cases where it would fail or at > least commented to state when / how it would throw? It does cover 3 cases really. Either the component could throw when initializes, or onWebInstallBlocked or onWebInstallRequested could throw. I can see your point about the one block covering all of that but the result in each case is the same so I think it is cleaner as it is. I'll add a comment about it though. > How about prefixing all params with a in all patches? Uhh yeah. I see the style guide still recommends this so I guess so. > get* functions should return what they claim to get yet safeCall doesn't always > return a value and the get* functions don't have a @return comment... found > this very confusing to grok Basically all the get functions are async, so they don't return anything, they eventually call a callback and pass that the thing you're getting. Not sure if there is better wording that we should be using in these cases, maybe asyncGetAddon f.e.? But that feels needlessly long to me.
Comment 54•14 years ago
|
||
(In reply to comment #53) > (In reply to comment #26) > >... > > nit: as we discussed, this pattern repeats itself a lot. Not sure if there is a > > better way though but you could likely create a helper that created a copy of > > all of the providers you are interested in outside of callNextProvider. > > I've implemented a helper object that I think makes this marginally nicer, > though I think the name of it is pretty sucky so take a look when I post the > updated patch and let me know what you thing. I leave this as your call since it is only marginal. > >... > > This try block is covering a lot of cases without it being clear how it would > > fail. Can it be broken up to only cover the cases where it would fail or at > > least commented to state when / how it would throw? > > It does cover 3 cases really. Either the component could throw when > initializes, or onWebInstallBlocked or onWebInstallRequested could throw. I can > see your point about the one block covering all of that but the result in each > case is the same so I think it is cleaner as it is. I'll add a comment about it > though. That works and thanks! > > get* functions should return what they claim to get yet safeCall doesn't always > > return a value and the get* functions don't have a @return comment... found > > this very confusing to grok > > Basically all the get functions are async, so they don't return anything, they > eventually call a callback and pass that the thing you're getting. Not sure if > there is better wording that we should be using in these cases, maybe > asyncGetAddon f.e.? But that feels needlessly long to me. Understood... it just goes against our naming convention. While reviewing I'd see a get* function, assume it would return what it was getting, and then see it was async.
Assignee | ||
Comment 55•14 years ago
|
||
(In reply to comment #28) > (From update of attachment 433275 [details] [diff] [review]) > Many of the files have an ext prefix... should they be prefixed with addonmgr > or something similar now that it handles more than just extensions? On the one hand yes I think that would be nice, on the other I think that is too long for a file prefix, if we can come up with something shorter that makes sense then I'll switch. > >+ let loadgroup = null; > >+ try { > >+ if (window) { > Since the idl requires this param and it will throw below if it is invalid is > the check for window really necessary? The idl requires that a parameter be passed but that parameter can still be null. > >+ AddonManager.getInstallForURL(uri, function(install) { > >+ if (install) { > >+ installs.push(install); > >+ if (callback) { > >+ install.addListener({ > >+ onDownloadCancelled: function(install) { > >+ callback.callback(uri, USER_CANCELLED); > >+ }, > callback.callback? Yeah perhaps one of these should be renamed. extIInstallCallback is marked as a function interface so webpage JS can just give a normal JS function for it, XPConnect then wraps that in an object that implements extIInstallCallback whose first method will call the original webpage JS function. I've renamed this so it looks like callback.onInstallEnded. > >+ > >+ onDownloadFailed: function(install, error) { > >+ if (error == AddonManager.DOWNLOADERROR_CORRUPT_FILE) > >+ callback.callback(uri, CANT_READ_ARCHIVE); > >+ else > >+ callback.callback(uri, DOWNLOAD_ERROR); > >+ }, > >+ > >+ onInstallFailed: function(install, error) { > >+ }, > Why doesn't the callback get called for the install failed case? Fixed. > > >+ > >+ onInstallEnded: function(install, status) { > >+ callback.callback(uri, SUCCESS); > >+ } > >+ }); > >+ } > >+ } > >+ else if (callback) { > >+ callback.callback(uri, UNSUPPORTED_TYPE); > >+ } > nit: brace consistency I put braces around all blocks in an if...else statement if any one of them requires it.
Assignee | ||
Comment 56•14 years ago
|
||
(In reply to comment #29) > >+ if (updateType) > >+ uri = uri.replace(/%UPDATE_TYPE%/g, updateType); > >+ let app = addon.matchingTargetApplication; > >+ if (app) > >+ uri = uri.replace(/%ITEM_MAXAPPVERSION%/g, app.maxVersion); > This was always present before... under what conditions will it not exist now? Through the API it is possible to attempt to find updates for an add-on that has no compatibility information for either the current application or the toolkit. In this case there is no sensible ITEM_MAXVERSION to substitute. Commented to that effect.
Assignee | ||
Comment 57•14 years ago
|
||
(In reply to comment #30) > >+ * @param object > >+ * An object to copy from > >+ * @param properties > >+ * An array of properties to be copied > >+ * @param target > >+ * An optional target object to bopy the properties to > I have a new favorite word! bopy! Here and below This is what I get for writing code comments at 1am! > >+ getIntPref: function(name, defaultValue) { > >+ try { > >+ return Services.prefs.getIntPref(name); > >+ } > >+ catch (e) { > >+ return defaultValue; > >+ } > >+ } > >+} > Consider something along the lines of the following for the three functions > above though I am fine with it > getPref: function(aFunc, aPrefName, aDefaultValue) { > try { > return Services.prefs[aFunc](aPrefName); > } > catch (e) { > } > return aDefaultValue; > } I think I prefer this as is, it makes for shorter lines and accidentally passing in the wrong method name could be masked by the try...catch block.
Comment 58•14 years ago
|
||
(In reply to comment #55) > (In reply to comment #28) > > (From update of attachment 433275 [details] [diff] [review] [details]) > > Many of the files have an ext prefix... should they be prefixed with addonmgr > > or something similar now that it handles more than just extensions? > > On the one hand yes I think that would be nice, on the other I think that is > too long for a file prefix, if we can come up with something shorter that makes > sense then I'll switch. Not a big deal with all the other stuff going on but perhaps am as the prefix? > > >+ let loadgroup = null; > > >+ try { > > >+ if (window) { > > Since the idl requires this param and it will throw below if it is invalid is > > the check for window really necessary? > > The idl requires that a parameter be passed but that parameter can still be > null. Right but you have + let loadgroup = null; + try { + if (window) { + loadgroup = window.QueryInterface(Ci.nsIInterfaceRequestor) + .getInterface(Ci.nsIWebNavigation) + .QueryInterface(Ci.nsIDocumentLoader).loadGroup; + } + } + catch (e) { + } so it seems like you don't need it
Assignee | ||
Comment 59•14 years ago
|
||
(In reply to comment #31) > (From update of attachment 433277 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm > >new file mode 100644 > >--- /dev/null > >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm > >... > >+var XPIProvider = { > >+ // An array of known install locations > >+ installLocations: null, > >+ // A dictionary of known install locations by name > >+ installLocationsByName: null, > >+ // An array of currently running AddonInstalls > is running the proper term here? active seems more appropriate I think. > >+ > >+ /** > >+ * Builds the current install state. This is a JS object that describes the > >+ * known add-ons in the install locations and their modification times. Any > >+ * change to the installed set should yield a change in this object. > >+ * > >+ * The returned object is an array. Each item in the array is an object > >+ * representing an install location with its name and items which is a > >+ * dictionary of installed items by ID. Each installed item is an object with > >+ * descriptor and mtime properties. > nit: I think 'name and items' should be 'name and installed items' to be > consistent with 'Each installed item' or vice versa. > I'd prefer a java-doc comment here > I think getInstallState should be more descriptive. Perhaps I split this up into getInstallLocationStates and getItemStates with comments for both which I think makes it clearer what is going on. > >+ * @returns true if an item was installed or uninstalled > Seems strange for a function named processPendingInstalls to deal with > uninstalled items. Please add details in the comment for when this condition > occurs. Renamed it to processPendingFileChanges
Assignee | ||
Comment 60•14 years ago
|
||
(In reply to comment #32) > (From update of attachment 433277 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm > >new file mode 100644 > >--- /dev/null > >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm > >... > >+ /** > >+ * Processes any changes to the installed set of add-ons. > 'set' here and elsewhere... it isn't clear what a set is. > > >+ * @param state > >+ * The current install state > looking at state here I find it even more confusing since it is iterated in > reverse order to get install location names. I've tried to clarify this method with more comments as I know it is probably the most complex part of the code, I've also split parts of it out into their own functions for readabilities sake.
Assignee | ||
Comment 61•14 years ago
|
||
(In reply to comment #34) > >+ */ > >+ enableRequiresRestart: function XPI_enableRequiresRestart(addon) { > >+ // If the theme we're enabling is the skin currently selected then it doesn't > >+ // require a restart to enable it. > >+ if (addon.type == "theme") > >+ return addon.internalName != Prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN); > >+ return addon.type != "bootstrapped"; > >+ }, > Does this also get called when an add-on that is in the to be disabled state > and is then enabled? No, just for when an add-on is inactive and it is becoming active, on which basis maybe activateRequiresRestart might be a better name? Not totally sure. > >+ } > >+ catch (e) { > >+ WARN("Error loading bootstrap.js for " + id + ": " + e); > >+ return; > >+ } > >+ } > >+ else { > >+ WARN("Bootstrap missing for " + id); > Will all jetpack add-ons have a bootstrap? If not this should just be a LOG As things currently stand a bootstrap.js is required for these types of add-ons to do anything so we should definitely warn about a missing one so developers see it.
Assignee | ||
Comment 62•14 years ago
|
||
(In reply to comment #35) > >+ let wasDisabled = addon.userDisabled || addon.appDisabled; > >+ let isDisabled = userDisabled || appDisabled; > >+ // Update the properties in the database > >+ XPIDatabase.setAddonState(addon, userDisabled, appDisabled, addon.pendingUninstall); > A note about why pendingUninstall is passed when the add-on isn't being > uninstalled would be helpful. I've renamed setAddonState to setAddonProperties and it now takes a dictionary of properties to change for an add-on. > >+var XPIDatabase = { > >+ // True if the database connection has been opened > >+ initialised: false, > >+ // A cache of statements that are used and need to be finalized on shutdown > >+ statements: {}, > >+ // A cache of weak references DBAddonInternals so we can reuse objects where > >+ // possible > nit: weak reference DBAddonInternals? Also, using the term cache is a tad > generic for the var especially since there is also the statements cache. > > >+ cache: [], > >+ Renamed them to statementCache and addonCache.
Assignee | ||
Comment 63•14 years ago
|
||
(In reply to comment #36) > Dave, it has been a few years since I've spent any significant time on sql... > what do you think of getting someone like sdwilsh to review that part? Yep, Shawn has said he'll take a look. I'll get him to do so when I attach the next iteration of patches
Assignee | ||
Comment 64•14 years ago
|
||
(In reply to comment #39) > >+ this.__defineGetter__("iconURL", function() { > >+ return addon.active ? addon.iconURL : null; > >+ }); > Does this mean disabled add-ons won't display their icon which was fixed in bug > 511091? Right now yes, I have a follow-up bug to fix that (bug 552744). Don't think it is necessary to fix for the first trunk landing but it should be fairly simple to do anyway. > >+ entry = this._location.clone().QueryInterface(Ci.nsILocalFile); > >+ entry.append(id); > Why are you recreating the entry? At a glance it looks to be the same as the > original entry. I had to do that because I discovered bug 530188 in testing (the profile directory ends up being inside a symlink on OSX in the xpcshell tests). I need to double check whether this is still a problem though since some of the surrounding code has changed. > >+ /** > >+ * Installs an item from a directory into the install location. > >+ * @param id > >+ * The ID of the item to install > >+ * @param source > >+ * The directory to install from > >+ */ > >+ installItem: function DirInstallLocation_installItem(id, source) { > >+ let dir = this._location.clone().QueryInterface(Ci.nsILocalFile); > >+ dir.append(id); > >+ if (dir.exists()) > >+ dir.remove(true); > >+ source = source.clone(); > Is cloning really necessary here? Seems like all of the call sites are already > using a clone. Probably not necessary but I'd rather leave it in. I don't like methods that change their arguments unless that is explicitly intentional. I've seen problems with that in the past many times. > >+ uninstallItem: function DirInstallLocation_uninstallItem(id) { > >+ let dir = this._location.clone(); > >+ dir.append(id); > >+ if (!dir.exists()) > >+ throw new Error("Attempt to uninstall unknown item " + id); > >+ dir.remove(true); > >+ delete this._DirToIDMap[dir.path]; > >+ delete this._IDToDirMap[id]; > Could it exist in _DirToIDMap or _IDToDirMap and if it could should it be > deleted from them if the dir doesn't exist? I suspect not. I guess it's possible if the directory is removed by an outside application while Firefox is running. Not sure we can ever really maintain a good state in that case.
Assignee | ||
Comment 65•14 years ago
|
||
(In reply to comment #47) > >+ if (appVendor != "") > >+ appVendor += "\\"; > Did you intentionally remove the workaround for Thundebird not providing > vendor? Lanikai still doesn't provide vendor but they do add their registry > keys under Mozilla. I thought they had fixed that for some reason. Re-added.
Assignee | ||
Comment 66•14 years ago
|
||
I've updated all the code patches from the comments so far. Going to attach those now. I'll address Alex's comments in the tests and attach new test patches later. The two things I haven't done from the comments yet are switching attributes to use an "a" prefix and doing any tests on tracing.
Attachment #433273 -
Attachment is obsolete: true
Attachment #434395 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 67•14 years ago
|
||
Attachment #433274 -
Attachment is obsolete: true
Assignee | ||
Comment 68•14 years ago
|
||
Attachment #433275 -
Attachment is obsolete: true
Attachment #434397 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 69•14 years ago
|
||
Attachment #433277 -
Attachment is obsolete: true
Attachment #434398 -
Flags: review?(robert.bugzilla)
Attachment #433277 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 70•14 years ago
|
||
Attachment #433278 -
Attachment is obsolete: true
Attachment #434399 -
Flags: review?(robert.bugzilla)
Attachment #433278 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 71•14 years ago
|
||
Attachment #433279 -
Attachment is obsolete: true
Attachment #434400 -
Flags: review?(robert.bugzilla)
Attachment #433279 -
Flags: review?(robert.bugzilla)
Assignee | ||
Updated•14 years ago
|
Attachment #434398 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 72•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 Shawn, this is the patch with all the SQL in it, look down to the XPIDatabase to see the horror!
Comment 73•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 You don't do the try-finally bits right when using a statement only once. You want to do this: try { // stuff with statement } finally { stmt.finalize(); } Problem functions: * XPIDB_migrateData I think you are OK for resetting statements because of your magically wrapper helper (although using generators makes that less clear to me). Also, according to the new code style, we are supposed to be bracing for all ifs regardless if they are one liners or not. In general, you don't want to select *. Marco has dealt with a bunch of problems with that (I don't recall the details, but ask him, and he can tell you). You'd also probably be better off pulling your SQL statements into constants so they are 1) more readable, 2) in one place so it's easy to modify them when you change your schema, and 3) easier to give to someone else (like drh) when they ask for them because we've had some performance regression. You may want to add some error logging to the error console for all of your executeAsync calls. This would be pretty simple to do: const errorHandler = { handleError: function(...) { ... } } Then when you call executeAsync: stmt.executeAsync({ ... }.prototype = errorHandler); I'm 90% sure that'd work and wouldn't complicate your code. Looks good otherwise. r=sdwilsh
Attachment #434398 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 74•14 years ago
|
||
(In reply to comment #21) > (From update of attachment 433281 [details] [diff] [review]) > This is way too big for me to review all in one swallow. I think I'll need a > few days to go through all of this. > > >+++ b/toolkit/mozapps/extensions/test/addons/test_bootstrap1/bootstrap.js > >@@ -0,0 +1,9 @@ > >+Components.utils.import("resource://gre/modules/Services.jsm"); > >+ > >+function enable() { > >+ Services.prefs.setIntPref("bootstraptest.version", 1); > >+} > >+ > >+function disable() { > >+ Services.prefs.setIntPref("bootstraptest.version", 0); > >+} > > Can you write a brief comment here explaining where the bootstrap part of the > API > is, and who calls enable/disable? The preliminary API is here: https://wiki.mozilla.org/Extension_Manager:Bootstrapped_Extensions I expect to change this a little this week but not in any fundamental way. > >+++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js > >+ * Portions created by the Initial Developer are Copyright (C) 2009 > > Wrong year. The file was first written in 2009. However I'm removing the MPL and switching to public domain licenses for the tests anyway. > >+ if (restart) { > >+ if (expectedRestarts !== undefined) > >+ restartManager(expectedRestarts - 1); > > I strongly suggest checking the arguments (and the typeof each argument) before > executing any code here. If isNaN(expectedResults), you're going to hit a > stack > overflow error (startupManager calls restartManager(NaN), which calls > startupManager(NaN)...) That will only be the case if the add-on manager requests to restart an infinite number of times, in which case the test will timeout and correctly fail. If the caller passes an invalid argument the test will fail. > >+function check_test_1() { > >+ AddonManager.getAddon("bootstrap1@tests.mozilla.org", function(b1) { > >+ do_check_neq(b1, null); > > typeof b1 != "undefined"? If b1 were undefined then the do_check_neq would fail. > >+ let dir = profileDir.clone(); > >+ dir.append("bootstrap1@tests.mozilla.org"); > >+ dir.append("bootstrap.js"); > >+ let uri = Services.io.newFileURI(dir).spec; > >+ do_check_eq(b1.getResourceURL("bootstrap.js"), uri); > > Is it worth checking that bootstrap.js exists and won't throw an exception for > a > syntax error? > > Actually, I'd like to see tests written for bootstrap.js failure conditions: > * where bootstrap.js throws an exception in enable, > * where bootstrap.js throws an exception in disable, > * where bootstrap.js just has a syntax error and can't be parsed > * where bootstrap.js throws an exception before enable and disable are > defined > * where bootstrap.js throws an exception after enable and disable are defined > * where bootstrap.js throws an exception after enable is defined, but before > disable is defined. I think it's worthwhile to have these tests, but I'm going to file a follow-up bug for them. I can't immediately see how we could test these cases, in fact I haven't deceided what should happen in those cases. Right now the code just ignores any exceptions or missing methods so we'd somehow have to test that the bootstrap extension did nothing when loaded. I've filed bug 554805. > Let's make sure enable and disable will both function (in a sandbox?) before > installing or enabling the add-on. Not sure what you are suggesting here. > >+// Tests that a restart shuts down and restarts the add-on > >+function run_test_5() { > >+ AddonManager.getAddon("bootstrap1@tests.mozilla.org", function(b1) { > ... > >+ do_check_false(isExtensionInRegistry(profileDir, b1.id)); > > Huh? Why would this be false? If the extension's enabled and in-place after a > restart, why would it not be in the registry somewhere? No, bootstrapped extensions don't appear in extensions.ini since we don't want the platform to load any components or chrome included in them. > At this point, I wonder what the point of having all these "numbered" tests is. > If you ever have to introduce something between test 8 and test 9, either > there's renumbering involved, or a run_test_8_1. Why not just have a > run_next_test() generic call, and register each of these tests in a sequence, > with less generic function names? (This might also allow reuse of a couple > test > functions: if test 3 and test 4 cancel each other out, test 2 might be re-run > to ensure we're still in a good state.) I'd like to hear what Rob thinks about this. On the one hand I can see good reasons for doing that, on the other I started doing it and ended up with stupidly long names for the first few functions I did which doesn't seem helpful. Tests can always change number if necessary, though adding to the end would generally be better, and a short name in error logs is easier for me to read, a comment should be with each test to say what it is doing. Shared tests that check a specific state should be in more generally named functions I agree. > There's a tradeoff, in that you sacrifice the line numbers in your version > (which point to a specific test failure) for storing expected results in the > ADDONS object collection directly in my version. Perhaps a happy medium exists > between the two versions. There is, we can have a single state checking function as the different cases vary only slightly. > >+// Sets up the profile by installing an add-on. > >+function run_test() { > >+ do_check_true(hasFlag(a1.permissions, AddonManager.PERM_CANDISABLE)); > >+ do_check_false(hasFlag(a1.permissions, AddonManager.PERM_CANENABLE)); > > Nit: Suggest renaming these variables to AddonManager.PERM_CAN_DISABLE, > AddonManager.PERM_CAN_ENABLE. Maybe, Rob? > >+function run_test() { > >+ // Make sure we only register once despite multiple calls > >+ AddonManager.addInstallListener(InstallListener); > >+ AddonManager.addAddonListener(AddonListener); > >+ AddonManager.addInstallListener(InstallListener); > >+ AddonManager.addAddonListener(AddonListener); > > How are you you testing that here? addInstallListener doesn't throw > NS_ERROR_ALREADY_INITIALIZED, I presume. Does it return a special value that > you're not checking here? > > Without poking into the internals of AddonManager, you really have no way of > knowing InstallListener is in the AddonManager's registry exactly one time. If the second attempt registered the listener a second time then it would receive every event twice and so we'd see test failures because the received events did not match the expected events. > >+function check_test_1() { > >+ check_test(); > >+ AddonManager.getAddon("addon1@tests.mozilla.org", function(a1) { > ... > >+ AddonManager.getAddonsWithPendingOperations(null, function(list) { > ... > >+ AddonManager.getInstalls(null, function(installs) { > >+ AddonManager.getAddon("addon1@tests.mozilla.org", function(a1) { > ... > > Here we go again with the four-levels-deep nesting. Also, a1 is now defined > twice in this scope - once by the innermost function (where we're executing > now), > and once by the function just inside check_test_1. I recommend renaming one of > these variables - even though JS is smart enough to know what you mean. A > developer reading this might not be that smart. This is kind of the state of things with asynchronous code. I'm not sure what you're advocating as an alternative unless it is split the function up into lots of small functions, but really I think this is a lot more readable as code execution essentially flows from one line to the next. > > >+ // Should have been installed sometime in the last second. > >+ do_check_true((Date.now() - a1.installDate.getTime()) < 1000); > >+ do_check_false((Date.now() - a1.installDate.getTime()) < 0); > > Date.now() could conceivably change between these two test lines. (When we run > these xpcshell tests several times a day... this makes random failures more > probable.) I can't see how that could affect the test result. > var timeElapsed = Date.now() - a1.installDate.getTime(); > do_check_true(timeElapsed < 1000); > do_check_false(timeElapsed < 0); > > >+function check_test_2(install) { > >+ // Pause the install here and start it again in run_test_3 > >+ do_execute_soon(function() { run_test_3(install) }); > >+ return false > > Nit: missing semicolon. > > Looking over this file, I can't help but think: what would happen if a test > failed at check_test_7 in this file? The do_throw stack trace would be VERY > long indeed. Other super-deeply-nested, multi-functions-deep tests could > easily > spew dozens of lines of xpcshell stack traces. Heck, if you keep this up, you > might find yourself accidentally discovering how deep a JS stack can be - and > how far is too far. This isn't generally the case. Aside from a few exceptions the async methods are truly asynchronous so the JS stack gets emptied and started afresh. In your specific case of check_test_7 for example an error at say the a3.uninstall() call shows a stack of just 5 frames (not including the frames inside the xpcshell harness files).
Assignee | ||
Comment 75•14 years ago
|
||
Attachment #433280 -
Attachment is obsolete: true
Attachment #434785 -
Flags: review?(robert.bugzilla)
Attachment #433280 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 76•14 years ago
|
||
Attachment #433281 -
Attachment is obsolete: true
Attachment #434786 -
Flags: review?(robert.bugzilla)
Attachment #433281 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 77•14 years ago
|
||
Attachment #433282 -
Attachment is obsolete: true
Attachment #434787 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 78•14 years ago
|
||
(In reply to comment #73) > Also, according to the new code style, we are supposed to be bracing for all > ifs regardless if they are one liners or not. I can't find this documented anywhere. The only semi-formal style guide I can find is https://developer.mozilla.org/en/JavaScript_style_guide which approves no braces for single lines.
Comment 79•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/AddonManager.jsm >@@ -0,0 +1,820 @@ >+/* >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is the Extension Manager. >+# >+# The Initial Developer of the Original Code is >+# Mozilla Foundation. nit: per Gerv http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html It should be "the Mozilla Foundation." >+ >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+ >+const PREF_EM_UPDATE_ENABLED = "extensions.update.enabled"; >+ >+Components.utils.import("resource://gre/modules/Services.jsm"); >+ >+var EXPORTED_SYMBOLS = [ "AddonManager", "AddonManagerPrivate" ]; >+ >+// A list of providers to load by default >+const PROVIDERS = [ >+ "resource://gre/modules/XPIProvider.jsm", >+ "resource://gre/modules/LightweightThemeManager.jsm" >+]; >+ >+/** >+ * Calls a callback method consuming any thrown exception. Any parameters after >+ * the callback parameter will be passed to the callback. nit: not a big deal but java docs have an empty comment line between the description and params, etc. If you fix this please do so in all of the patchs. >+ * @param callback >+ * The callback method to call >+ */ >+function safeCall(callback) { >+ var args = Array.slice(arguments, 1); >+ try { >+ callback.apply(null, args); >+ } >+ catch (e) { >+ WARN("Exception calling callback: " + e); >+ } >+} >+ >+/** >+ * Calls a method on a provider if it exists and consumes any thrown exception. >+ * Any parameters after the dflt parameter are passed to the provider's method. >+ * @param provider >+ * The provider to call >+ * @param method >+ * The method name to call >+ * @param dflt >+ * A default return value if the provider does not implement the named >+ * method or throws an error. >+ * @return the return value from the provider or dflt if the provider does not >+ * implement method or throws an error >+ */ >+function callProvider(provider, method, dflt) { >+ if (!(method in provider)) >+ return dflt; nit: sometimes you have a newline and other you don't for this code pattern here and elsewhere >+ var args = Array.slice(arguments, 3); >+ try { >+ return provider[method].apply(provider, args); >+ } >+ catch (e) { >+ ERROR("Exception calling provider." + method + ": " + e); >+ return dflt; >+ } >+} >+ >+function AsyncObjectCaller(objects, method, listener) { Intercaps and add a function header comment. >+ this.objects = objects.slice(0); >+ this.method = method; >+ this.listener = listener; >+ >+ this.callNext(); >+} >+ >+AsyncObjectCaller.prototype = { >+ objects: null, >+ method: null, >+ listener: null, >+ >+ callNext: function() { >+ if (this.objects.length == 0) { >+ this.listener.noMoreObjects(this); >+ return; >+ } >+ >+ let object = this.objects.shift(); >+ if (!this.method || this.method in object) Please add a comment about the use of this.method being undefined / null or add it to the function header comment. >+ this.listener.nextObject(this, object); >+ else >+ this.callNext(); >+ } >+}; >+ >+/** >+ * This is the real manager, kept here rather than in AddonManager to keep its >+ * contents hidden from API users. >+ */ >+var AddonManagerInternal = { >... >+ /** >+ * Registers a new AddonProvider. >+ * @param provider >+ * The provider to register >+ */ >+ registerProvider: function AM_registerProvider(provider) { >+ this.providers.push(provider); >+ >+ // If we're registering after startup call this provider's startup immediately. nit: no need for the word immediately and it puts this over 80 >+ if (this.started) >+ callProvider(provider, "startup"); >+ }, >+ >+ /** >+ * Shuts down the addon manager and all registered providers, this must clean >+ * up everything in order for automated tests to fake restarts. >+ */ >+ shutdown: function AM_shutdown() { >+ this.providers.forEach(function(provider) { >+ callProvider(provider, "shutdown"); >+ }); >+ >+ this.installListeners = null; >+ this.addonListeners = null; >+ this.started = false; >+ }, >+ >+ /** >+ * Performs a background update check by starting an update for all add-ons >+ * that are available to update. nit: this comment seems a bit confusing. Perhaps Performs a background update check for all add-ons that can be updated. >+ */ >+ backgroundUpdateCheck: function AM_backgroundUpdateCheck() { >+ if (!Services.prefs.getBoolPref(PREF_EM_UPDATE_ENABLED)) >+ return; >+ this.getAddonsByType(null, function getAddonsCallback(addons) { >+ addons.forEach(function BUC_forEachCallback(addon) { >+ if (addon.permissions & AddonManager.PERM_CANUPGRADE) { >+ addon.findUpdates({ >+ onUpdateAvailable: function BUC_onUpdateAvailable(addon, install) { >+ install.install(); >+ } >+ }, AddonManager.UPDATE_WHEN_PERIODIC_UPDATE); >+ } >+ }); >+ }); >+ }, >+ >+ /** >+ * Calls all registered InstallListeners with an event. Any parameters after >+ * the additional parameter are passed to the listener. >+ * @param method >+ * The method on the listeners to call >+ * @param additional How about extraListeners or something similar for the param? >+ * An array of InstallListeners to also call nit: An array of InstallListeners to call in addition to the registered InstallListeners >+ * @return false if any of the listeners returned false, true otherwise >+ */ >+ callInstallListeners: function AM_callInstallListeners(method, additional) { >+ let result = true; >+ let listeners = this.installListeners; >+ if (additional) >+ listeners = additional.concat(listeners); nit: if the order they are called is not important I'd prefer that the registered InstallListeners are called first >+ let args = Array.slice(arguments, 2); >+ listeners.forEach(function(listener) { >+ try { >+ if (method in listener) { >+ if (listener[method].apply(listener, args) === false) >+ result = false; >+ } >+ } >+ catch (e) { >+ WARN("InstallListener threw exception when calling " + method + ": " + e); >+ } >+ }); >+ return result; >+ }, >+ >+ /** >+ * Calls all registered AddonListeners with an event. Any parameters after >+ * the method parameter are passed to the listener. >+ * @param method >+ * The method on the listeners to call >+ */ >+ callAddonListeners: function AM_callInstallListeners(method) { AM_callInstallListeners already used by callInstallListeners s/AM_callInstallListeners/AM_callAddonListeners/ >+ var args = Array.slice(arguments, 1); >+ this.addonListeners.forEach(function(listener) { >+ try { >+ if (method in listener) >+ listener[method].apply(listener, args); >+ } >+ catch (e) { >+ WARN("AddonListener threw exception when calling " + method + ": " + e); >+ } >+ }); >+ }, >+ >+ /** >+ * Notifies all providers that an add-on has been enabled when that type of >+ * add-on only supports a single add-on being enabled at a time. This allows >+ * the providers to disable theirs if necessary. >+ * @param id >+ * The id of the enabled add-on >+ * @param type >+ * The type of the enabled add-on >+ * @param pendingRestart >+ * A boolean indicating if the change will only take place the next >+ * time the application is restarted >+ */ >+ notifyAddonChanged: function AM_notifyAddonChanged(id, type, pendingRestart) { >+ this.providers.forEach(function(provider) { >+ callProvider(provider, "addonChanged", null, id, type, pendingRestart); >+ }); >+ }, >+ >+ /** >+ * Gets an AddonInstall for a URL. nit: Probably be a good thing to change the beginning of these comments for all of the async get* calls s/Gets/Asynchronously gets/ >+ * @param url >+ * The url the add-on is located at >+ * @param callback >+ * A callback to pass the AddonInstall to >+ * @param mimetype >+ * The mimetype of the add-on >+ * @param hash >+ * An optional hash of the add-on >+ * @param name >+ * An optional placeholder name while the add-on is being downloaded >+ * @param iconURL >+ * An optional placeholder icon URL while the add-on is being downloaded >+ * @param version >+ * An optional placeholder version while the add-on is being downloaded >+ * @param loadgroup >+ * An optional nsILoadGroup to associate any network requests with >+ * @throws if the url, callback or mimetype arguments are invalid >+ */ >+ getInstallForURL: function AM_getInstallForURL(url, callback, mimetype, hash, >+ name, iconURL, version, >+ loadgroup) { >+ if (!url || !mimetype || !callback) >+ throw new TypeError("Invalid arguments"); >+ >+ for (let i = 0; i < this.providers.length; i++) { >+ if (callProvider(this.providers[i], "supportsMimetype", false, mimetype)) { >+ callProvider(this.providers[i], "getInstallForURL", null, >+ url, hash, name, iconURL, version, loadgroup, >+ function(install) { >+ safeCall(callback, install); >+ }); >+ return; >+ } >+ } >+ safeCall(callback, null); >+ }, >+ >+ /** >+ * Gets an AddonInstall for an nsIFile. >+ * @param file >+ * the file the add-on is located at nit: the nsIFile where the add-on is located >+ * @param callback >+ * A callback to pass the AddonInstall to >+ * @param mimetype >+ * An optional mimetype hint for the add-on >+ * @throws if the file or callback arguments are invalid not sure if this comment is technically correct since you don't verify that file is an nsIFile... could just change it to if they are not specified here and elsewhere.
Comment 80•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >+ /** >+ * Gets an array of add-ons. >+ * @param ids >+ * An array of IDs to retrieve >+ * @param callback >+ * The callback to pass an array of Addons to >+ * @throws if the id or callback arguments are invalid >+ */ >+ getAddons: function AM_getAddon(ids, callback) { s/AM_getAddon/AM_getAddons/ I went through all of the AM_'s and these were the only two that I've found.
Comment 81•14 years ago
|
||
(In reply to comment #74) > (In reply to comment #21) > > At this point, I wonder what the point of having all these "numbered" tests is. > > If you ever have to introduce something between test 8 and test 9, either > > there's renumbering involved, or a run_test_8_1. Why not just have a > > run_next_test() generic call, and register each of these tests in a sequence, > > with less generic function names? (This might also allow reuse of a couple > > test > > functions: if test 3 and test 4 cancel each other out, test 2 might be re-run > > to ensure we're still in a good state.) > > I'd like to hear what Rob thinks about this. On the one hand I can see good > reasons for doing that, on the other I started doing it and ended up with > stupidly long names for the first few functions I did which doesn't seem > helpful. Tests can always change number if necessary, though adding to the end > would generally be better, and a short name in error logs is easier for me to > read, a comment should be with each test to say what it is doing. Shared tests > that check a specific state should be in more generally named functions I > agree. IMO as far as naming function in tests go I am fine for the most part with whatever the author of the test decides is easier for them to maintain and I'm pretty much just happy with having the tests. Renumbering tests functions doesn't happen that often and is usually very simple to do. > > >+// Sets up the profile by installing an add-on. > > >+function run_test() { > > >+ do_check_true(hasFlag(a1.permissions, AddonManager.PERM_CANDISABLE)); > > >+ do_check_false(hasFlag(a1.permissions, AddonManager.PERM_CANENABLE)); > > > > Nit: Suggest renaming these variables to AddonManager.PERM_CAN_DISABLE, > > AddonManager.PERM_CAN_ENABLE. > > Maybe, Rob? For myself, I'm fine either way but looking in mxr it appears there are more of _CAN_, _CANT_, _CANNOT_, etc. than there are _CAN*, etc.
Comment 82•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm >... >+ */ >+ getInstallForFile: function AM_getInstallForFile(file, callback, mimetype) { >+ if (!file || !callback) >+ throw new TypeError("Invalid arguments"); How about Cr.NS_ERROR_INVALID_ARG here and elsewhere? >+ >+ new AsyncObjectCaller(this.providers, "getInstallForFile", { >+ nextObject: function(caller, provider) { >+ callProvider(provider, "getInstallForFile", null, file, >+ function(install) { >+ if (install) >+ safeCall(callback, install); >+ else >+ caller.callNext(); >+ }); >+ }, >+ >+ noMoreObjects: function(caller) { >+ safeCall(callback, null); >+ } >+ }); >+ }, >+ >+ /** >+ * Gets all current AddonInstalls optionally limiting to a list of types. >+ * @param types >+ * An optional array of types to retrieve Please add what a 'type' is here and elsewhere... we know what this means but a reader of the comment would have to investigate more than should be necessary. >+ * @param callback >+ * A callback which will be passed an array of AddonInstalls >+ * @throws if the callback argument is not invalid nit: just noticed that when you changed @returns to @return you didn't remove the additional space that is no longer required between @param comment, @throws comment, @return comment. >+ */ >+ getInstalls: function AM_getInstalls(types, callback) { >+ if (!callback) >+ throw new TypeError("Invalid arguments"); >+ >+ let installs = []; >+ >+ new AsyncObjectCaller(this.providers, "getInstalls", { >+ nextObject: function(caller, provider) { >+ callProvider(provider, "getInstalls", null, types, >+ function(providerInstalls) { >+ installs = installs.concat(providerInstalls); >+ caller.callNext(); >+ }); >+ }, >+ >+ noMoreObjects: function(caller) { >+ safeCall(callback, installs); >+ } >+ }); >+ }, >+ >+ /** >+ * Checks whether installation is enabled for a particular mimetype. >+ * @param mimetype >+ * The mimetype to check >+ * @return true if installation is enabled for the mimetype >+ */ >+ isInstallEnabled: function AM_isInstallEnabled(mimetype) { >+ for (let i = 0; i < this.providers.length; i++) { >+ if (callProvider(this.providers[i], "supportsMimetype", false, mimetype)) >+ return callProvider(this.providers[i], "isInstallEnabled"); >+ } >+ return false; >+ }, Is there protection against multiple providers from supporting the same mimetype? Specifically in relation to isInstallEnabled can a provider state it supports a mimetype and state that install is not enabled while another supports the same mimetype and return state that install is enabled? >+ /** >+ * Checkes whether a particular source is allowed to install add-ons of a >+ * given mimetype. >+ * @param mimetype >+ * The mimetype of the add-on >+ * @param uri >+ * The uri of the source, may be null >+ * @return true if the source is allowed to install these types of add-on >+ */ >+ isInstallAllowed: function AM_isInstallAllowed(mimetype, uri) { >+ for (let i = 0; i < this.providers.length; i++) { >+ if (callProvider(this.providers[i], "supportsMimetype", false, mimetype)) >+ return callProvider(this.providers[i], "isInstallAllowed", null, uri); >+ } >+ }, Same thing here and elsewhere but in this case can a provider support a mimetype and not allow installing for source specified while another that supports the same mimetype does installing for the same source? >+ /** >+ * Starts installation of an array of AddonInstalls notifying the registered >+ * web install listener of blocked or started installs. >+ * @param mimetype >+ * The mimetype of add-ons being installed >+ * @param source >+ * The nsIDOMWindowInternal that started the installs >+ * @param uri >+ * the nsIURI that started the installs >+ * @param installs >+ * An array of AddonInstalls to be installed >+ */ >+ startInstallation: function AM_startInstallation(mimetype, source, uri, >+ installs) { nit: don't really like the name. >+ >+ /** >+ * Gets an add-on with a specific ID. >+ * @param id >+ * The ID of the add-on to retrieve >+ * @param callback >+ * The callback to pass the retrieved add-on to >+ * @throws if the id or callback arguments are invalid nit: not specified since it doesn't check for validity... here and elsewhere. >+ */ >+ getAddon: function AM_getAddon(id, callback) { >+ if (!id || !callback) >+ throw new TypeError("Invalid arguments"); >+ >+ new AsyncObjectCaller(this.providers, "getAddon", { >+ nextObject: function(caller, provider) { >+ callProvider(provider, "getAddon", null, id, function(addon) { >+ if (addon) >+ safeCall(callback, addon); >+ else >+ caller.callNext(); >+ }); >+ }, >+ >+ noMoreObjects: function(caller) { >+ safeCall(callback, null); >+ } >+ }); >+ }, >+ >+ /** >+ * Gets an array of add-ons. >+ * @param ids >+ * An array of IDs to retrieve >+ * @param callback >+ * The callback to pass an array of Addons to s/an array/the array/ - here and elsewhere >+ * @throws if the id or callback arguments are invalid >+ */ >+ getAddons: function AM_getAddon(ids, callback) { >+ if (!ids || !callback) >+ throw new TypeError("Invalid arguments"); >+ >+ let addons = []; >+ >+ new AsyncObjectCaller(ids, null, { >+ nextObject: function(caller, id) { >+ AddonManagerInternal.getAddon(id, function(addon) { >+ addons.push(addon); >+ caller.callNext(); >+ }); >+ }, >+ >+ noMoreObjects: function(caller) { >+ safeCall(callback, addons); >+ } >+ }); >+ }, >+ >+ /** >+ * Gets add-ons of specific types. >+ * @param types >+ * An array of types to retrieve or null to retrieve all types >+ * @param callback >+ * The callback to pass an array of Addons to. >+ * @throws if the callback argument is invalid >+ */ >+ getAddonsByType: function AM_getAddonsByType(types, callback) { Should probably be getAddonsByTypes since it takes an array of types
Comment 83•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm >... >+ /** >+ * Gets add-ons that have operations waiting for an application restart to >+ * take effect. s/take effect/complete/ >+ * @param types >+ * An optional array of types to limit the list to >+ * @param callback >+ * The callback to pass the array of Addons to >+ * @throws if the callback argument is invalid >+ */ >+ getAddonsWithPendingOperations: >+ function AM_getAddonsWithPendingOperations(types, callback) { >+ >+/** >+ * This is a private API for the startup and platform integration code to use >+ * It is not properly documented and subject to change at any point without nit: I wouldn't bother with stating that it is not properly documented since the forwards are but I might mention to refer to the forward declares for documentation. >+ * warning. Should not be used outside of core Mozilla code. All methods just nit: I'd start the comment with "Should not be used outside of core Mozilla code." >+/** >+ * This is the public API that UI and developers should be calling. All methods >+ * just forward to AddonManagerInternal. >+ */ >+var AddonManager = { >... >+ // Constants representing different types of errors while downloading an >+ // add-on. >+ // The download failed due to network problems. >+ DOWNLOADERROR_NETWORK: -1, >+ // The downloaded file did not match the provided hash. >+ DOWNLOADERROR_INCORRECT_HASH: -2, >+ // The downloaded file seems to be corrupted in some way. >+ DOWNLOADERROR_CORRUPT_FILE: -3, I prefer if errors start with ERROR_ >+ // Constants for permissions in Addon.permissions. >+ // Indicates that the Addon can be uninstalled. >+ PERM_CANUNINSTALL: 1, >+ // Indicates that the Addon can be enabled by the user. >+ PERM_CANENABLE: 2, >+ // Indicates that the Addon can be disabled by the user. >+ PERM_CANDISABLE: 4, >+ // Indicates that the Addon can be upgraded. >+ PERM_CANUPGRADE: 8, nit: there appear to be more _CAN_'s than _CAN*'s in mxr but it is by no means 100% >+ >+ getInstallForURL: function(url, callback, mimetype, hash, name, iconURL, >+ version, loadgroup) { >+ AddonManagerInternal.getInstallForURL(url, callback, mimetype, hash, name, >+ iconURL, version, loadgroup); >+ }, >+ >+ getInstallForFile: function(file, callback, mimetype) { >+ AddonManagerInternal.getInstallForFile(file, callback, mimetype); >+ }, >+ >+ getAddon: function(id, callback) { >+ AddonManagerInternal.getAddon(id, callback); >+ }, >+ >+ getAddons: function(ids, callback) { >+ AddonManagerInternal.getAddons(ids, callback); >+ }, >+ >+ getAddonsWithPendingOperations: function(types, callback) { >+ AddonManagerInternal.getAddonsWithPendingOperations(types, callback); >+ }, >+ >+ getAddonsByType: function(types, callback) { >+ AddonManagerInternal.getAddonsByType(types, callback); >+ }, >+ >+ getInstalls: function(types, callback) { >+ AddonManagerInternal.getInstalls(types, callback); >+ }, >+ >+ isInstallEnabled: function(type) { >+ return AddonManagerInternal.isInstallEnabled(type); >+ }, >+ >+ isInstallAllowed: function(type, uri) { >+ return AddonManagerInternal.isInstallAllowed(type, uri); >+ }, >+ >+ startInstallation: function(type, source, uri, installs) { >+ AddonManagerInternal.startInstallation(type, source, uri, installs); >+ }, >+ >+ addInstallListener: function(listener) { >+ AddonManagerInternal.addInstallListener(listener); >+ }, >+ >+ removeInstallListener: function(listener) { >+ AddonManagerInternal.removeInstallListener(listener); >+ }, >+ >+ addAddonListener: function(listener) { >+ AddonManagerInternal.addAddonListener(listener); >+ }, >+ >+ removeAddonListener: function(listener) { >+ AddonManagerInternal.removeAddonListener(listener); >+ } >+}; Please add names for the anonymous functions
Comment 84•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >Bug 553169: Implement the basic extension manager API and platform integration. > >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in >--- a/browser/installer/package-manifest.in >+++ b/browser/installer/package-manifest.in >@@ -301,7 +301,7 @@ > @BINPATH@/components/NetworkGeolocationProvider.js > @BINPATH@/components/GPSDGeolocationProvider.js > @BINPATH@/components/nsSidebar.js >-@BINPATH@/components/nsExtensionManager.js >+@BINPATH@/components/extManager.js > @BINPATH@/components/nsBlocklistService.js > #ifdef MOZ_UPDATER > @BINPATH@/components/nsUpdateService.js I believe you changed this to amManager.js. Bummer about the am standing for Add-ons Manager and being followed by Manager. :(
Comment 85•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >diff --git a/toolkit/mozapps/extensions/amIManager.idl b/toolkit/mozapps/extensions/amIManager.idl >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/amIManager.idl >... >+/** >+ * This interface is used to allow various parts of the platform to talk to Please update the comment description since this interface has changed pretty dramatically as it has been updated... should update the interface name to be more descriptive as well. >+ */ >+[scriptable, uuid(4fdf4f84-73dc-4857-9bbe-84895e8afd5d)] >+interface amIManager : nsISupports >+{ >+ /** >+ * Checks if installation is enabled for a webpage. >+ * @param mimetype >+ * The mimetype for the add-on to be installed >+ * @param referer >+ * The URL of the webpage trying to install an add-on >+ * @return true if installation is enabled >+ */ >+ boolean isInstallEnabled(in AString mimetype, in nsIURI referer); >+ >+ /** >+ * Installs an array of new add-ons nit: are they always new? Might be better to just state "Installs an array of add-ons" >+ * @param mimetype >+ * The mimetype for the add-ons >+ * @param window >+ * The window installing the add-ons >+ * @param referer >+ * The URI for the webpage installing the add-ons >+ * @param uris >+ * The URIs of add-ons to be installed >+ * @param hashes >+ * The hashes for the add-ons to be installed >+ * @param names >+ * The names for the add-ons to be installed >+ * @param icons >+ * The icons for the add-ons to be installed >+ * @param callback >+ * An optional callback to notify about installation success and >+ * failure >+ * @param installCount >+ * An optional argument including the number of add-ons to install >+ * @return true if the installation was successfully started >+ */ >+ boolean install(in AString mimetype, in nsIDOMWindowInternal window, >+ in nsIURI referer, >+ [array, size_is(installCount)] in wstring uris, >+ [array, size_is(installCount)] in wstring hashes, >+ [array, size_is(installCount)] in wstring names, >+ [array, size_is(installCount)] in wstring icons, >+ [optional] in amIInstallCallback callback, >+ [optional] in PRUint32 installCount); >+};
Comment 86•14 years ago
|
||
Comment on attachment 434397 [details] [diff] [review] base rev 2 >diff --git a/toolkit/mozapps/extensions/amManager.js b/toolkit/mozapps/extensions/amManager.js >+ >+/** >+ * This component serves as integration between the platform and AddonManager. >+ * It is responsible for initialization and shutdown as well as a couple of >+ * other things. Update the comment here as well. >+amManager.prototype = { >+ observe: function AMC_observe(subject, topic, data) { >+ var os = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); nit: might as well make this a let to be consistent >+ >+ switch (topic) { >+ case "profile-after-change": >+ os.addObserver(this, "xpcom-shutdown", false); >+ AddonManagerPrivate.startup(); >+ break; >+ case "xpcom-shutdown": >+ os.removeObserver(this, "xpcom-shutdown"); >+ AddonManagerPrivate.shutdown(); >+ break; >+ } >+ }, >+ >... >+ notify: function EM_notify(timer) { s/EM_/AMC_/ I'd like to verify the new comments before r+'ing this. I'd really like to see a better name than amManager but since everything is still under @mozilla.org/extensions/ you should probably go with your original naming for the component of extManager. Next review should be an r+
Attachment #434397 -
Flags: review?(robert.bugzilla) → review-
Comment 87•14 years ago
|
||
Comment on attachment 434395 [details] [diff] [review] updatechecker rev 2 Only a nit that might have other occurrences in the patch >+ function getRequiredProperty(ds, source, property) { >+ let value = getProperty(ds, source, property); >+ if (!value) >+ throw new Error("Missing required property " + property); >+ return value; >+ } >+ >+ let rdfParser = Cc["@mozilla.org/rdf/xml-parser;1"]. >+ createInstance(Ci.nsIRDFXMLParser); >+ let ds = Cc["@mozilla.org/rdf/datasource;1?name=in-memory-datasource"]. >+ createInstance(Ci.nsIRDFDataSource); >+ rdfParser.parseString(ds, request.channel.URI, request.responseText); >+ >+ switch (type) { >+ case "extension": >+ var item = PREFIX_EXTENSION + id; >+ break; >+ case "theme": >+ item = PREFIX_THEME + id; >+ break; >+ default: >+ item = PREFIX_ITEM + id; >+ break; >+ } >+ >+ let extensionRes = gRDF.GetResource(item); >+ >+ // If we have an update key then the update manifest must be signed >+ if (updateKey) { >+ let signature = getProperty(ds, extensionRes, "signature"); >+ if (!signature) >+ throw new Error("Update manifest for " + id + " does not contain a required signature"); >+ let serializer = new RDFSerializer(); >+ let updateString = null; >+ >+ try { >+ updateString = serializer.serializeResource(ds, extensionRes); >+ } >+ catch (e) { >+ throw new Error("Failed to generate signed string for " + id + ". Serializer threw " + e); >+ } >+ >+ let result = false; >+ try { nit: a few lines above this you have a newline before the try and here you don't. >+ let verifier = Cc["@mozilla.org/security/datasignatureverifier;1"]. >+ getService(Ci.nsIDataSignatureVerifier); >+ result = verifier.verifyData(updateString, signature, updateKey); >+ } Looks good r=me
Attachment #434395 -
Flags: review?(robert.bugzilla) → review+
Assignee | ||
Comment 88•14 years ago
|
||
Comment on attachment 434400 [details] [diff] [review] xpinstall rev 2 Just spotted I accidentally renamed everything to amInstalltrigger as opposed to amInstallTrigger. Going to fix that up now.
Attachment #434400 -
Flags: review?(robert.bugzilla)
Comment 89•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+const KEY_APP_PROFILE = "app-profile"; >+const KEY_APP_GLOBAL = "app-global"; >+const KEY_APP_SYSTEM_LOCAL = "app-system-local"; >+const KEY_APP_SYSTEM_SHARE = "app-system-share"; >+const KEY_APP_SYSTEM_USER = "app-system-user"; Need to handle winreg-app-global and winreg-app-user as well
Assignee | ||
Comment 90•14 years ago
|
||
This is the fixed xpinstall patch, also unbitrotted for trunk.
Attachment #434400 -
Attachment is obsolete: true
Attachment #435043 -
Flags: review?(robert.bugzilla)
Comment 91•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 Damn... I was kind of hhoping you had left bopy :( >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+/** >+ * Extracts files from a ZIP file into a directory. >+ * @param zipFile >+ * The source XPI file that contains the item. Best to be consistent between calling it a ZIP file and an XPI file... I personally prefer ZIP especially since it is consistent with other comments. >... >+/** >+ * Replaces %...% strings in an addon url (update and updateInfo) with >+ * appropriate values. >+ * @param addon >+ * The AddonInternal representing the item >+ * @param appVersion >+ * The application version to use for %APP_VERSION% Please note that this is optional, etc. >+ * @param uri >+ * The uri to escape >+ * @param updateType >+ * An optional number representing the type of update, only applicable >+ * when creating a url for retrieving an update manifest >+ * @return the appropriately escaped uri. >+ */ >+function escapeAddonURI(addon, appVersion, uri, updateType) nit: I tend to prefer optional params after required params. >... >+/** >+ * A generator to synchronously return result rows from an mozIStorageStatement. >+ * @param statement >+ * The statement to execute >+ */ >+function resultRows(statement) { >+ try { >+ while (statement.executeStep()) >+ yield statement.row; nit: every other while with a single statement after it has braces >+ } >+ finally { >+ statement.reset(); >+ } >+} >+ >+/** >+ * A helpful wrapper around the prefs service that allows for default values >+ * when requested values aren't set. >+ */ >+var Prefs = { >+ /** >+ * Gets a preference from the default branch ignoring user-set values. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @return the default value of the preference or defaultValue if there is none >+ */ >+ getDefaultCharPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getDefaultBranch("").getCharPref(name); >+ } >+ catch (e) { >+ } >+ return defaultValue; >+ }, >+ >+ /** >+ * Gets a string preference. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @return the value of the preference or defaultValue if there is none >+ */ >+ getCharPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getCharPref(name); >+ } >+ catch (e) { >+ } >+ return defaultValue; >+ }, >+ >+ /** >+ * Gets a boolean preference. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @return the value of the preference or defaultValue if there is none >+ */ >+ getBoolPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getBoolPref(name); >+ } >+ catch (e) { >+ return defaultValue; >+ } >+ }, >+ >+ /** >+ * Gets an integer preference. >+ * @param name >+ * The name of the preference >+ * @param defaultValue >+ * A value to return if the preference does not exist >+ * @return the value of the preference or defaultValue if there is none >+ */ >+ getIntPref: function(name, defaultValue) { >+ try { >+ return Services.prefs.getIntPref(name); >+ } >+ catch (e) { >+ return defaultValue; >+ } >+ } >+} Please make return defaultValue in getBoolPref and getIntPref consistent with the change you made to getCharPref. >+var XPIProvider = { >+ // An array of known install locations >+ installLocations: null, >+ // A dictionary of known install locations by name >+ installLocationsByName: null, >+ // An array of currently active AddonInstalls >+ installs: null, >+ // The default skin for the application >+ defaultSkin: "classic/1.0", >+ // The currently sselected skin or the skin that will be switched to after a s/sselected/selected/ >+ /** >+ * Gets the item states for an install location. >+ * @param location >+ * The install location to retrieve the item states for >+ * @return a dictionary mapping add-on IDs to objects with a descriptor >+ * property which contains the add-ons directory descriptor and an mtime >+ * property which contains the add-on's last modified time as the number of >+ * milliseconds since the epoch. excellent >+ */ >+ getItemStates: function XPI_getItemStates(location) { >+ let itemStates = {}; >+ location.itemLocations.forEach(function(dir) { >+ let id = location.getIDForLocation(dir); >+ itemStates[id] = { >+ descriptor: dir.persistentDescriptor, >+ mtime: dir.lastModifiedTime >+ }; >+ }); >+ >+ return itemStates; >+ }, >+ >+ /** >+ * Gets an array of install location states which uniquely describes the >+ * the current set of installed add-ons, their location and last modified >+ * times. This doesn't parse all that well. Perhaps Gets an array of install location states which uniquely describes all installed add-ons with the add-on's InstallLocation name and last modified time. >+ * @return an array of states for each install location. Each state is an >+ * object a name property holding the location's name and an items property >+ * holding the item states for the location. This as well. Perhaps an array of add-on states for each install location. Each state is an object with a name property holding the location's name and an items property holding the item states for the location.
Comment 92•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Check the staging directories of install locations for any new items to be >+ * installed or items to be uninstalled. nit: new isn't necessary... 'for items to be installed or uninstalled' should be sufficient. >+ * @param manifests >+ * A dictionary to add detected install manifests to for the purpose >+ * of passing through updated compatibility information >+ * @return true if an item was installed or uninstalled >+ */ >+ processPendingFileChanges: function XPI_processPendingFileChanges(manifests) { >+ // TODO maybe this should be passed off to the install locations to handle? >+ let changed = false; >+ this.installLocations.forEach(function(location) { >+ manifests[location.name] = {}; >+ // We can't install anything into locked locations >+ if (location.locked) >+ return; >+ >+ let dir = location.getStagingDir(); >+ if (!dir || !dir.exists()) >+ return; >+ >+ let entries = dir.directoryEntries; >+ while (entries.hasMoreElements()) { >+ let stageDir = entries.getNext().QueryInterface(Ci.nsILocalFile); stageDir isn't really the staging directory. Perhaps stageDirEntries? >+ >+ // Only directories are important. Files may be updated manifests. >+ if (!stageDir.isDirectory()) { >+ WARN("Ignoring file: " + stageDir.path); >+ continue; >+ } >+ >+ // Check that the directory's name is a valid ID. >+ let id = stageDir.leafName; >+ if (!gIDTest.test(id)) { >+ WARN("Ignoring directory whose name is not a valid add-on ID: " + >+ stageDir.path); >+ continue; >+ } >+ >+ try { >+ // Check if the directory contains an install manifest. >+ let manifest = stageDir.clone(); >+ manifest.append(FILE_INSTALL_MANIFEST); This is an awfully big try catch. At the very least you have a dir that can be cloned, etc. >+ >+ // If not this is a signal we should uninstall the add-on with the >+ // matching ID in this install location >+ if (!manifest.exists()) { >+ LOG("Processing uninstall of " + id + " in " + location.name); >+ location.uninstallItem(id); >+ // The file check later will spot the removal and cleanup the database >+ changed = true; >+ continue; >+ } >+ >+ LOG("Processing install of " + id + " in " + location.name); >+ let newdir = location.installItem(id, stageDir); >+ >+ // Check for a cached AddonInternal for this item, it may contain >+ // updated compatibility information >+ let jsonfile = dir.clone(); >+ jsonfile.append(id + ".json"); >+ if (jsonfile.exists()) { >+ LOG("Found updated manifest for " + id + " in " + location.name); >+ let addon = null; >+ let stream = Cc["@mozilla.org/network/file-input-stream;1"]. >+ createInstance(Ci.nsIFileInputStream); >+ let json = Cc["@mozilla.org/dom/json;1"]. >+ createInstance(Ci.nsIJSON); >+ try { >+ stream.init(jsonfile, -1, 0, 0); >+ addon = json.decodeFromStream(stream, jsonfile.fileSize); >+ addon._sourceBundle = newdir; >+ } >+ finally { >+ stream.close(); >+ } >+ manifests[location.name][id] = addon; >+ } >+ else { >+ manifests[location.name][id] = null; >+ } >+ changed = true; >+ } >+ catch (e) { >+ ERROR("Error processing staged item " + stageDir.leafName + " in " + >+ location.name + ": " + e); >+ } >+ finally { >+ if (stageDir.exists()) >+ stageDir.remove(true); >+ } >+ } >+ try { >+ dir.remove(true); >+ } >+ catch (e) { >+ // Non-critical, just saves some perf on startup if we clean this up. >+ LOG("Error removing staging dir " + dir.path + ": " + e); >+ } >+ }); >+ return changed; >+ },
Comment 93•14 years ago
|
||
s/stageDirEntries/stageDirEntry/
Comment 94•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 You could also use entry instead of stageDirEntry if you prefer. >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ processPendingFileChanges: function XPI_processPendingFileChanges(manifests) { >... + let dir = location.getStagingDir(); + if (!dir || !dir.exists()) + return; + How about stagingDir instead? >... >+ try { >+ // Check if the directory contains an install manifest. >+ let manifest = stageDir.clone(); >+ manifest.append(FILE_INSTALL_MANIFEST); >... >+ >+ // If not this is a signal we should uninstall the add-on with the >+ // matching ID in this install location nit: don't think you need to mention matching ID since that is a given. Perhaps If the install manifest doesn't exist uninstall this add-on in this install location. >+ if (!manifest.exists()) { >+ LOG("Processing uninstall of " + id + " in " + location.name); >+ location.uninstallItem(id); >+ // The file check later will spot the removal and cleanup the database >+ changed = true; >+ continue; >+ } >+ >+ LOG("Processing install of " + id + " in " + location.name); >+ let newdir = location.installItem(id, stageDir); instead of newDir how about addonDir or addonInstallDir? >+ >+ // Check for a cached AddonInternal for this item, it may contain >+ // updated compatibility information >+ let jsonfile = dir.clone(); >+ jsonfile.append(id + ".json"); >+ if (jsonfile.exists()) { >+ LOG("Found updated manifest for " + id + " in " + location.name); >+ let addon = null; Please put the addon declaration before the try catch. >+ let stream = Cc["@mozilla.org/network/file-input-stream;1"]. >+ createInstance(Ci.nsIFileInputStream); Perhaps s/stream/fis/ since that is what you use elsewhere >+ let json = Cc["@mozilla.org/dom/json;1"]. >+ createInstance(Ci.nsIJSON); >+ try { >+ stream.init(jsonfile, -1, 0, 0); >+ addon = json.decodeFromStream(stream, jsonfile.fileSize); >+ addon._sourceBundle = newdir; >+ } Is it worth logging that this failed?
Comment 95•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Compares the add-ons that are currently installed to those that were >+ * known to be installed when the application last ran and applies those >+ * changes to the database. nit: applies those changes to the database is a tad unclear. Perhaps applies any changes found to the database >+ * @param state >+ * The array of current install location states >+ * @param manifests >+ * A dictionary of cached AddonInstalls for items that have been >+ * installed >+ * @param updateCompatibility >+ * True if appDisabled should be re-calculated for items since an >+ * application version change has occurred. nit: a tad unclear. Perhaps one of the following: true to update item's appDisabled property when the application version has changed. or true to recalculate an item's appDisabled property when the application version has changed. >+ * @return true if a change requiring a restart was detected >+ */ >+ processFileChanges: function XPI_processFileChanges(state, manifests, >+ updateCompatibility, >+ migrateData) { >+ let visibleItems = {}; >+ >+ /** >+ * Called when an add-on has been detected to have been modified, either >+ * because it has been upgraded or it has moved to a new directory. Updates an add-on's metadata and determines if a restart of the application is necessary. This is called when either the add-on's install directory path or last modified time has changed. >+ * @param installLocation >+ * The install location containing the add-on >+ * @param oldAddon >+ * The AddonInternal as it appeared the last time the application >+ * ran >+ * @param item >+ * The new state of the add-on could item be better named as state or addonState? Here and elsewhere >+ * @return a boolean indicating if restarting the application is required >+ * to complete changing this add-on >+ */ >+ function itemChanged(installLocation, oldAddon, item) { The name itemChanged at first made me think it was to detect whether an item has changed. Perhaps a name that describes what it does? updateMetadata or something similar? >+ LOG("Item " + oldAddon.id + " modified in " + installLocation.name); >+ >+ // Check if there is an update manifest for this add-on >+ let newAddon = manifests[installLocation.name][oldAddon.id]; >+ >+ try { >+ // If not load it from the directory nit: I don't believe that it refers to the update manifest as noted in the previous comment. >+ if (!newAddon) >+ newAddon = loadManifestFromDir(installLocation.getLocationForID(oldAddon.id)); >+ >+ // The ID in the manifest must match the ID of the old add-on (and >+ // hence the directory name). Please update the comment to state something like 'the ID of the manifest that was loaded' since the manifest could be the update manifest or the install manifest unless I'm mistaken. Not sure why you included "(and hence the directory name)"... could you rephrase it? >+ if (newAddon.id != oldAddon.id) >+ throw new Error("Incorrect id in install manifest"); >+ } >+ catch (e) { >+ WARN("Item is invalid: " + e); >+ XPIDatabase.removeAddonMetadata(oldAddon); >+ installLocation.uninstallItem(oldAddon.id); >+ // If this was an active item then we must force a restart >+ if (oldAddon.active) { >+ if (oldAddon.type == "bootstrapped") >+ delete XPIProvider.bootstrappedAddons[oldAddon.id]; >+ else >+ return true; >+ } >+ >+ return false; >+ } >+ >+ // Set the additional properties on the new AddonInternal >+ newAddon._installLocation = installLocation.name; Ideally _installLocation would be an InstallLocation and it would be better to use _installLocationName instead. >+ newAddon.userDisabled = oldAddon.userDisabled; >+ newAddon.installDate = oldAddon.installDate; >+ newAddon.updateDate = item.mtime; >+ newAddon.visible = !(newAddon.id in visibleItems); >+ >+ // Update the database >+ XPIDatabase.updateAddonMetadata(oldAddon, newAddon, item.descriptor); >+ if (newAddon.visible) { >+ visibleItems[newAddon.id] = newAddon; >+ // If the old version was active and wasn't bootstrapped or the new >+ // version will be active and isn't bootstrapped then we must force a >+ // restart >+ if ((oldAddon.active && oldAddon.type != "bootstrapped") || >+ (newAddon.active && newAddon.type != "bootstrapped")) { >+ return true; >+ } >+ } >+ >+ return false; >+ } >+ >+ /** >+ * Called when no change has been detected for an installed add-on. This should describe what the function does as well... also s/change has/changes have/ in case you keep that part. >+ * @param installLocation >+ * The install location containing the add-on >+ * @param oldAddon >+ * The AddonInternal as it appeared the last time the application >+ * ran >+ * @param item >+ * The new state of the add-on >+ * @return a boolean indicating if restarting the application is required >+ * to complete changing this add-on >+ */ >+ function itemUnchanged(installLocation, oldAddon, item) { >+ let changed = false; >+ >+ // No change to this item but it might have become visible >+ if (!(oldAddon.id in visibleItems)) { No change to this add-on's metadata?
Assignee | ||
Comment 96•14 years ago
|
||
(In reply to comment #79) > >+# The Original Code is the Extension Manager. > >+# > >+# The Initial Developer of the Original Code is > >+# Mozilla Foundation. > nit: per Gerv > http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html > > It should be "the Mozilla Foundation." Well damn > >+function AsyncObjectCaller(objects, method, listener) { > Intercaps and add a function header comment. Are you saying you don't want this with intercaps? > >+ * @return false if any of the listeners returned false, true otherwise > >+ */ > >+ callInstallListeners: function AM_callInstallListeners(method, additional) { > >+ let result = true; > >+ let listeners = this.installListeners; > >+ if (additional) > >+ listeners = additional.concat(listeners); > nit: if the order they are called is not important I'd prefer that the > registered InstallListeners are called first The extra listeners are basically those registered for a specific AddonInstall. I think it makes more sense to call those first and the general listeners second.
Assignee | ||
Comment 97•14 years ago
|
||
(In reply to comment #81) > > > Nit: Suggest renaming these variables to AddonManager.PERM_CAN_DISABLE, > > > AddonManager.PERM_CAN_ENABLE. > > > > Maybe, Rob? > For myself, I'm fine either way but looking in mxr it appears there are more of > _CAN_, _CANT_, _CANNOT_, etc. than there are _CAN*, etc. Renamed.
Assignee | ||
Comment 98•14 years ago
|
||
(In reply to comment #82) > >+ * @param callback > >+ * A callback which will be passed an array of AddonInstalls > >+ * @throws if the callback argument is not invalid > nit: just noticed that when you changed @returns to @return you didn't remove > the additional space that is no longer required between @param comment, > @throws comment, @return comment. Adding that to the list of things to do once the rest of the review is done. > >+ isInstallEnabled: function AM_isInstallEnabled(mimetype) { > >+ for (let i = 0; i < this.providers.length; i++) { > >+ if (callProvider(this.providers[i], "supportsMimetype", false, mimetype)) > >+ return callProvider(this.providers[i], "isInstallEnabled"); > >+ } > >+ return false; > >+ }, > Is there protection against multiple providers from supporting the same > mimetype? Specifically in relation to isInstallEnabled can a provider state it > supports a mimetype and state that install is not enabled while another > supports the same mimetype and return state that install is enabled? I've changed this so if any provider says installation is enabled then it is enabled. Likewise for allowed below. > >+ * @param installs > >+ * An array of AddonInstalls to be installed > >+ */ > >+ startInstallation: function AM_startInstallation(mimetype, source, uri, > >+ installs) { > nit: don't really like the name. How does "installAddonsFromWebpage" grab you?
Assignee | ||
Comment 99•14 years ago
|
||
(In reply to comment #85) > >+++ b/toolkit/mozapps/extensions/amIManager.idl > >... > >+/** > >+ * This interface is used to allow various parts of the platform to talk to > Please update the comment description since this interface has changed pretty > dramatically as it has been updated... should update the interface name to be > more descriptive as well. The only thing that this interface is used for right now is communicating new installs triggered by webpages to the AddonManager jsm. So how about amIWebInstaller? Going to hold off on actually making that change for now.
Comment 100•14 years ago
|
||
(In reply to comment #96) > (In reply to comment #79) > >... > > >+function AsyncObjectCaller(objects, method, listener) { > > Intercaps and add a function header comment. > > Are you saying you don't want this with intercaps? My mistake... must have misread > > >+ * @return false if any of the listeners returned false, true otherwise > > >+ */ > > >+ callInstallListeners: function AM_callInstallListeners(method, additional) { > > >+ let result = true; > > >+ let listeners = this.installListeners; > > >+ if (additional) > > >+ listeners = additional.concat(listeners); > > nit: if the order they are called is not important I'd prefer that the > > registered InstallListeners are called first > > The extra listeners are basically those registered for a specific AddonInstall. > I think it makes more sense to call those first and the general listeners > second. Makes sense (In reply to comment #98) > (In reply to comment #82) > > >... > > >+ * @param installs > > >+ * An array of AddonInstalls to be installed > > >+ */ > > >+ startInstallation: function AM_startInstallation(mimetype, source, uri, > > >+ installs) { > > nit: don't really like the name. > > How does "installAddonsFromWebpage" grab you? Sounds good (In reply to comment #99) > The only thing that this interface is used for right now is communicating new > installs triggered by webpages to the AddonManager jsm. So how about > amIWebInstaller? Going to hold off on actually making that change for now. Much better... not sure if it should be amIWebInstall (ala xpinstall) or amIWebInstaller but I can go either way with it.
Assignee | ||
Comment 101•14 years ago
|
||
(In reply to comment #86) > I'd like to verify the new comments before r+'ing this. I'd really like to see > a better name than amManager but since everything is still under > @mozilla.org/extensions/ you should probably go with your original naming for > the component of extManager. Next review should be an r+ I guess there is actually nothing stopping us moving to @mozilla.org/addons/. I can't think of a great name though, this component is a jumble of the amIWebInstaller, the bits needed to startup and shutdown AddonManager and the listener for the background update timer, which leaves me thinking something generic like amPlatformIntegration. Got any better ideas?
Assignee | ||
Comment 102•14 years ago
|
||
(In reply to comment #89) > (From update of attachment 434398 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm > >... > >+const KEY_APP_PROFILE = "app-profile"; > >+const KEY_APP_GLOBAL = "app-global"; > >+const KEY_APP_SYSTEM_LOCAL = "app-system-local"; > >+const KEY_APP_SYSTEM_SHARE = "app-system-share"; > >+const KEY_APP_SYSTEM_USER = "app-system-user"; > Need to handle winreg-app-global and winreg-app-user as well I'll do these as a separate patch in bug 555083
Comment 103•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 Please replace True with true in comments as appropriate Please check code that goes beyond 80... there are a couple of places that are longer than 80 that can easily wrap. >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Called when no change has been detected for an installed add-on. >+ * @param installLocation >+ * The install location containing the add-on >+ * @param oldAddon >+ * The AddonInternal as it appeared the last time the application >+ * ran >+ * @param item >+ * The new state of the add-on >+ * @return a boolean indicating if restarting the application is required nit: you typically comment '@return true...' but didn't do so here and a couple of other places where you return a boolean. >+ * to complete changing this add-on >+ */ >+ function itemUnchanged(installLocation, oldAddon, item) { itemUnchanged naming and other similar names as I might have previously mentioned >+ /** >+ * Called when an add-on has been removed. >+ * @param location >+ * The name of the install location containing the add-on >+ * @param oldAddon >+ * The AddonInternal as it appeared the last time the application >+ * ran >+ * @return a boolean indicating if restarting the application is required >+ * to complete changing this add-on >+ */ >+ function itemRemoved(location, oldAddon) { >+ // This item has disappeared >+ LOG("Item " + oldAddon.id + " removed from " + location); >+ XPIDatabase.removeAddonMetadata(oldAddon); >+ if (oldAddon.active) { >+ >+ // Enable the default theme if the previously active theme has been >+ // removed >+ if (oldAddon.type == "theme") >+ XPIProvider.enableDefaultTheme(); >+ >+ // If this was an active item and bootstrapped we must remove it from >+ // the bootstrapped list, otherwise we need to force a restart. >+ if (oldAddon.type == "bootstrapped") >+ delete XPIProvider.bootstrappedAddons[oldAddon.id]; >+ else >+ return true; if (oldAddon.type != "bootstrapped") return true; delete XPIProvider.bootstrappedAddons[oldAddon.id]; >+ >+ return false; >+ } >+ >+ /** >+ * Called when a new add-on has been detected. >+ * @param installLocation >+ * The install location containing the add-on >+ * @param id >+ * The ID of the add-on >+ * @param item >+ * The new state of the add-on >+ * @param migrateData >+ * If during startup the database had to be upgraded this will >+ * contain data that used to be held about this add-on >+ * @return a boolean indicating if restarting the application is required >+ * to complete changing this add-on >+ */ >+ function itemAdded(installLocation, id, item, migrateData) { >+ LOG("New item " + id + " installed in " + installLocation.name); >+ >+ // Check the updated manifests lists for a manifest for this add-on >+ let newAddon = manifests[installLocation.name][id]; >+ try { >+ // Otherwise load the manifest from the add-on's directory. >+ if (!newAddon) >+ newAddon = loadManifestFromDir(installLocation.getLocationForID(id)); >+ // The add-on in the manifest should match the expected ID. nit: The add-on ID >+ if (newAddon.id != id) >+ throw new Error("Incorrect id in install manifest"); >+ } >+ catch (e) { >+ WARN("Item is invalid: " + e); >+ >+ // Remove the invalid item from the install location, no restart will >+ // be necessary >+ installLocation.uninstallItem(id); >+ return false; >+ } >+ >+ // Update the AddonInternal properties. >+ newAddon._installLocation = installLocation.name; >+ newAddon.visible = !(newAddon.id in visibleItems); >+ newAddon.installDate = item.mtime; >+ newAddon.updateDate = item.mtime; >+ >+ // If there is migration data then apply it. >+ if (migrateData) { >+ newAddon.userDisabled = migrateData.userDisabled; >+ if ("installDate" in migrateData) >+ newAddon.installDate = migrateData.installDate; >+ } >+ >+ // Update the database. >+ XPIDatabase.addAddonMetadata(newAddon, item.descriptor); >+ >+ // Visible bootstrapped items need to be added to the bootstrap list. >+ if (newAddon.visible) { >+ visibleItems[newAddon.id] = newAddon; >+ if (newAddon.type == "bootstrapped") >+ XPIProvider.bootstrappedAddons[newAddon.id] = item.descriptor; >+ else >+ return true; if (newAddon.type != "bootstrapped") return true; XPIProvider.bootstrappedAddons[newAddon.id] = item.descriptor; >+ } >+ >+ return false; >+ } >+ >+ let changed = false; >+ let knownLocations = XPIDatabase.getInstallLocations(); >+ >+ // It is important that we are iterating through the install locations >+ // in reverse order of priority. Because of this if we see multiple add-ons >+ // with the same ID then the first one is the one that should be visible. Perhaps something along the lines of: The install locations are iterated in reverse order of priority so when there are multiple add-ons installed with the same ID the correct one will be the one that is visible. >+ // This is tracked with the visibleItems dictionary. >+ state.reverse().forEach(function(st) { >+ >+ // We can't include the install location directly in the state as it has >+ // to be cached as JSON. >+ let installLocation = this.installLocationsByName[st.name]; >+ let items = st.items; >+ >+ // Check if the database knows about any add-ons in this install location. >+ let pos = knownLocations.indexOf(installLocation.name); >+ if (pos >= 0) { >+ knownLocations.splice(pos, 1); >+ let addons = XPIDatabase.getAddonsInLocation(installLocation.name); >+ // Iterate through the add-ons installed the last time the application >+ // ran >+ addons.forEach(function(oldAddon) { >+ // Check if the add-on is still installed >+ if (oldAddon.id in items) { >+ let item = items[oldAddon.id]; >+ delete items[oldAddon.id]; >+ >+ // The add-on has changed if the modification time has changed, or >+ // the directory it is installed in has changed or we have an >+ // updated manifest for it. >+ if (oldAddon.id in manifests[installLocation.name] || >+ oldAddon.updateDate != item.mtime || >+ oldAddon._descriptor != item.descriptor) >+ changed = itemChanged(installLocation, oldAddon, item) || changed; >+ else >+ changed = itemUnchanged(installLocation, oldAddon, item) || changed; >+ } >+ else { >+ changed = itemRemoved(installLocation.name, oldAddon) || changed; >+ } >+ }, this); >+ } >+ >+ // All the remaining items in this install location must be new. >+ >+ // Get the migration data for this install location. >+ let locMigrateData = {}; >+ if (migrateData && installLocation.name in migrateData) >+ locMigrateData = migrateData[installLocation.name]; >+ for (let id in items) { >+ changed = itemAdded(installLocation, id, items[id], locMigrateData[id]) || >+ changed; >+ } >+ }, this); >+ >+ // The remaining locations found in the database haven't any add-ons >+ // installed in them any longer so remove them. confusing wording... seems like the locations are being removed. >+ knownLocations.forEach(function(location) { >+ let addons = XPIDatabase.getAddonsInLocation(location); >+ addons.forEach(function(oldAddon) { >+ changed = itemRemoved(location, oldAddon) || changed; >+ }, this); >+ }, this); >+ >+ // Cache the new install location states >+ cache = JSON.stringify(this.getInstallLocationStates()); >+ Services.prefs.setCharPref(PREF_INSTALL_CACHE, cache); >+ return changed; >+ }, >+ >+ /** >+ * Checks for any changes that have occurred since the last time the >+ * application was launched. >+ * @param appChanged >+ * True if the application has changed version number since the last >+ * launch >+ * @return true if a change requiring a restart was detected >+ */ >+ checkForChanges: function XPI_checkForChanges(appChanged) { >+ LOG("checkForChanges"); >+ >+ // First install any new items into the locations, we'll detect these when >+ // we read the install state >+ let manifests = {}; >+ let changed = this.processPendingFileChanges(manifests); >+ >+ // We have to hold the DB scheme in prefs so we don't need to load the >+ // database to see if we need to migrate data >+ let schema = Prefs.getIntPref(PREF_DB_SCHEMA, 0); >+ >+ let migrateData = null; >+ let cache = null; >+ if (schema != DB_SCHEMA) { >+ // If the schema has changed then we need to migrate data from the old >+ // schema nit: since it is inside the if block The schema has changed so migrate data from the old schema >+ migrateData = XPIDatabase.migrateData(schema); >+ } >+ else { >+ // Otherwise if the database exists we can trust the previous file cache nit: If the database exists the previous file cache can be trusted >+ let db = FileUtils.getFile(KEY_PROFILEDIR, [FILE_DATABASE], true); >+ if (db.exists()) >+ cache = Prefs.getCharPref(PREF_INSTALL_CACHE, null); >+ } >+ >+ // Load the list of bootstrapped add-ons first so processFileChanges can >+ // modify it >+ this.bootstrappedAddons = JSON.parse(Prefs.getCharPref(PREF_BOOTSTRAP_ITEMS, "{}")); >+ let state = this.getInstallLocationStates(); >+ if (appChanged || changed || cache == null || cache != JSON.stringify(state)) >+ changed = this.processFileChanges(state, manifests, appChanged, migrateData); >+ >+ // If the application crashed before completing any pending operations then >+ // we should perform them now. >+ if (changed || Prefs.getBoolPref(PREF_PENDING_OPERATIONS)) { >+ LOG("Restart necessary"); >+ XPIDatabase.updateActiveAddons(); >+ Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, false); >+ return true; >+ } >+ >+ LOG("No changes found"); Is this true or just cruft? You return true indicating that a change requiring a restart was detected before and after this log statement >+ >+ // Check that the add-ons list still exists >+ let addonsList = FileUtils.getFile(KEY_PROFILEDIR, [FILE_XPI_ADDONS_LIST], true); >+ if (!addonsList.exists()) { >+ LOG("Add-ons list is missing, recreating"); >+ XPIDatabase.writeAddonsList(); >+ return true; >+ } >+ >+ let bootstrappedAddons = this.bootstrappedAddons; >+ this.bootstrappedAddons = {}; >+ for (let id in bootstrappedAddons) { >+ let dir = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile); >+ dir.persistentDescriptor = bootstrappedAddons[id]; >+ this.activateAddon(id, dir, true, false); >+ } >+ >+ // Let these shutdown a little earlier when they still have access to most >+ // of XPCOM >+ Services.obs.addObserver({ >+ observe: function(subject, topic, data) { >+ Services.prefs.setCharPref(PREF_BOOTSTRAP_ITEMS, >+ JSON.stringify(XPIProvider.bootstrappedAddons)); >+ for (let id in XPIProvider.bootstrappedAddons) >+ XPIProvider.deactivateAddon(id, true, false); >+ Services.obs.removeObserver(this, "quit-application-granted"); >+ } >+ }, "quit-application-granted", false); >+ >+ return false; >+ }, >+ >+ /** >+ * Called to test whether this provider supports installing a particular >+ * mimetype. >+ * @param mimetype >+ * The mimetype to check for >+ * @return true if the mimetype is application/x-xpinstall >+ */ >+ supportsMimetype: function XPI_supportsMimetype(mimetype) { >+ return mimetype == "application/x-xpinstall"; >+ }, >+ >+ /** >+ * Called to test whether installing XPI add-ons is enabled. >+ * @return true if installing is enabled >+ */ >+ isInstallEnabled: function XPI_isInstallEnabled() { >+ return Prefs.getBoolPref(PREF_XPI_ENABLED, false); >+ }, Perhaps change the default to true and note what the default is above. >+ /** >+ * Called to test whether installing XPI add-ons from a URI is allowed. >+ * @param uri >+ * The URI being installed from >+ * @return true if installing is allowed >+ */ >+ isInstallAllowed: function XPI_isInstallAllowed(uri) { >+ if (!this.isInstallEnabled()) >+ return false; >+ >+ if (!uri) >+ return true; >+ >+ // file: and chrome: don't need whitelisted hosts >+ if (uri.schemeIs("chrome") || uri.schemeIs("file")) >+ return true; >+ >+ >+ let permission = Services.perms.testPermission(uri, XPI_PERMISSION); >+ if (permission == Ci.nsIPermissionManager.DENY_ACTION) >+ return false; >+ >+ let requireWhitelist = Prefs.getBoolPref(PREF_XPI_WHITELIST_REQUIRED, true); >+ if (requireWhitelist && (permission != Ci.nsIPermissionManager.ALLOW_ACTION)) >+ return false; >+ >+ return true; >+ }, >+ >+ /** >+ * Called to get an AddonInstall for a URL. Is this really for a URL or for a URL and the parameters specified? >+ * @param url >+ * The URL to be installed >+ * @param hash >+ * A hash for the install >+ * @param name >+ * A name for the install >+ * @param iconURL >+ * An icon URL for the install >+ * @param version >+ * A version for the install >+ * @param loadgroup >+ * A loadgroup to associate requests with >+ * @param callback >+ * A callback to pass the AddonInstall to >+ */ >+ getInstallForURL: function XPI_getInstallForURL(url, hash, name, iconURL, >+ version, loadgroup, callback) { >+ AddonInstall.createDownload(function(install) { >+ callback(install.wrapper); >+ }, url, hash, name, iconURL, version, loadgroup); >+ }, >+ >... >+ /** >+ * Called to get Addons that have pending operations. >+ * @param types >+ * An array of types to fetch. Can be null to get all types >+ * @param callback >+ * A callback to pass an array of Addons to >+ */ >+ getAddonsWithPendingOperations: function XPI_getAddonsWithPendingOperations(types, callback) { >+ XPIDatabase.getVisibleAddonsWithPendingOperations(types, function(addons) { >+ let results = [createWrapper(a) for each (a in addons)]; >+ XPIProvider.installs.forEach(function(install) { >+ if (install.state == AddonManager.STATE_INSTALLED && >+ !(install.addon instanceof DBAddonInternal)) { >+ results.push(createWrapper(install.addon)); >+ } nit: you typically don't brace for this case >+ }); >+ callback(results); >+ }); >+ }, >+ >... >+ /** >+ * Finds and enabled the default theme, used when there is no longer any theme >+ * selected for some reason. could be worded better >+ */ >+ enableDefaultTheme: function XPI_enableDefaultTheme() { >+ LOG("Activating default theme"); >+ let addons = XPIDatabase.getAddonsByType("theme"); >+ addons.forEach(function(a) { >+ // If this is theme contains the default skin and it is currently visible >+ // then mark it as enabled. >+ if (a.internalName == this.defaultSkin && a.visible) >+ this.updateAddonDisabledState(a, false); >+ }, this); >+ }, >... >+ /** >+ * Tests whether installing an add-on will require a restart. >+ * @param addon >+ * The add-on to test >+ * @return true if the operation requires a restart >+ */ >+ installRequiresRestart: function XPI_installRequiresRestart(addon) { >+ // Themes not currently in use can be installed immediately >+ if (addon.type == "theme") >+ return addon.internalName == Prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN); >+ return addon.type != "bootstrapped"; >+ }, nit: new line >+ /** >+ * Tests whether uninstalling an add-on will require a restart. >+ * @param addon >+ * The add-on to test >+ * @return true if the operation requires a restart >+ */ >+ uninstallRequiresRestart: function XPI_uninstallRequiresRestart(addon) { >+ // Themes not currently in use can be uninstalled immediately >+ if (addon.type == "theme") >+ return addon.internalName == Prefs.getCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN); >+ return addon.type != "bootstrapped"; >+ }, >... >+ /** >+ * Uninstalls an add-on, immediately if possible or marks it as pending >+ * uninstall if not. >+ * @param addon >+ * The DBAddonInternal to uninstall >+ * @throws if the addon cannot be uninstalled because it is in an install >+ * location that does not allow it >+ */ >+ uninstallAddon: function XPI_uninstallAddon(addon) { >+ if (!(addon instanceof DBAddonInternal)) >+ throw new Error("Can only uninstall installed addons."); >+ >+ installLocation = this.installLocationsByName[addon._installLocation]; >+ if (installLocation.locked) >+ throw new Error("Cannot uninstall addons from locked install locations"); >+ >+ // Inactive add-ons don't require a restart to uninstall >+ let requiresRestart = addon.active && this.uninstallRequiresRestart(addon); >+ >+ if (requiresRestart) { >+ // We create an empty directory in the staging directory to indicate that >+ // an uninstall is necessary on next startup. >+ let stage = installLocation.getStagingDir(); >+ stage.append(addon.id); >+ if (!stage.exists()) >+ stage.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY); >+ >+ XPIDatabase.setAddonProperties(addon, { >+ pendingUninstall: true >+ }); >+ Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, true); >+ } >+ >+ // If the add-on is not visible then there is no need to notify listeners. >+ if (!addon.visible) >+ return; >+ >+ let wrapper = createWrapper(addon); >+ AddonManagerPrivate.callAddonListeners("onUninstalling", wrapper, requiresRestart); >+ >+ if (!requiresRestart) { >+ if (addon.type == "bootstrapped") >+ this.deactivateAddon(addon.id, false, true); >+ installLocation.uninstallItem(addon.id); >+ XPIDatabase.removeAddonMetadata(addon); >+ AddonManagerPrivate.callAddonListeners("onUninstalled", wrapper); >+ // TODO reveal hidden items Please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ } >+ >+ // Notify any other providers that a new theme has been enabled >+ if (addon.type == "theme" && addon.active) >+ AddonManagerPrivate.notifyAddonChanged(null, addon.type, requiresRestart); >+ }, >+ >+ /** >+ * Cancels the pending uninstall of an add-on. >+ * @param addon >+ * The DBAddonInternal to cancel uninstall for >+ */ >+ cancelUninstallAddon: function XPI_cancelUninstallAddon(addon) { >+ if (!(addon instanceof DBAddonInternal)) >+ throw new Error("Can only cancel uninstall for installed addons."); >+ >+ let stage = installLocation.getStagingDir(); >+ stage.append(addon.id); >+ if (stage.exists()) >+ stage.remove(true); At first glance I thought this was the staging dir because of the name stage. >+ >+ XPIDatabase.setAddonProperties(addon, { >+ pendingUninstall: false >+ }); >+ >+ if (!addon.visible) >+ return; >+ >+ Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, true); >+ >+ // TODO hide hidden items Please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ let wrapper = createWrapper(addon); >+ AddonManagerPrivate.callAddonListeners("onOperationCancelled", wrapper); >+ >+ // Notify any other providers that this theme is now enabled again. >+ if (addon.type == "theme" && addon.active) >+ AddonManagerPrivate.notifyAddonChanged(addon.id, addon.type, false); >+ } >+};
Comment 104•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+var XPIDatabase = { >+ // True if the database connection has been opened >+ initialized: false, >+ // A cache of statements that are used and need to be finalized on shutdown >+ statementCache: {}, >+ // A cache of weak referenced DBAddonInternals so we can reuse objects where >+ // possible >+ addonCache: [], >+ >... >+ /** >+ * Shuts down the database connection and releases all cached objects. >+ */ >+ shutdown: function XPIDB_shutdown() { >+ if (this.initialized) { >+ for each (let stmt in this.statementCache) >+ stmt.finalize(); >+ this.statementCache = {}; >+ this.addonCache = []; >+ this.connection.asyncClose(); >+ this.initialized = false; >+ delete this.connection; >+ this.__defineGetter__("connection", function() { >+ return this.openConnection(); >+ }); This is to allow test restarts? If so please add a comment >... >+/** >+ * Instantiates an AddonInstall and passes the new object to a callback when >+ * it is complete. >+ * @param callback >+ * The callback to pass the AddonInstall to >+ * @param installLocation >+ * The install location the add-on will be installed into >+ * @param url >+ * The URL to get the add-on from Please note that this can be an nsIFileURL or...? >+ * @param hash >+ * An optional hash for the add-on >+ * @param name >+ * An optional name for the add-on >+ * @param type >+ * An optional type for the add-on >+ * @param iconURL >+ * An optional icon for the add-on >+ * @param version >+ * An optional version for the add-on >+ * @param infoURL >+ * An optional URL of release notes for the add-on >+ * @param existingAddon >+ * The add-on this install will update if known >+ * @param loadgroup >+ * The nsILoadGroup to associate any requests with >+ * @throws if the url is the url of a local file and the hash does not match >+ * or the add-on does not contain an valid install manifest >+ */ >+function AddonInstall(callback, installLocation, url, hash, name, type, iconURL, >+ version, infoURL, existingAddon, loadgroup) { >+ this.wrapper = new AddonInstallWrapper(this); >+ this.installLocation = installLocation; >+ this.sourceURL = url; >+ this.loadgroup = loadgroup; >+ this.listeners = []; >+ this.existingAddon = existingAddon; >+ >+ if (url instanceof Ci.nsIFileURL) { >+ this.file = url.file.QueryInterface(Ci.nsILocalFile); >+ this.state = AddonManager.STATE_DOWNLOADED; >+ this.progress = this.file.fileSize; >+ this.maxProgress = this.file.fileSize; progress and maxProgress here and elsewhere aren't really progress as the term is typically used... how about downloadedBytes and totalBytes or something similar? >+ >+ if (this.hash) { >+ let crypto = Cc["@mozilla.org/security/hash;1"]. >+ createInstance(Ci.nsICryptoHash); >+ let fis = Cc["@mozilla.org/network/file-input-stream;1"]. >+ createInstance(Ci.nsIFileInputStream); >+ fis.init(this.file, -1, -1, false); >+ crypto.updateFromStream(fis, this.file.fileSize); >+ let hash = crypto.finish(true); >+ if (hash != this.hash) >+ throw new Error("Hash mismatch"); >+ } >+ >... >+AddonInstall.prototype = { >+ installLocation: null, >+ wrapper: null, >+ stream: null, >+ crypto: null, >+ hash: null, >+ loadgroup: null, >+ listeners: null, >+ >+ name: null, >+ type: null, >+ version: null, >+ iconURL: null, >+ infoURL: null, >+ sourceURL: null, >+ file: null, >+ certificate: null, >+ certName: null, >+ >+ existingAddon: null, >+ addon: null, >+ >+ state: null, >+ progress: null, >+ maxProgress: null, >+ >+ /** >+ * Resumes installation of this add-on from whatever state it is currently at This comment implies that the add-on install was stopped and this will resume it which I don't think is correct. >+ * @throws if installation cannot proceed from the current state >+ */ >+ install: function AI_install() { Seems like this could be renamed to something that more appropriately describes what it does and startDownload and startInstall could just be download and install. >+ switch (this.state) { >+ case AddonManager.STATE_AVAILABLE: >+ this.startDownload(); >+ break; >+ case AddonManager.STATE_DOWNLOADED: >+ this.startInstall(); >+ break; >+ case AddonManager.STATE_DOWNLOADING: >+ case AddonManager.STATE_CHECKING: >+ case AddonManager.STATE_INSTALLING: >+ // Installation is already running >+ return; >+ default: >+ throw new Error("Cannot start installing from this state"); >+ } >+ }, >+ >+ /** >+ * Cancels installation of this add-on. >+ * @throws if installation cannot be cancelled from the current state >+ */ >+ cancel: function AI_cancel() { >+ switch (this.state) { >+ case AddonManager.STATE_DOWNLOADING: >+ break; >+ case AddonManager.STATE_AVAILABLE: >+ case AddonManager.STATE_DOWNLOADED: >+ LOG("Cancelling download of " + this.sourceURL.spec); >+ this.state = AddonManager.STATE_CANCELLED; >+ AddonManagerPrivate.callInstallListeners("onDownloadCancelled", >+ this.listeners, this.wrapper); >+ if (this.file && !(this.sourceURL instanceof Ci.nsIFileURL)) >+ this.file.remove(true); >+ break; >+ case AddonManager.STATE_INSTALLED: >+ LOG("Cancelling install of " + this.addon.id); >+ let stageDir = this.installLocation.getStagingDir(); >+ let stageFile = stageDir.clone(); I'd really like it if stageDir was renamed throughout to refer to the add-on's staged dir and stageFile named appropriately. >+ stageDir.append(this.addon.id); >+ stageFile.append(this.addon.id + ".json"); >+ if (stageDir.exists()) >+ stageDir.remove(true); >+ if (stageFile.exists()) >+ stageFile.remove(true); >+ this.state = AddonManager.STATE_CANCELLED; >+ AddonManagerPrivate.callInstallListeners("onInstallCancelled", >+ this.listeners, this.wrapper); >+ break; >+ default: >+ throw new Error("Cannot cancel from this state"); >+ } >+ }, >... >+ /** >+ * Starts downloading the add-on's XPI file. >+ */ >+ startDownload: function AI_startDownload() { >+ Components.utils.import("resource://gre/modules/CertUtils.jsm"); >+ >+ this.state = AddonManager.STATE_DOWNLOADING; >+ if (!AddonManagerPrivate.callInstallListeners("onDownloadStarted", >+ this.listeners, this.wrapper)) { >+ this.state = AddonManager.STATE_CANCELLED; >+ AddonManagerPrivate.callInstallListeners("onDownloadCancelled", >+ this.listeners, this.wrapper) >+ return; >+ } >+ >+ this.crypto = Cc["@mozilla.org/security/hash;1"]. >+ createInstance(Ci.nsICryptoHash); >+ if (this.hash) { >+ [alg, this.hash] = this.hash.split(":", 2); >+ try { >+ this.crypto.initWithString(alg); >+ } >+ catch (e) { >+ WARN("Unknown hash algorithm " + alg); >+ this.state = AddonManager.STATE_DOWNLOAD_FAILED; >+ AddonManagerPrivate.callInstallListeners("onDownloadFailed", >+ this.listeners, this.wrapper, >+ AddonManager.DOWNLOADERROR_INCORRECT_HASH); >+ return; >+ } >+ } >+ else { >+ // Just a dummy Please comment why the dummy is needed >+ this.crypto.initWithString("sha1"); >+ } >+ >+ try { >+ this.file = FileUtils.getDir("TmpD", ["tmp.xpi"]); Why not getFile? >+ this.file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE); Don't know for sure if this has been fixed else where but create and createUnique for a file are evil under some circumstances - see bug 300692. iirc this works ok when creating a directory per the comments in bug 300692. >+ this.stream = Cc["@mozilla.org/network/file-output-stream;1"]. >+ createInstance(Ci.nsIFileOutputStream); >+ this.stream.init(this.file, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | >+ FileUtils.MODE_TRUNCATE, FileUtils.PERMS_FILE, 0); >+ >+ let listener = Cc["@mozilla.org/network/stream-listener-tee;1"]. >+ createInstance(Ci.nsIStreamListenerTee); >+ listener.init(this, this.stream); >+ let channel = NetUtil.newChannel(this.sourceURL); >+ if (this.loadGroup) >+ channel.loadGroup = this.loadgroup; >+ >+ // Verify that we don't end up on an insecure channel if we haven't got a >+ // hash to verify with Please referenmce the bug on the issue you and I discussed >+ if (!this.hash) >+ channel.notificationCallbacks = new BadCertHandler(); >+ channel.asyncOpen(listener, null); >+ } >+ catch (e) { >+ WARN("Failed to start download: " + e); >+ this.state = AddonManager.STATE_DOWNLOAD_FAILED; >+ AddonManagerPrivate.callInstallListeners("onDownloadFailed", >+ this.listeners, this.wrapper, >+ AddonManager.DOWNLOADERROR_NETWORK); >+ } >+ }, >+ >+ /** >+ * Update the crypto hasher with the new data and call the progress listeners. >+ * @see nsIStreamListener >+ */ >+ onDataAvailable: function AI_onDataAvailable(request, context, inputstream, >+ offset, count) { >+ this.crypto.updateFromStream(inputstream, count); >+ this.progress += count; >+ if (!AddonManagerPrivate.callInstallListeners("onDownloadProgress", >+ this.listeners, this.wrapper)) { >+ // TODO cancel the download and make it available again Please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ } >... >+ try { >+ this.loadManifest(); >+ this.name = this.addon.selectedLocale.name; >+ this.type = this.addon.type; >+ this.version = this.addon.version; >+ // TODO fix this to not allow chrome URLs etc. >+ //this.iconURL = this.addon.iconURL; Please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ if (this.addon.isCompatible) { >+ this.downloadCompleted(); >+ } >+ else { >+ // TODO Should we send some event here? I'm not sure but it wouldn't hurt to file a bug for this >+ this.state = AddonManager.STATE_CHECKING; >+ let self = this; >+ new UpdateChecker(this.addon, { >+ onUpdateFinished: function(addon, status) { >+ self.downloadCompleted(); >+ } >+ }, AddonManager.UPDATE_WHEN_ADDON_INSTALLED); >+ } >+ } >+ catch (e) { >+ this.downloadFailed(AddonManager.DOWNLOADERROR_CORRUPT_FILE, e); >+ } >+ } >+ else { >+ if (request instanceof Ci.nsIHttpChannel) >+ this.downloadFailed(AddonManager.DOWNLOADERROR_NETWORK, >+ request.responseStatus + " " + request.responseStatusText); >+ else >+ this.downloadFailed(AddonManager.DOWNLOADERROR_NETWORK, status); >+ } >+ } >+ else { >+ this.downloadFailed(AddonManager.DOWNLOADERROR_NETWORK, status); >+ } >+ }, >+ >+ /** >+ * Helper to signal to listeners that the download failed. >+ * @param reason >+ * Something to log about the failure nit: confusing wording >+ * @param error >+ * The error code to pass to the listeners >+ */ >+ downloadFailed: function(reason, error) { >+ WARN("Download failed: " + error + "\n"); >+ this.state = AddonManager.STATE_DOWNLOAD_FAILED; >+ AddonManagerPrivate.callInstallListeners("onDownloadFailed", this.listeners, this.wrapper, reason); >+ this.file.remove(true); >+ }, >+ >+ /** >+ * Helper to signal to listeners that the download completed. >+ */ >+ downloadCompleted: function() { >+ let self = this; >+ XPIDatabase.getVisibleAddonForID(this.addon.id, function(addon) { >+ self.existingAddon = addon; >+ self.state = AddonManager.STATE_DOWNLOADED; >+ if (AddonManagerPrivate.callInstallListeners("onDownloadEnded", self.listeners, self.wrapper)) >+ self.install(); >+ }); >+ }, >+ >+ // TODO This relies on the assumption that we are always installing into the >+ // highest priority install location so the resulting add-on will be visible >+ // overriding any existing copy in another install location. Please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ /** >+ * Installs the add-on into the install location. >+ */ >+ startInstall: function AI_startInstall() { >+ this.state = AddonManager.STATE_INSTALLING; >+ if (!AddonManagerPrivate.callInstallListeners("onInstallStarted", >+ this.listeners, this.wrapper)) { >+ this.state = AddonManager.STATE_DOWNLOADED; >+ AddonManagerPrivate.callInstallListeners("onInstallCancelled", >+ this.listeners, this.wrapper) >+ return; >+ } >+ >+ let isUpgrade = this.existingAddon && >+ this.existingAddon._installLocation == this.installLocation.name; This is why I think the add-on's _installLocation property should be better named since it implies it is an installLocation when it is just the installLocation's name. >+ let requiresRestart = XPIProvider.installRequiresRestart(this.addon); >+ // Restarts is required if the existing add-on is active and disabling it >+ // requires a restart >+ if (!requiresRestart && this.existingAddon) { >+ requiresRestart = this.existingAddon.active && >+ XPIProvider.disableRequiresRestart(this.existingAddon); >+ } >+ >+ LOG("Starting install of " + this.sourceURL.spec); >+ AddonManagerPrivate.callAddonListeners("onInstalling", >+ createWrapper(this.addon), >+ requiresRestart); >+ let stageDir = this.installLocation.getStagingDir(); >+ try { >+ // First stage the file regardless of whether restarting is necessary >+ let stageFile = stageDir.clone(); >+ stageDir.append(this.addon.id); >+ if (stageDir.exists()) >+ stageDir.remove(true); >+ stageDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY); >+ extractFiles(this.file, stageDir); >+ >+ if (requiresRestart) { >+ // Cache the AddonInternal as it may have updated compatibiltiy info >+ stageFile.append(this.addon.id + ".json"); >+ if (stageFile.exists()) >+ stageFile.remove(true); >+ let stream = Cc["@mozilla.org/network/file-output-stream;1"]. >+ createInstance(Ci.nsIFileOutputStream); >+ let converter = Cc["@mozilla.org/intl/converter-output-stream;1"]. >+ createInstance(Ci.nsIConverterOutputStream); >+ let json = Cc["@mozilla.org/dom/json;1"]. >+ createInstance(Ci.nsIJSON); >+ try { >+ stream.init(stageFile, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | >+ FileUtils.MODE_TRUNCATE, FileUtils.PERMS_FILE, 0); >+ converter.init(stream, "UTF-8", 0, 0x0000); >+ >+ // A little hacky but we can't (and shouldn't) cache the source bundle. >+ let bundle = this.addon._sourceBundle; >+ delete this.addon._sourceBundle; >+ converter.writeString(json.encode(this.addon)); >+ this.addon._sourceBundle = bundle; >+ } >+ finally { >+ converter.close(); >+ stream.close(); >+ } >+ >+ LOG("Install of " + this.sourceURL.spec + " completed."); >+ this.state = AddonManager.STATE_INSTALLED; >+ AddonManagerPrivate.callInstallListeners("onInstallEnded", this.listeners, >+ this.wrapper, createWrapper(this.addon)); >+ } >+ else { >+ // TODO We can probably reduce the number of DB operations going on here >+ // We probably also want to support rolling back failed upgrades etc. I agree and please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ >+ // Deactivate and remove the old add-on as necessary >+ if (this.existingAddon) { >+ if (this.existingAddon.active) { >+ if (this.existingAddon.type == "bootstrapped") >+ XPIProvider.deactivateAddon(this.existingAddon.id, false, isUpgrade); >+ // If this is an upgrade its metadata will be removed below >+ if (!isUpgrade) { >+ this.existingAddon.active = false; >+ XPIDatabase.updateAddonActive(this.existingAddon); >+ } >+ } >+ if (isUpgrade) >+ this.installLocation.uninstallItem(this.existingAddon.id); >+ } >+ >+ // Install the new add-on into its final directory >+ let dir = this.installLocation.installItem(this.addon.id, stageDir); >+ >+ // Update the metadata in the database >+ this.addon._installLocation = this.installLocation.name; >+ this.addon.installDate = this.addon.updateDate = dir.lastModifiedTime; >+ this.addon.visible = true; >+ if (isUpgrade) { >+ this.addon.installDate = this.existingAddon.installDate; >+ XPIDatabase.updateAddonMetadata(this.existingAddon, this.addon, dir.persistentDescriptor); >+ } >+ else { >+ XPIDatabase.addAddonMetadata(this.addon, dir.persistentDescriptor); >+ } >+ >+ // Retrieve the new DBAddonInternal for the add-on we just added >+ let self = this; >+ XPIDatabase.getAddonInLocation(this.addon.id, this.installLocation.name, function(a) { >+ self.addon = a; >+ if (self.addon.active && self.addon.type == "bootstrapped") >+ XPIProvider.activateAddon(self.addon.id, dir, false, true); >+ AddonManagerPrivate.callAddonListeners("onInstalled", createWrapper(self.addon)); >+ >+ LOG("Install of " + self.sourceURL.spec + " completed."); >+ self.state = AddonManager.STATE_INSTALLED; >+ AddonManagerPrivate.callInstallListeners("onInstallEnded", self.listeners, self.wrapper, createWrapper(self.addon)); >+ }); >+ } >+ } >+ catch (e) { >+ WARN("Failed to install: " + e); >+ if (stageDir.exists()) >+ stageDir.remove(true); >+ this.state = AddonManager.STATE_INSTALL_FAILED; >+ AddonManagerPrivate.callInstallListeners("onInstallFailed", this.listeners, this.wrapper, e); >+ } >+ finally { >+ // If the file was downloaded then delete it >+ if (!(this.sourceURL instanceof Ci.nsIFileURL)) >+ this.file.remove(true); >+ } >+ } >+} There have been problems in the past where the xpi wasn't removed when done under some circumstances. Are there tests for this? If not, could it be tested and if so, could you file a bug to add the tests?
Comment 105•14 years ago
|
||
Comment on attachment 434398 [details] [diff] [review] xpiprovider rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+/** >+ * Creates a new AddonInstall to install a local. Missing the remainder of the comment >+ * @param callback >+ * The callback to pass the new AddonInstall to >+ * @param file >+ * The file to install >+ */ >+AddonInstall.createInstall = function(callback, file) { >+ let location = XPIProvider.installLocationsByName[KEY_APP_PROFILE]; Add comment that only the profile location is supported. >+ let url = Services.io.newFileURI(file); >+ try { >+ new AddonInstall(callback, location, url); >+ } >+ catch(e) { >+ callback(null); >+ } >+}; >+ >+/** >+ * Creates a new AddonInstall to download and install a URL. >+ * @param callback >+ * The callback to pass the new AddonInstall to >+ * @param uri >+ * The URI to download >+ * @param hash >+ * A hash for the add-on >+ * @param name >+ * A name for the add-on >+ * @param iconURL >+ * An icon for the add-on nit: 'An icon URL for the add-on'. I believe I nitted on this before in one location and you changed it... please check the patches for other occurences. >+ * @param version >+ * A version for the add-on >+ * @param loadgroup >+ * An nsILoadGroup to associate the download with >+ */ >+AddonInstall.createDownload = function(callback, uri, hash, name, iconURL, version, loadgroup) { >+ let location = XPIProvider.installLocationsByName[KEY_APP_PROFILE]; >+ let url = NetUtil.newURI(uri); >+ new AddonInstall(callback, location, url, hash, name, null, >+ iconURL, version, null, null, loadgroup); >+}; >+ >+/** >+ * Creates a new AddonInstall for an update. >+ * @param callback >+ * The callback to pass the new AddonInstall to >+ * @param addon >+ * The add-on being updated nit: The add-on to be update >+ * @param update >+ * The update details Not sure but perhaps 'The update metadata'? >+ */ >+AddonInstall.createUpdate = function(callback, addon, update) { >+ let location = XPIProvider.installLocationsByName[addon._installLocation]; >+ let url = NetUtil.newURI(update.updateURL); >+ let infoURL = null; >+ if (update.updateInfoURL) >+ infoURL = escapeAddonURI(addon, null, update.updateInfoURL); >+ new AddonInstall(callback, location, url, update.updateHash, >+ addon.selectedLocale.name, addon.type, >+ addon.iconURL, update.version, infoURL, addon); >+}; >... >+/** >+ * The AddonInternal is an internal only representation of known items. It may >+ * have come from the database (see DBAddonInternal below) or an install manifest. nits: confusing wording are there unknown items and what differentiates them from the known items? also, can items be replaced with add-ons here and else where? >... >+/** >+ * The DBAddonInternal is a special AddonInternal that has been retrieved from >+ * the database. Add-ons retrieved synchronously only have the basic metadata >+ * the rest is filled out synchronously when needed. Asynchronously read items >+ * have all data available. nit: confusing wording Add-ons retrieved synchronously only have the basic metadata the rest is filled out synchronously when needed >+ */ >+function DBAddonInternal() { >+ this.__defineGetter__("targetApplications", function() { >+ delete this.targetApplications; >+ return this.targetApplications = XPIDatabase._getTargetApplications(this); >+ }); >+ >+ this.__defineGetter__("locales", function() { >+ delete this.locales; >+ return this.locales = XPIDatabase._getLocales(this); >+ }); >+ >+ this.__defineGetter__("defaultLocale", function() { >+ delete this.defaultLocale; >+ return this.defaultLocale = XPIDatabase._getDefaultLocale(this); >+ }); >+} >+ >+DBAddonInternal.prototype = { >+ applyCompatibilityUpdate: function(update) { >+ let changes = []; >+ this.targetApplications.forEach(function(ta) { >+ update.targetApplications.forEach(function(updateTarget) { >+ if (ta.id == updateTarget.id && (ta.minVersion != updateTarget.minVersion || >+ ta.maxVersion != updateTarget.maxVersion)) { >+ ta.minVersion = updateTarget.minVersion; >+ ta.maxVersion = updateTarget.maxVersion; >+ changes.push(ta); >+ } >+ }); >+ }); >+ XPIDatabase.updateTargetApplications(this, changes); >+ XPIProvider.updateAddonDisabledState(this); >+ return changes.length > 0; >+ } >+} Looks like this always syncs compatibility info... previously, it only did this on app version change. >+DBAddonInternal.prototype.__proto__ = AddonInternal.prototype; >+ >+/** >+ * Creates an AddonWrapper for an AddonInternal. >+ * @param addon >+ * The AddonInternal to wrap >+ * @return an AddonWrapper or null if addon was null >+ */ >+function createWrapper(addon) { >+ if (!addon) >+ return null; >+ if (!addon.wrapper) >+ addon.wrapper = new AddonWrapper(addon); >+ return addon.wrapper; >+} >+ >+/** >+ * The AddonWrapper wraps an Addon to provide the data visible to public callers >+ * through the API. nit: Perhaps something similar to: The AddonWrapper wraps an Addon to provide the data visible to consumers of the public API. >+ */ >+function AddonWrapper(addon) { >+ ["id", "version", "type", "optionsURL", "aboutURL", "isCompatible", >+ "providesUpdatesSecurely", "blocklistState", "appDisabled", >+ "userDisabled"].forEach(function(prop) { >+ this.__defineGetter__(prop, function() addon[prop]); >+ }, this); >+ >+ ["installDate", "updateDate"].forEach(function(prop) { >+ this.__defineGetter__(prop, function() new Date(addon[prop])); >+ }, this); >+ >+ this.__defineGetter__("iconURL", function() { >+ return addon.active ? addon.iconURL : null; >+ }); >+ >+ PROP_LOCALE_SINGLE.forEach(function(prop) { >+ this.__defineGetter__(prop, function() addon.selectedLocale[prop]); >+ }, this); >+ >+ PROP_LOCALE_MULTI.forEach(function(prop) { >+ this.__defineGetter__(prop, function() addon.selectedLocale[prop]); >+ }, this); >+ >+ this.__defineGetter__("screenshots", function() { >+ return []; >+ }); >+ >+ this.__defineGetter__("updateAutomatically", function() addon.updateAutomatically); >+ this.__defineSetter__("updateAutomatically", function(val) { >+ // TODO store this in the DB Please note the bug number for this unless it is among the bugs you've already have a patch pending review. >+ addon.updateAutomatically = val; >+ }); >+ >+ this.__defineGetter__("pendingOperations", function() { >+ let pending = 0; >+ if (!(addon instanceof DBAddonInternal)) >+ pending |= AddonManager.PENDING_INSTALL; >+ if (addon.pendingUninstall) >+ pending |= AddonManager.PENDING_UNINSTALL; >+ if (addon.active && (addon.userDisabled || addon.appDisabled)) >+ pending |= AddonManager.PENDING_DISABLE; >+ if (!addon.active && (!addon.userDisabled && !addon.appDisabled)) >+ pending |= AddonManager.PENDING_ENABLE; >+ return pending; >+ }); Seems a tad strange to have checks for the addon being both PENDING_INSTALL / PENDING_UNINSTALL and PENDING_DISABLE / PENDING_ENABLE. >... Several comments in DirectoryInstallLocation use the term item where I think add-on would be more consistent / appropriate. I'm not positive that installItem / uninstallItem should also be renamed to installAddon / uninstallAddon but I am leaning that way. Thoughts? Several of the comments below apply to WinRegInstallLocation as well. Thanks for filing bug 555083 >+/** >+ * An object which identifies a directory install location for items. The >+ * location consists of a root directory. Each add-on installed in the location >+ * appears as either a directory or a simple test file with the name of the >+ * add-on's ID. There is also a staging directory where add-ons waiting to be >+ * installed are placed in the same format. Add-ons waiting to be uninstalled >+ * appear as an empty directory in the staging directory. nit: An object which identifies a directory install location for add-ons. nit: the following doesn't help with my understanding of what a DirectoryInstallLocation is while making me ask what is different between a directory and a root directory in this context. It is just a directory after all. 'The location consists of a root directory.' nit: a better word than 'appears' would be nice. Each add-on installed in the location appears as either a directory or a simple test file with the name of the add-on's ID in the directory install location directory. Perhaps: Add-ons can be installed into a directory install location as either a directory containing the add-on or a file with the add-on's ID for the name and the full path to the add-on for the contents. nit: same format as what? does the staging directory exist all of the time? There is also a staging directory where add-ons waiting to be installed are placed in the same format and add-ons waiting to be uninstalled appear as an empty directory in the staging directory. >+ * @param name >+ * The string identifier of the install location The string identifier for the install location >+ * @param location >+ * The root directory of the install location The directory for the install location I think this param should really be directory >+ * @param locked >+ * True if add-ons cannot be installed, uninstalled or upgraded in the >+ * install location >+ */ >+function DirectoryInstallLocation(name, location, locked) { >+ this._name = name; >+ if (location.exists() && !location.isDirectory()) >+ throw new Error("Location must be a directory."); >+ >+ this.locked = locked; >+ this._location = location; _directory as well? >+ this._IDToDirMap = {}; >+ this._DirToIDMap = {}; >+ >+ this._readAddons(); >+} >+ >+DirectoryInstallLocation.prototype = { >+ _name : "", >+ _location : null, >+ _IDToDirMap : null, // mapping from ID to directory object >+ _DirToIDMap : null, // mapping from directory path to ID s/ID/add-on ID/ both lines above >... >+ /** >+ * Finds all the items installed in this location. >+ */ >+ _readAddons: function DirInstallLocation__readAddons() { >+ try { >+ let entries = this._location.directoryEntries >+ .QueryInterface(Ci.nsIDirectoryEnumerator); >+ let entry; >+ while (entry = entries.nextFile) { >+ // Should never happen really >+ if (!entry instanceof Ci.nsILocalFile) >+ continue; >+ >+ let id = entry.leafName; >+ >+ if (id == DIR_STAGE) >+ continue; >+ >+ if (!gIDTest.test(id)) { >+ LOG("Ignoring file entry whose name is not a valid add-on ID: " + >+ entry.path); >+ continue; >+ } >+ >+ // XXX Bug 530188 requires us to clone this entry >+ entry = this._location.clone().QueryInterface(Ci.nsILocalFile); >+ entry.append(id); >+ if (entry.isFile()) { >+ newEntry = this._readDirectoryFromFile(entry); >+ if (!newEntry || !newEntry.exists()) { Seems like there should be an isDirectory check here >... >+ /** >+ * Gets the staging directory to put items that are pending install into. This is also used for uninstall now >+ * @return an nsIFile >+ */ >+ getStagingDir: function DirInstallLocation_getStagingDir() { >+ let dir = this._location.clone(); >+ dir.append(DIR_STAGE); >+ return dir; >+ }, >+ I suspect the next pass will be an r+
Attachment #434398 -
Flags: review?(robert.bugzilla) → review-
Comment 106•14 years ago
|
||
Comment on attachment 434399 [details] [diff] [review] lwtheme rev 2 >diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm >--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm >+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm >@@ -39,6 +39,14 @@ var EXPORTED_SYMBOLS = ["LightweightThem > const Cc = Components.classes; > const Ci = Components.interfaces; > >+Components.utils.import("resource://gre/modules/AddonManager.jsm"); >+Components.utils.import("resource://gre/modules/Services.jsm"); >+ >+const ID_SUFFIX = "@personas.mozilla.org"; >+const PREF_LWTHEME_TO_SELECT = "extensions.lwThemeToSelect"; >+const PREF_GENERAL_SKINS_SELECTEDSKIN = "general.skins.selectedSkin"; >+const ADDON_TYPE = "theme"; >+ > const MAX_USED_THEMES_COUNT = 8; > > const MAX_PREVIEW_SECONDS = 30; >@@ -48,7 +56,7 @@ const OPTIONAL = ["footerURL", "textcolo > "previewURL", "author", "description", "homepageURL", > "updateURL", "version"]; > >-const PERSIST_ENABLED = true; >+const PERSIST_ENABLED = false; > const PERSIST_BYPASS_CACHE = false; What are these constants used for? Developer testing? Why did you flip the value of PERSIST_ENABLED? If they are needed / desired please add a comment describing what they are used for. The same goes to a lesser degree for ADDON_TYPE but I suspect this is mainly for consistency with other implementations. > const PERSIST_FILES = { > headerURL: "lightweighttheme-header", >@@ -112,36 +120,41 @@ var LightweightThemeManager = { > set currentTheme (aData) { > aData = _sanitizeTheme(aData); > >+ let needsRestart = (ADDON_TYPE == "theme") && >+ Services.prefs.prefHasUserValue(PREF_GENERAL_SKINS_SELECTEDSKIN); What is the point of checking if the constant ADDON_TYPE which always equals theme equals theme?
Comment 107•14 years ago
|
||
Comment on attachment 434399 [details] [diff] [review] lwtheme rev 2 Quick reminder about supporting or dropping support for the xpi file drop install in the app extensions directory. There is a surprising lack of comments in this code :( Is there a reason for this? >diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm >+const ID_SUFFIX = "@personas.mozilla.org"; What purpose is served by suffixing persona id's? >... >@@ -112,36 +120,41 @@ var LightweightThemeManager = { >... >+ let theme = this.getUsedTheme(aData.id); >+ if (!theme) { >+ var wrapper = new AddonWrapper(aData); >+ AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false); >+ } >+ let current = this.currentTheme; >+ if (!current || current.id != aData.id) { >+ let usedThemes = _usedThemesExceptId(aData.id); >+ if (current) >+ usedThemes.splice(1, 0, aData); >+ else >+ usedThemes.unshift(aData); >+ _updateUsedThemes(usedThemes); >+ } >+ if (!theme) { >+ AddonManagerPrivate.callAddonListeners("onInstalled", wrapper); >+ } nit: unneeded braces >@@ -154,11 +167,21 @@ var LightweightThemeManager = { > }, > > forgetUsedTheme: function (aId) { >+ let theme = this.getUsedTheme(aId); >+ if (!theme) >+ return; >+ >+ let wrapper = new AddonWrapper(theme); >+ AddonManagerPrivate.callAddonListeners("onUninstalling", wrapper, false); >+ > var currentTheme = this.currentTheme; >- if (currentTheme && currentTheme.id == aId) >- this.currentTheme = null; >+ if (currentTheme && currentTheme.id == aId) { >+ _prefs.setBoolPref("isThemeSelected", false); >+ AddonManagerPrivate.notifyAddonChanged(null, ADDON_TYPE, false); >+ } > > _updateUsedThemes(_usedThemesExceptId(aId)); >+ AddonManagerPrivate.callAddonListeners("onUninstalled", wrapper); > }, > > previewTheme: function (aData) { >@@ -235,9 +258,206 @@ var LightweightThemeManager = { > }; > > req.send(null); >+ }, >+ >+ themeChanged: function(aData) { >+ if (_previewTimer) { >+ _previewTimer.cancel(); >+ _previewTimer = null; >+ } >+ >+ if (aData) { >+ let usedThemes = _usedThemesExceptId(aData.id); >+ usedThemes.unshift(aData); >+ _updateUsedThemes(usedThemes); >+ if (PERSIST_ENABLED) >+ _persistImages(aData); >+ } >+ >+ _prefs.setBoolPref("isThemeSelected", aData != null); >+ _notifyWindows(aData); >+ _observerService.notifyObservers(null, "lightweight-theme-changed", null); >+ }, >+ >+ startup: function() { >+ try { >+ let id = Services.prefs.getCharPref(PREF_LWTHEME_TO_SELECT); >+ if (id) >+ this.themeChanged(this.getUsedTheme(id)); >+ else >+ this.themeChanged(null); >+ Services.prefs.clearUserPref(PREF_LWTHEME_TO_SELECT); >+ } >+ catch (e) { >+ } >+ }, Seems like this should just use prefHasUserValue... you can get rid of the try catch then as well. Relying on an empty string for the pref is hacky. As a side note, I wonder if someone will ever want to ship a default lightweight theme with an app. >+ >+ addonChanged: function(id, type, pendingRestart) { >+ if (type != ADDON_TYPE) >+ return; >+ >+ let id = _getInternalID(id); >+ let current = this.currentTheme; >+ >+ try { >+ let next = Services.prefs.getCharPref(PREF_LWTHEME_TO_SELECT); >+ if (id == next && pendingRestart) >+ return; >+ >+ Services.prefs.clearUserPref(PREF_LWTHEME_TO_SELECT); >+ if (next) { >+ AddonManagerPrivate.callAddonListeners("onOperationCancelled", >+ new AddonWrapper(this.getUsedTheme(next))); >+ } >+ else { >+ if (id == current.id) { >+ AddonManagerPrivate.callAddonListeners("onOperationCancelled", >+ new AddonWrapper(current)); >+ return; >+ } >+ } >+ } >+ catch (e) { >+ } >+ >+ if (current) { >+ if (current.id == id) >+ return; >+ let wrapper = new AddonWrapper(current); >+ if (pendingRestart) { >+ Services.prefs.setCharPref(PREF_LWTHEME_TO_SELECT, ""); >+ AddonManagerPrivate.callAddonListeners("onDisabling", wrapper, true); >+ } >+ else { >+ AddonManagerPrivate.callAddonListeners("onDisabling", wrapper, false); >+ this.themeChanged(null); >+ AddonManagerPrivate.callAddonListeners("onDisabled", wrapper); >+ } >+ } >+ >+ if (id) { >+ let theme = this.getUsedTheme(id); >+ let wrapper = new AddonWrapper(theme); >+ if (pendingRestart) { >+ Services.prefs.setCharPref(PREF_LWTHEME_TO_SELECT, id); >+ AddonManagerPrivate.callAddonListeners("onEnabling", wrapper, true); >+ } >+ else { >+ this.themeChanged(theme); >+ AddonManagerPrivate.callAddonListeners("onEnabling", wrapper, false); >+ AddonManagerPrivate.callAddonListeners("onEnabled", wrapper); >+ } Why isn't themeChanged; between onEnabling and onEnabled? >+ } >+ }, >+ >+ getAddon: function(id, callback) { >+ id = _getInternalID(id); >+ if (id) { >+ let theme = this.getUsedTheme(id); >+ if (theme) { >+ callback(new AddonWrapper(theme)); >+ return; >+ } >+ } >+ callback(null); >+ }, >+ >+ getAddonsByType: function(types, callback) { >+ if (types && types.indexOf(ADDON_TYPE) < 0) { Why not just == -1? >+ callback([]); >+ return; >+ } >+ >+ callback([new AddonWrapper(a) for each (a in this.usedThemes)]); >+ }, nit: in getAddon you return early for when you have found a theme and in getAddonsByType you return early when you don't handle the add-on type requested. It would be simple to change for consistency. >+}; >+ >+function AddonWrapper(theme) { >+ this.__defineGetter__("id", function() theme.id + ID_SUFFIX); >+ this.__defineGetter__("type", function() ADDON_TYPE); >+ this.__defineGetter__("isActive", function() { >+ let current = LightweightThemeManager.currentTheme; >+ if (current) >+ return theme.id == current.id; >+ return false; >+ }); >+ >+ ["name", "version", "description", "homepageURL", "iconURL"].forEach(function(prop) { >+ this.__defineGetter__(prop, function() theme[prop]); >+ }, this); >+ this.__defineGetter__("creator", function() theme.author); >+ this.__defineGetter__("screenshots", function() [theme.previewURL]); >+ >+ this.__defineGetter__("pendingOperations", function() { >+ let pending = 0; hmmm... might be worthwhile adding a PENDING_NONE or something along those lines for these cases. >+ if (this.isActive == this.userDisabled) >+ pending |= this.isActive ? AddonManager.PENDING_DISABLE : AddonManager.PENDING_ENABLE; >+ return pending; >+ }); >+ >+ this.__defineGetter__("permissions", function() { >+ let permissions = AddonManager.PERM_CANUNINSTALL; >+ if (this.userDisabled) >+ permissions |= AddonManager.PERM_CANENABLE; >+ return permissions; >+ }); >+ >+ this.__defineGetter__("userDisabled", function() { >+ try { >+ let toSelect = Services.prefs.getCharPref(PREF_LWTHEME_TO_SELECT); >+ return theme.id != toSelect; >+ } >+ catch (e) { >+ let current = LightweightThemeManager.currentTheme; >+ return !current || current.id != theme.id; >+ } >+ }); >+ this.__defineSetter__("userDisabled", function(val) { >+ if (val) >+ throw new Error("Cannot disable the active theme"); >+ LightweightThemeManager.currentTheme = theme; >+ }); Seems a little strange to me that this should throw. Also, can it only be called for the active theme per what is thrown? >+ >+ this.uninstall = function() { >+ LightweightThemeManager.forgetUsedTheme(theme.id); >+ }; >+ >+ this.cancelUninstall = function() { >+ if (!addon.pendingUninstall) >+ throw new Error("Item is not marked to be uninstalled"); >+ }; addon.pendingUninstall seems wrong. Is there a way to cancel the uninstall of a lwtheme? >+ >+ this.findUpdates = function(listener, reason, appVersion, platformVersion) { >+ }; >+} >+ >+AddonWrapper.prototype = { >+ get appDisabled() { >+ return false; >+ }, >+ >+ get isCompatible() { >+ return false; true? Minusing mainly because of the questions I've asked.
Attachment #434399 -
Flags: review?(robert.bugzilla) → review-
Comment 108•14 years ago
|
||
(In reply to comment #107) > (From update of attachment 434399 [details] [diff] [review]) >... > >+ get isCompatible() { > >+ return false; > true? Just saw bug 553777
Comment 109•14 years ago
|
||
Comment on attachment 434785 [details] [diff] [review] browsertests rev 2 Very nice... in several of the test descriptions in multiple files s/build-in/built-in/ Other than that looks good
Attachment #434785 -
Flags: review?(robert.bugzilla) → review+
Comment 110•14 years ago
|
||
Comment on attachment 434786 [details] [diff] [review] xpcshelltests rev 2 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >--- a/toolkit/mozapps/extensions/XPIProvider.jsm >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm >@@ -3388,13 +3388,14 @@ AddonInstall.prototype = { > > // Update the metadata in the database > this.addon._installLocation = this.installLocation.name; >- this.addon.installDate = this.addon.updateDate = dir.lastModifiedTime; >+ this.addon.updateDate = dir.lastModifiedTime; > this.addon.visible = true; > if (isUpgrade) { > this.addon.installDate = this.existingAddon.installDate; > XPIDatabase.updateAddonMetadata(this.existingAddon, this.addon, dir.persistentDescriptor); > } > else { >+ this.addon.installDate = this.addon.updateDate; > XPIDatabase.addAddonMetadata(this.addon, dir.persistentDescriptor); > } Forgot to comment on this initially... much better >diff --git a/toolkit/mozapps/extensions/test/addons/test_bootstrap2/install.rdf b/toolkit/mozapps/extensions/test/addons/test_bootstrap2/install.rdf >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/test/addons/test_bootstrap2/install.rdf >@@ -0,0 +1,24 @@ >+<?xml version="1.0"?> >+ >+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" >+ xmlns:em="http://www.mozilla.org/2004/em-rdf#"> >+ >+ <Description about="urn:mozilla:install-manifest"> >+ <em:id>bootstrap1@tests.mozilla.org</em:id> Found the id a tad confusing at first since this is in the test_bootstrap2 directory. Could you use test_bootstrap1-1 and test_bootstrap1-2 or something similar for directory names? Not a big deal if it is a hassle
Comment 111•14 years ago
|
||
Comment on attachment 434786 [details] [diff] [review] xpcshelltests rev 2 >diff --git a/toolkit/mozapps/extensions/test/xpcshell/data/test_migrate.rdf b/toolkit/mozapps/extensions/test/xpcshell/data/test_migrate.rdf >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/test/xpcshell/data/test_migrate.rdf >@@ -0,0 +1,56 @@ >+<?xml version="1.0"?> >+ >+<!-- This is a copy of extensions.rdf from Firefox 3.5 including four >+ test extensions. Addon1 is enabled, addon2 is disabled, addon3 was pending >+ disable at the next restart and addon4 was pending enable at the next >+ restart. --> tiny nit: should just use 'is' instead of 'was' since that is the state it currently is in. >diff --git a/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf b/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/test/xpcshell/data/test_update.rdf >@@ -0,0 +1,57 @@ >+<?xml version="1.0" encoding="UTF-8"?> >+ >+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" >+ xmlns:em="http://www.mozilla.org/2004/em-rdf#"> >+ >+ <Description about="urn:mozilla:extension:addon1@tests.mozilla.org"> >+ <em:updates> >+ <Seq> >+ <li> >+ <Description> >+ <em:version>1.0</em:version> >+ <em:targetApplication> >+ <Description> >+ <em:id>xpcshell@tests.mozilla.org</em:id> >+ <em:minVersion>1</em:minVersion> >+ <em:maxVersion>1</em:maxVersion> >+ </Description> >+ </em:targetApplication> >+ </Description> >+ </li> >+ <li> >+ <Description> >+ <em:version>2.0</em:version> >+ <em:targetApplication> >+ <Description> >+ <em:id>xpcshell@tests.mozilla.org</em:id> >+ <em:minVersion>1</em:minVersion> >+ <em:maxVersion>1</em:maxVersion> >+ <em:updateLink>http://localhost:4444/addons/test_update.xpi</em:updateLink> >+ </Description> >+ </em:targetApplication> >+ </Description> >+ </li> >+ </Seq> >+ </em:updates> >+ </Description> >+ >+ tiny nit: extra newline >+ <Description about="urn:mozilla:extension:addon2@tests.mozilla.org"> >diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js >@@ -0,0 +1,450 @@ >+/* Any copyright is dedicated to the Public Domain. >+ * http://creativecommons.org/publicdomain/zero/1.0/ >+ */ This is great but I wish I had remembered this for the mochitests I added a short while ago. :( >+const AM_Cc = Components.classes; >+const AM_Ci = Components.interfaces; >+ >+const NS_APP_USER_PROFILE_50_DIR = "ProfD"; Is this still used? >... >+// Need to create and register a profile folder. >+const gProfD = do_get_profile().QueryInterface(AM_Ci.nsILocalFile); nit: the comment above is no longer true
Updated•14 years ago
|
Attachment #434786 -
Flags: review?(robert.bugzilla) → review+
Comment 112•14 years ago
|
||
Comment on attachment 434786 [details] [diff] [review] xpcshelltests rev 2 Thanks Alex... I didn't go through them anywhere near as thoroughly as you did. With comments addressed r=me
Comment 113•14 years ago
|
||
Comment on attachment 435043 [details] [diff] [review] xpinstall rev 2 Don't forget about fixing up xulrunner and other apps... are there bugs for this? >Bug 553169: Implement InstallTrigger, the XPI content handler and confirmation for web triggered installs. > >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >--- a/browser/app/profile/firefox.js >+++ b/browser/app/profile/firefox.js >@@ -52,11 +52,6 @@ pref("general.startup.browser", true); > > pref("browser.chromeURL","chrome://browser/content/"); > pref("browser.hiddenWindowChromeURL", "chrome://browser/content/hiddenWindow.xul"); >-pref("xpinstall.dialog.confirm", "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul"); >-pref("xpinstall.dialog.progress.skin", "chrome://mozapps/content/extensions/extensions.xul"); >-pref("xpinstall.dialog.progress.chrome", "chrome://mozapps/content/extensions/extensions.xul"); >-pref("xpinstall.dialog.progress.type.skin", "Extension:Manager"); >-pref("xpinstall.dialog.progress.type.chrome", "Extension:Manager"); > > // Developers can set this to |true| if they are constantly changing files in their > // extensions directory so that the extension system does not constantly think that >@@ -173,6 +168,8 @@ pref("extensions.dss.switchPending", fal > pref("extensions.{972ce4c6-7e08-4474-a285-3208198ce6fd}.name", "chrome://browser/locale/browser.properties"); > pref("extensions.{972ce4c6-7e08-4474-a285-3208198ce6fd}.description", "chrome://browser/locale/browser.properties"); > >+pref("xpinstall.enabled", true); >+pref("xpinstall.whitelist.required", true); > pref("xpinstall.whitelist.add", "addons.mozilla.org"); > pref("xpinstall.whitelist.add.36", "getpersonas.com"); Probably not worth changing since people already have whitelist entries and the prefs are well known. >diff --git a/toolkit/mozapps/extensions/amIInstallTrigger.idl b/toolkit/mozapps/extensions/amIInstallTrigger.idl >... >+ /** >+ * Tests if installation is enabled. >+ * Please use "enabled" in the future. Might as well comment that this deprecated >+ */ >+ boolean updateEnabled(); >+ >+ /** >+ * Tests if installation is enabled. >+ */ >+ boolean enabled(); >+ >+ /** >+ * Starts a new installation of a set of add-ons. >+ * @param args >+ * The add-ons to install. This should be a JS object, each property >+ * is the name of an add-on to be installed. The value of the >+ * property should either be a string URL, or an object with the >+ * following properties: >+ * * URL for the add-on's URL >+ * * IconURL for an icon for the add-on >+ * * Hash for a hash of the add-on >+ * @param callback >+ * A callback to call as each installation succeeds or fails >+ * @return true if the installations were successfully started >+ */ >+ boolean install(in nsIVariant args, [optional] in amIInstallCallback callback); >+ >+ /** >+ * Starts installing a new add-on >+ * @param type >+ * Unused, retained for backwards compatibility >+ * @param url >+ * The URL of the add-on >+ * @param skin >+ * Unused, retained for backwards compatibility >+ * @return true if the installation was successfully started >+ */ >+ boolean installChrome(in PRUint32 type, in AString url, in AString skin); >+ >+ /** >+ * Starts installing a new add-on >+ * @param url >+ * The URL of the add-on >+ * @param flags >+ * Unused, retained for backwards compatibility >+ * @return true if the installation was successfully started >+ */ >+ boolean startSoftwareUpdate(in AString url, [optional] in PRInt32 flags); Is it worth coming up with a new name now and marking this as deprecated? >diff --git a/toolkit/mozapps/extensions/amIWebInstallListener.idl b/toolkit/mozapps/extensions/amIWebInstallListener.idl >... >+/** >+ * The registered amIWebInstallListener is used to notify about new installs >+ * triggered by websites. The default implementation displays a confirmation >+ * dialog when add-ons are ready to install and uses the observer service to >+ * notify when installations are blocked. >+ */ >+[scriptable, uuid(ea806f3a-1b27-4d3d-9aee-88dec4c29fda)] >+interface amIWebInstallListener : nsISupports >+{ >+ /** >+ * Called when an attempt to install a file from the web is blocked for >+ * security reasons. I'd avoid the term security reasons. The stance in the past for a site trying to install not being whitelisted is this prevents an annoyance and doesn't provide actual security. It is closer to a popup blocker. Dave, what do you think of dveditz reviewing this patch... he is much more familiar with the original code than I am.
Comment 114•14 years ago
|
||
Comment on attachment 434787 [details] [diff] [review] xpinstalltests rev 2 interdiff to the rescue :) What about the following tests from xpinstall? browser_opendialog.js browser_chrome.js browser_cancel.js >diff --git a/toolkit/mozapps/extensions/test/xpinstall/browser_signed_multiple.js b/toolkit/mozapps/extensions/test/xpinstall/browser_signed_multiple.js >... >+function confirm_install(window) { >+ items = window.document.getElementById("itemList").childNodes; >+ is(items.length, 2, "Should be 2 items listed in the confirmation dialog"); >+ let item = get_item(items, TESTROOT + "signed.xpi"); >+ if (item) { >+ is(item.name, "Signed XPI Test", "Should have seen the name from the trigger list"); >+ is(item.cert, "(Object Signer)", "Should have seen the signer"); >+ is(item.signed, "true", "Should have listed the item as signed"); >+ } >+ item = get_item(items, TESTROOT + "signed2.xpi"); >+ if (item) { >+ is(item.name, "Signed XPI Test", "Should have seen the name from the trigger list"); >+ is(item.cert, "(Object Signer)", "Should have seen the signer"); >+ is(item.signed, "true", "Should have listed the item as signed"); >+ } Why did you remove the check for the url here and in other tests? r=me with answers to these questions and / or the removed tests added.
Attachment #434787 -
Flags: review?(robert.bugzilla) → review+
Comment 115•14 years ago
|
||
I'm really sorry I wasn't able to finish the test reviews before you got to them, Robert. I would have liked to finish, but I have a lot less time to work on mozilla.org stuff than you or Dave do. :)
Comment 116•14 years ago
|
||
Comment on attachment 435043 [details] [diff] [review] xpinstall rev 2 Talked with Dave and he is going to resubmit this patch with the fixes from bug 553135 and request review from dveditz
Updated•14 years ago
|
Whiteboard: [rewrite] → [rewrite][needs-patch]
Updated•14 years ago
|
Attachment #435043 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 117•14 years ago
|
||
> >+ // If the application crashed before completing any pending operations then
> >+ // we should perform them now.
> >+ if (changed || Prefs.getBoolPref(PREF_PENDING_OPERATIONS)) {
> >+ LOG("Restart necessary");
> >+ XPIDatabase.updateActiveAddons();
> >+ Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, false);
> >+ return true;
> >+ }
> >+
> >+ LOG("No changes found");
> Is this true or just cruft? You return true indicating that a change requiring
> a restart was detected before and after this log statement
It is true in that if the code reaches this point no changes to the installed add-ons have been found. The only case that we return true after this is when extensions.ini is missing in which case we must re-create it and force the platform to restart even though there has been no change to the installed add-ons.
Assignee | ||
Comment 118•14 years ago
|
||
(In reply to comment #104) > >+ if (url instanceof Ci.nsIFileURL) { > >+ this.file = url.file.QueryInterface(Ci.nsILocalFile); > >+ this.state = AddonManager.STATE_DOWNLOADED; > >+ this.progress = this.file.fileSize; > >+ this.maxProgress = this.file.fileSize; > progress and maxProgress here and elsewhere aren't really progress as the term > is typically used... how about downloadedBytes and totalBytes or something > similar? We seem to use variants of progress and maxProgress in places like nsIWebProgressListener, nsIProgressEventSink. If you still want it changed I can do though. > >+ this.file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE); > Don't know for sure if this has been fixed else where but create and > createUnique for a file are evil under some circumstances - see bug 300692. > iirc this works ok when creating a directory per the comments in bug 300692. Used the code suggested in bug 554307. > >+ finally { > >+ // If the file was downloaded then delete it > >+ if (!(this.sourceURL instanceof Ci.nsIFileURL)) > >+ this.file.remove(true); > >+ } > >+ } > >+} > There have been problems in the past where the xpi wasn't removed when done > under some circumstances. Are there tests for this? If not, could it be tested > and if so, could you file a bug to add the tests? Filed bug 557719.
Comment 119•14 years ago
|
||
(In reply to comment #118) > (In reply to comment #104) > > >+ if (url instanceof Ci.nsIFileURL) { > > >+ this.file = url.file.QueryInterface(Ci.nsILocalFile); > > >+ this.state = AddonManager.STATE_DOWNLOADED; > > >+ this.progress = this.file.fileSize; > > >+ this.maxProgress = this.file.fileSize; > > progress and maxProgress here and elsewhere aren't really progress as the term > > is typically used... how about downloadedBytes and totalBytes or something > > similar? > > We seem to use variants of progress and maxProgress in places like > nsIWebProgressListener, nsIProgressEventSink. If you still want it changed I > can do though. That's fine
Assignee | ||
Comment 120•14 years ago
|
||
(In reply to comment #105) > >+DBAddonInternal.prototype = { > >+ applyCompatibilityUpdate: function(update) { > >+ let changes = []; > >+ this.targetApplications.forEach(function(ta) { > >+ update.targetApplications.forEach(function(updateTarget) { > >+ if (ta.id == updateTarget.id && (ta.minVersion != updateTarget.minVersion || > >+ ta.maxVersion != updateTarget.maxVersion)) { > >+ ta.minVersion = updateTarget.minVersion; > >+ ta.maxVersion = updateTarget.maxVersion; > >+ changes.push(ta); > >+ } > >+ }); > >+ }); > >+ XPIDatabase.updateTargetApplications(this, changes); > >+ XPIProvider.updateAddonDisabledState(this); > >+ return changes.length > 0; > >+ } > >+} > Looks like this always syncs compatibility info... previously, it only did this > on app version change. This is taken care of in bug 552732 > Several comments in DirectoryInstallLocation use the term item where I think > add-on would be more consistent / appropriate. I'm not positive that > installItem / uninstallItem should also be renamed to installAddon / > uninstallAddon but I am leaning that way. Thoughts? I've purged the term "item" only leaving it in the add-on URI escapes and PREFIX_ITEM_URI where I think it still makes sense.
Assignee | ||
Comment 121•14 years ago
|
||
Updated with comments. The only bit I haven't changed is the naming of install/startDownload/startInstall. Do you have some suggestions for those? The latter two seem to make sense to me as they can both be asynchronous operations so they are just being started. The former is the public method that the API exposes to make everything go.
Attachment #434398 -
Attachment is obsolete: true
Attachment #437625 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 122•14 years ago
|
||
Comment on attachment 434396 [details] [diff] [review] toolkit rev 2 This stops nsAppRunner from needing to call the extension manager directly and just allows us to signal that a restart is necessary.
Attachment #434396 -
Flags: review?(benjamin)
Assignee | ||
Updated•14 years ago
|
Attachment #437625 -
Attachment description: xpinstall rev 3 → xpiprovider rev 3
Assignee | ||
Comment 123•14 years ago
|
||
Dan this patch replaces the bulk of xpinstall with a small C++ component that handles passing the arguments from webpage calls to InstallTrigger to the new extension manager which now handles all of the downloading and installing by itself.
Attachment #437630 -
Flags: review?(dveditz)
Assignee | ||
Updated•14 years ago
|
Attachment #435043 -
Attachment is obsolete: true
Assignee | ||
Comment 124•14 years ago
|
||
(In reply to comment #107) > (From update of attachment 434399 [details] [diff] [review]) > Quick reminder about supporting or dropping support for the xpi file drop > install in the app extensions directory. Dropping support I think. We can add it back later if we find a good reason. I probably need to make a list of this stuff for the docs/blogs. > There is a surprising lack of comments in this code :( Is there a reason for > this? > > >diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm > > >+const ID_SUFFIX = "@personas.mozilla.org"; > What purpose is served by suffixing persona id's? At the moment personas use a very basic string (actually just a number) as an ID. I guess having a suffix made them feel a bit more ID-like, but I suppose it isn't really necessary. > >+ startup: function() { > >+ try { > >+ let id = Services.prefs.getCharPref(PREF_LWTHEME_TO_SELECT); > >+ if (id) > >+ this.themeChanged(this.getUsedTheme(id)); > >+ else > >+ this.themeChanged(null); > >+ Services.prefs.clearUserPref(PREF_LWTHEME_TO_SELECT); > >+ } > >+ catch (e) { > >+ } > >+ }, > Relying on an empty string for the pref is hacky. It might be worth changing in the future, but it is what we do on trunk right now so I'd prefer to keep it as is for now. > >+ if (id) { > >+ let theme = this.getUsedTheme(id); > >+ let wrapper = new AddonWrapper(theme); > >+ if (pendingRestart) { > >+ Services.prefs.setCharPref(PREF_LWTHEME_TO_SELECT, id); > >+ AddonManagerPrivate.callAddonListeners("onEnabling", wrapper, true); > >+ } > >+ else { > >+ this.themeChanged(theme); > >+ AddonManagerPrivate.callAddonListeners("onEnabling", wrapper, false); > >+ AddonManagerPrivate.callAddonListeners("onEnabled", wrapper); > >+ } > Why isn't themeChanged; between onEnabling and onEnabled? The problem is that in onEnabling the tests expect to see that the theme doesn't have PERM_CAN_ENABLE, but until the theme is changed it does. I've worked around that by setting an additional parameter for AddonWrappers that represent themes in the process of being enabled. > >+ getAddonsByType: function(types, callback) { > >+ if (types && types.indexOf(ADDON_TYPE) < 0) { > Why not just == -1? Once upon a time I worked in a language where it was only guaranteed to be < 0, not necessarily -1. Old habits die hard. > >+ this.__defineSetter__("userDisabled", function(val) { > >+ if (val) > >+ throw new Error("Cannot disable the active theme"); > >+ LightweightThemeManager.currentTheme = theme; > >+ }); > Seems a little strange to me that this should throw. Also, can it only be > called for the active theme per what is thrown? Stopped it throwing in the case that userDisabled is already true. The idea I'm going for here is that you can't just disable a theme through the API, you must enable a different theme and the API will then take care of disabling the previously active one.
Assignee | ||
Comment 125•14 years ago
|
||
(In reply to comment #106) > (From update of attachment 434399 [details] [diff] [review]) > >diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm > >--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm > >+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm > >@@ -39,6 +39,14 @@ var EXPORTED_SYMBOLS = ["LightweightThem > > const Cc = Components.classes; > > const Ci = Components.interfaces; > > > >+Components.utils.import("resource://gre/modules/AddonManager.jsm"); > >+Components.utils.import("resource://gre/modules/Services.jsm"); > >+ > >+const ID_SUFFIX = "@personas.mozilla.org"; > >+const PREF_LWTHEME_TO_SELECT = "extensions.lwThemeToSelect"; > >+const PREF_GENERAL_SKINS_SELECTEDSKIN = "general.skins.selectedSkin"; > >+const ADDON_TYPE = "theme"; > >+ > > const MAX_USED_THEMES_COUNT = 8; > > > > const MAX_PREVIEW_SECONDS = 30; > >@@ -48,7 +56,7 @@ const OPTIONAL = ["footerURL", "textcolo > > "previewURL", "author", "description", "homepageURL", > > "updateURL", "version"]; > > > >-const PERSIST_ENABLED = true; > >+const PERSIST_ENABLED = false; > > const PERSIST_BYPASS_CACHE = false; > What are these constants used for? Developer testing? Why did you flip the > value of PERSIST_ENABLED? There is an existing problem (bug ) where the xpcshell tests leak. so I just turned it off for testing. > If they are needed / desired please add a comment describing what they are used > for. The same goes to a lesser degree for ADDON_TYPE but I suspect this is > mainly for consistency with other implementations. See below > > const PERSIST_FILES = { > > headerURL: "lightweighttheme-header", > >@@ -112,36 +120,41 @@ var LightweightThemeManager = { > > set currentTheme (aData) { > > aData = _sanitizeTheme(aData); > > > >+ let needsRestart = (ADDON_TYPE == "theme") && > >+ Services.prefs.prefHasUserValue(PREF_GENERAL_SKINS_SELECTEDSKIN); > What is the point of checking if the constant ADDON_TYPE which always equals > theme equals theme? I'm trying to lay the groundwork for the time soon when we may switch things so users can select any XPI theme and any persona to be applied together. When that happens I think almost all I have to do is change ADDON_TYPE to something different and it will just all work. Maybe I'll just end up cleaning up a lot of that when that happens anyway but I think it works for now.
Assignee | ||
Comment 126•14 years ago
|
||
Updated from the comments
Attachment #434399 -
Attachment is obsolete: true
Attachment #437727 -
Flags: review?(robert.bugzilla)
Comment 127•14 years ago
|
||
Comment on attachment 437625 [details] [diff] [review] xpiprovider rev 3 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+/** >+ * Gets the currently selected locale for display. >+ * @return the selected locale or "en-US" if none is selected >+ */ >+function getLocale() { >+ if (Prefs.getBoolPref(PREF_MATCH_OS_LOCALE), false) >+ return Services.locale.getLocaleComponentForUserAgent(); >+ return Prefs.getCharPref(PREF_SELECTED_LOCALE, "en-US"); >+} >+ >+/** >+ * Selects the closest matching locale from a list of locales. >+ * >+ * @param locales >+ * An array of locales >+ * @return the best match for the currently selected locale Just an FYI... now that you removed the s from @returns you can remove the extra space it required between @param, @return, etc. and the comment. >+ >+/** >+ * Calculates whether an add-on should be appDisabled or not. >+ * >+ * @param addon >+ * The add-on to check >+ * @return true if the add-on should not be appDisabled >+ */ >+function isUsableAddon(addon) { >+ // Hack to ensure the default theme is always usable >+ if (addon.type == "theme" && addon.internalName == XPIProvider.defaultSkin) >+ return true; >+ if (XPIProvider.checkCompatibility) { >+ if (!addon.isCompatible) >+ return false; >+ } >+ else { >+ if (!addon.matchingTargetApplication) >+ return false; >+ } >+ if (XPIProvider.checkUpdateSecurity && !addon.providesUpdatesSecurely) >+ return false; >+ return addon.blocklistState != Ci.nsIBlocklistService.STATE_BLOCKED; >+} >+function loadManifestFromRDF(uri, stream) { >... >+ addon.targetPlatforms = getPropertyArray(ds, root, "targetPlatform"); I wasn't able to find a followup bug for adding support for targetPlatform or it implemented anywhere... what's going on with it? >+/** >+ * Extracts files from a ZIP file into a directory. >+ * >+ * @param zipFile >+ * The source ZIP file that contains the add-on. >+ * @param dir >+ * The nsIFile to extract to. >+ */ >+function extractFiles(zipFile, dir) { >+ function getTargetFile(dir, entry) { >+ let target = dir.clone(); >+ entry.split("/").forEach(function(part) { >+ target.append(part); >+ }); >+ return target; >+ } >+ >+ let zipReader = Cc["@mozilla.org/libjar/zip-reader;1"]. >+ createInstance(Ci.nsIZipReader); >+ zipReader.open(zipFile); >+ >+ try { >+ // create directories first >+ let entries = zipReader.findEntries("*/"); >+ while (entries.hasMore()) { >+ var entryName = entries.getNext(); >+ let target = getTargetFile(dir, entryName); >+ if (!target.exists()) { >+ try { >+ target.create(Ci.nsILocalFile.DIRECTORY_TYPE, >+ FileUtils.PERMS_DIRECTORY); >+ } >+ catch (e) { >+ ERROR("extractFiles: failed to create target directory for " + >+ "extraction file = " + target.path + ", exception = " + e + >+ "\n"); >+ } >+ } >+ } >+ >+ entries = zipReader.findEntries(null); >+ while (entries.hasMore()) { >+ let entryName = entries.getNext(); >+ let target = getTargetFile(dir, entryName); >+ if (target.exists()) >+ continue; >+ >+ zipReader.extract(entryName, target); >+ target.permissions |= FileUtils.PERMS_FILE; >+ } >+ } >+ finally { >+ zipReader.close(); >+ } >+} I don't think it is all that likely with the changes to the install process but this could fail at some point and still install successfully which is a little weird. >+/** >+ * Verifies that a zip file's contents are all signed by the same principal. >+ * Directory entries and anything in the META-INF directory are not checked. >+ * >+ * @param zip >+ * A nsIZipReader to check >+ * @param principal >+ * The nsIPrincipal to compare against >+ * @return true if all the contents were signed by the principal, false >+ * otherwise. nit: all of the contents that should be signed were signed by the principal >+ */ >+function verifyZipSigning(zip, principal) { >+function escapeAddonURI(addon, uri, updateType, appVersion) >+{ >+ var addonStatus = addon.userDisabled ? "userDisabled" : "userEnabled"; >+ >+ if (!addon.isCompatible) >+ addonStatus += ",incompatible"; >+ if (addon.blocklistState > 0) >+ addonStatus += ",blocklisted"; >+ >+ try { >+ var xpcomABI = Services.appinfo.XPCOMABI; >+ } catch (ex) { >+ xpcomABI = UNKNOWN_XPCOM_ABI; >+ } >+ >+ uri = uri.replace(/%ITEM_ID%/g, addon.id); >+ uri = uri.replace(/%ITEM_VERSION%/g, addon.version); >+ uri = uri.replace(/%ITEM_STATUS%/g, addonStatus); >+ uri = uri.replace(/%APP_ID%/g, Services.appinfo.ID); >+ uri = uri.replace(/%APP_VERSION%/g, appVersion ? appVersion : >+ Services.appinfo.version); >+ uri = uri.replace(/%REQ_VERSION%/g, REQ_VERSION); >+ uri = uri.replace(/%APP_OS%/g, Services.appinfo.OS); >+ uri = uri.replace(/%APP_ABI%/g, xpcomABI); >+ uri = uri.replace(/%APP_LOCALE%/g, getLocale()); >+ uri = uri.replace(/%CURRENT_APP_VERSION%/g, Services.appinfo.version); >+ >+ // If there is an updateType then replace the UPDATE_TYPE string >+ if (updateType) >+ uri = uri.replace(/%UPDATE_TYPE%/g, updateType); This was present before... just wanted to note that %UPDATE_TYPE% will be left in the url when updateType is not defined / null / etc. >+ >+ // If this add-on has compatibility information for either the current >+ // application or toolkit then replace the ITEM_MAXAPPVERSION with the >+ // maxVersion >+ let app = addon.matchingTargetApplication; >+ if (app) >+ uri = uri.replace(/%ITEM_MAXAPPVERSION%/g, app.maxVersion); This was always replaced previously and now that it is conditional the url will have %ITEM_MAXAPPVERSION% in the string if it specifies it. Also, metrics needs to be informed about update url changes >+ // Replace custom parameters (names of custom parameters must have at >+ // least 3 characters to prevent lookups for something like %D0%C8) >+ var catMan = null; >+ uri = uri.replace(/%(\w{3,})%/g, function(match, param) { >+ if (!catMan) { >+ catMan = Cc["@mozilla.org/categorymanager;1"]. >+ getService(Ci.nsICategoryManager); >+ } >+ >+ try { >+ var contractID = catMan.getCategoryEntry(CATEGORY_UPDATE_PARAMS, param); >+ var paramHandler = Cc[contractID]. >+ getService(Ci.nsIPropertyBag2); nit: might as well make this a single line since it is well short of 80
Comment 128•14 years ago
|
||
(In reply to comment #127) >... > Also, metrics needs to be informed about update url changes Specifically, if a value becomes optional in this case
Updated•14 years ago
|
Whiteboard: [rewrite][needs-patch] → [rewrite][needs-review]
Assignee | ||
Comment 129•14 years ago
|
||
(In reply to comment #101) > (In reply to comment #86) > > I'd like to verify the new comments before r+'ing this. I'd really like to see > > a better name than amManager but since everything is still under > > @mozilla.org/extensions/ you should probably go with your original naming for > > the component of extManager. Next review should be an r+ > > I guess there is actually nothing stopping us moving to @mozilla.org/addons/. I > can't think of a great name though, this component is a jumble of the > amIWebInstaller, the bits needed to startup and shutdown AddonManager and the > listener for the background update timer, which leaves me thinking something > generic like amPlatformIntegration. Got any better ideas? Never really got an answer to this Rob?
Assignee | ||
Comment 130•14 years ago
|
||
Missed some of Rob's comments, updated.
Attachment #437630 -
Attachment is obsolete: true
Attachment #437912 -
Flags: review?(dveditz)
Attachment #437630 -
Flags: review?(dveditz)
Assignee | ||
Comment 131•14 years ago
|
||
Comments addressed
Attachment #434786 -
Attachment is obsolete: true
Attachment #437918 -
Flags: review+
Assignee | ||
Comment 132•14 years ago
|
||
(In reply to comment #114) > (From update of attachment 434787 [details] [diff] [review]) > interdiff to the rescue :) > > What about the following tests from xpinstall? > browser_opendialog.js This tested that installing worked when the add-ons manager window is open, since installs don't open the add-ons manager window anymore it is largely irrelevant now. > browser_chrome.js This tests that nsIXPInstallManager API bypasses the whitelist, but that API is gone now. > browser_cancel.js This was verifying that cancelling installs through the xpinstall APIs works. It doesn't really belong in the xpinstall tests anymore, though I probably should add an xpcshell tests for bug 553552 > >diff --git a/toolkit/mozapps/extensions/test/xpinstall/browser_signed_multiple.js b/toolkit/mozapps/extensions/test/xpinstall/browser_signed_multiple.js > >... > >+function confirm_install(window) { > >+ items = window.document.getElementById("itemList").childNodes; > >+ is(items.length, 2, "Should be 2 items listed in the confirmation dialog"); > >+ let item = get_item(items, TESTROOT + "signed.xpi"); > >+ if (item) { > >+ is(item.name, "Signed XPI Test", "Should have seen the name from the trigger list"); > >+ is(item.cert, "(Object Signer)", "Should have seen the signer"); > >+ is(item.signed, "true", "Should have listed the item as signed"); > >+ } > >+ item = get_item(items, TESTROOT + "signed2.xpi"); > >+ if (item) { > >+ is(item.name, "Signed XPI Test", "Should have seen the name from the trigger list"); > >+ is(item.cert, "(Object Signer)", "Should have seen the signer"); > >+ is(item.signed, "true", "Should have listed the item as signed"); > >+ } > Why did you remove the check for the url here and in other tests? get_item gets an item with a specific url so testing the url is redundant.
Comment 133•14 years ago
|
||
(In reply to comment #129) > (In reply to comment #101) > > (In reply to comment #86) > > > I'd like to verify the new comments before r+'ing this. I'd really like to see > > > a better name than amManager but since everything is still under > > > @mozilla.org/extensions/ you should probably go with your original naming for > > > the component of extManager. Next review should be an r+ > > > > I guess there is actually nothing stopping us moving to @mozilla.org/addons/. I > > can't think of a great name though, this component is a jumble of the > > amIWebInstaller, the bits needed to startup and shutdown AddonManager and the > > listener for the background update timer, which leaves me thinking something > > generic like amPlatformIntegration. Got any better ideas? > > Never really got an answer to this Rob? I haven't been able to come up with a decent name yet.
Comment 134•14 years ago
|
||
Comment on attachment 437625 [details] [diff] [review] xpiprovider rev 3 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ processPendingFileChanges: function XPI_processPendingFileChanges(manifests) { >+ // TODO maybe this should be passed off to the install locations to handle? >+ let changed = false; >+ this.installLocations.forEach(function(location) { >+ manifests[location.name] = {}; >+ // We can't install or uninstall anything in locked locations >+ if (location.locked) >+ return; >+ >+ let stagingDir = location.getStagingDir(); >+ if (!stagingDir || !stagingDir.exists()) >+ return; >+ >+ let entries = stagingDir.directoryEntries; >+ while (entries.hasMoreElements()) { >+ let stageDirEntry = entries.getNext().QueryInterface(Ci.nsILocalFile); >+ >+ // Only directories are important. Files may be updated manifests. >+ if (!stageDirEntry.isDirectory()) { >+ WARN("Ignoring file: " + stageDirEntry.path); >+ continue; >+ } >+ >+ // Check that the directory's name is a valid ID. >+ let id = stageDirEntry.leafName; >+ if (!gIDTest.test(id)) { >+ WARN("Ignoring directory whose name is not a valid add-on ID: " + >+ stageDirEntry.path); >+ continue; >+ } >+ >+ // Check if the directory contains an install manifest. >+ let manifest = stageDirEntry.clone(); >+ manifest.append(FILE_INSTALL_MANIFEST); >+ >+ // If the install manifest doesn't exist uninstall this add-on in this >+ // install location. >+ nit: extra newline >+ if (!manifest.exists()) { >+ LOG("Processing uninstall of " + id + " in " + location.name); >+ location.uninstallAddon(id); >+ // The file check later will spot the removal and cleanup the database >+ changed = true; >+ continue; >+ } >... >+ /** >+ * Called when a new add-on has been enabled when only one add-on of that type >+ * can be enabled. >+ * >+ * @param id >+ * The ID of the newly enabled add-on >+ * @param type >+ * The type of the newly enabled add-on >+ * @param pendingRestart >+ * true if the newly enabled add-on will only become enabled after a >+ * restart >+ */ >+ addonChanged: function XPI_addonChanged(id, type, pendingRestart) { Don't really like this name since it is only called when an add-on (specifically a theme for this provider) is enabled. Could this be added >+ // Look for the previously enabled theme and find the internalName of the >+ // currently selected theme >+ let previous = null; >+ let newSkin = this.defaultSkin; >+ let addons = XPIDatabase.getAddonsByType("theme"); >+ addons.forEach(function(a) { >+ if (!a.visible) >+ return; >+ if (a.id == id) >+ newSkin = a.internalName; >+ else if (a.userDisabled == false && !a.pendingUninstall) >+ previous = a; previousSkin? >+ }, this); >+ >+ if (pendingRestart) { >+ Services.prefs.setBoolPref(PREF_DSS_SWITCHPENDING, true); >+ Services.prefs.setCharPref(PREF_DSS_SKIN_TO_SELECT, newSkin); >+ } >+ else { >+ Services.prefs.setCharPref(PREF_GENERAL_SKINS_SELECTEDSKIN, newSkin); >+ } >+ this.selectedSkin = newSkin; >+ >+ // Mark the previous theme as disabled. This won't cause recursion since >+ // only enabled calls notifyAddonChanged. >+ if (previous) >+ this.updateAddonDisabledState(previous, true); >+ }, >+ >+ /** >+ * When the previously selected theme is removed this method should be called >+ * to enable the default theme. >+ */ nit: this method will be called >+ enableDefaultTheme: function XPI_enableDefaultTheme() { >+ LOG("Activating default theme"); >+ let addons = XPIDatabase.getAddonsByType("theme"); >+ addons.forEach(function(a) { >+ // If this is theme contains the default skin and it is currently visible >+ // then mark it as enabled. >+ if (a.internalName == this.defaultSkin && a.visible) >+ this.updateAddonDisabledState(a, false); >+ }, this); >+ }, >+ >+ /** >+ * Activates a bootstrapped add-on by loading its JS scope and calling the >+ * appropriate method on it. Adds the add-on and its scope to the >+ * bootstrapScopes and bootstrappedAddons dictionaries. >+ * >+ * @param id >+ * The ID of the add-on being activated >+ * @param dir >+ * The directory containing the add-on >+ * @param startup >+ * true if the add-on is being activated during startup >+ * @param install >+ * true if the add-on is being activated during installation >+ */ >+ activateAddon: function XPI_activateAddon(id, dir, startup, install) { >+ let methods = ["enable"]; >+ if (startup) >+ methods.unshift("startup"); >+ if (install) >+ methods.unshift("install"); >+ let bootstrap = dir.clone(); >+ bootstrap.append("bootstrap.js"); >+ if (bootstrap.exists()) { >+ let uri = Services.io.newFileURI(bootstrap); >+ let scope = new Components.utils.Sandbox("chrome://mozapps/content/extensions"); >+ let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"]. >+ createInstance(Ci.mozIJSSubScriptLoader); >+ >+ try { >+ loader.loadSubScript(uri.spec, scope); >+ } >+ catch (e) { >+ WARN("Error loading bootstrap.js for " + id + ": " + e); >+ return; >+ } >+ >+ this.callBootstrapMethod(id, scope, methods); >+ this.bootstrappedAddons[id] = dir.persistentDescriptor; >+ this.bootstrapScopes[id] = scope; >+ } >+ else { >+ WARN("Bootstrap missing for " + id); >+ } >+ }, >+ >+ /** >+ * Dectivates a bootstrapped add-on by by calling the appropriate method on >+ * the cached JS scope. Removes the add-on and its scope from the >+ * bootstrapScopes and bootstrappedAddons dictionaries. >+ * >+ * @param id >+ * The ID of the add-on being deactivated >+ * @param shutdown >+ * true if the add-on is being deactivated during shutdown >+ * @param uninstall >+ * true if the add-on is being deactivated during uninstallation >+ */ >+ deactivateAddon: function XPI_activateAddon(id, shutdown, uninstall) { s/XPI_activateAddon/XPI_deactivateAddon/ Can you go through the other patches looking for other copy / paste errors like this one?
Comment 135•14 years ago
|
||
(In reply to comment #134) >... > >+ addonChanged: function XPI_addonChanged(id, type, pendingRestart) { > Don't really like this name since it is only called when an add-on > (specifically a theme for this provider) is enabled. Could this be added I was wondering if this could be instead called internally instead of through the notification but I suspect not.
Comment 136•14 years ago
|
||
(In reply to comment #121) > Created an attachment (id=437625) [details] > xpinstall rev 3 > > Updated with comments. The only bit I haven't changed is the naming of > install/startDownload/startInstall. Do you have some suggestions for those? The > latter two seem to make sense to me as they can both be asynchronous operations > so they are just being started. The former is the public method that the API > exposes to make everything go. I'm fine with the explanation for the last two. install seems a tad weird because it does a few things... not sure if there is a better name so keep it as install >+ /** >+ * Starts installation of this add-on from whatever state it is currently at >+ * if possible. >+ * >+ * @throws if installation cannot proceed from the current state >+ */ >+ install: function AI_install() { >+ switch (this.state) { >+ case AddonManager.STATE_AVAILABLE: >+ this.startDownload(); >+ break; >+ case AddonManager.STATE_DOWNLOADED: >+ this.startInstall(); >+ break; >+ case AddonManager.STATE_DOWNLOADING: >+ case AddonManager.STATE_CHECKING: >+ case AddonManager.STATE_INSTALLING: >+ // Installation is already running >+ return; >+ default: >+ throw new Error("Cannot start installing from this state"); >+ } >+ },
Comment 137•14 years ago
|
||
Comment on attachment 437625 [details] [diff] [review] xpiprovider rev 3 >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm >... >+ /** >+ * Updates the disabled state for an add-on. Its appDisabled property will be >+ * calculated and if the add-on is changed appropriate notifications will be >+ * sent out to the registered AddonListeners. >+ * >+ * @param addon >+ * The DBAddonInternal to update >+ * @param userDisabled >+ * Value for the userDisabled property. If undefined the value will >+ * not change >+ * @throws if addon is not a DBAddonInternal >+ */ >+ updateAddonDisabledState: function XPI_updateAddonDisabledState(addon, >+ userDisabled) { >+ if (!(addon instanceof DBAddonInternal)) >+ throw new Error("Can only update addon states for installed addons."); >+ >+ if (userDisabled === undefined) >+ userDisabled = addon.userDisabled; >+ >+ let appDisabled = !isUsableAddon(addon); >+ // No change means nothing to do here >+ if (addon.userDisabled == userDisabled && >+ addon.appDisabled == appDisabled) >+ return; >+ >+ let wasDisabled = addon.userDisabled || addon.appDisabled; >+ let isDisabled = userDisabled || appDisabled; >+ >+ // Update the properties in the database >+ XPIDatabase.setAddonProperties(addon, { >+ userDisabled: userDisabled, >+ appDisabled: appDisabled >+ }); >+ >+ // If the add-on is not visible or the add-on is not changing state then >+ // there is no need to do anything else >+ if (!addon.visible || (wasDisabled == isDisabled)) >+ return; >+ >+ // Flag that active states in the database need to be updated on shutdown >+ Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, true); >+ >+ let wrapper = createWrapper(addon); >+ // Have we just gone back to the current state? >+ if (isDisabled != addon.active) { >+ AddonManagerPrivate.callAddonListeners("onOperationCancelled", wrapper); >+ } >+ else { >+ let needsRestart = true; nit: no reason to set this to true since it will be set by either disableRequiresRestart or enableRequiresRestart. >+ if (isDisabled) { >+ needsRestart = this.disableRequiresRestart(addon); >+ AddonManagerPrivate.callAddonListeners("onDisabling", wrapper, >+ needsRestart); >+ } >+ else { >+ needsRestart = this.enableRequiresRestart(addon); >+ AddonManagerPrivate.callAddonListeners("onEnabling", wrapper, >+ needsRestart); >+ } >... >+AddonInstall.prototype = { >... >+ /** >+ * Cancels installation of this add-on. >+ * >+ * @throws if installation cannot be cancelled from the current state >+ */ >+ cancel: function AI_cancel() { >+ switch (this.state) { >+ case AddonManager.STATE_DOWNLOADING: >+ break; iirc there is a bug to add the ability to cancel the install when it is downloading. Just making sure. r=me with comments addressed. I'd really like the naming of addonChanged sorted but I can't think of a better one off the top of my head so I'll leave it up to you whether to change it or not
Attachment #437625 -
Flags: review?(robert.bugzilla) → review+
Updated•14 years ago
|
Attachment #437727 -
Flags: review?(robert.bugzilla) → review+
Assignee | ||
Comment 138•14 years ago
|
||
(In reply to comment #127) > >+function loadManifestFromRDF(uri, stream) { > >... > >+ addon.targetPlatforms = getPropertyArray(ds, root, "targetPlatform"); > I wasn't able to find a followup bug for adding support for targetPlatform or > it implemented anywhere... what's going on with it? Good catch, filed bug 558834 > > >+/** > >+ * Extracts files from a ZIP file into a directory. > >+ * > >+ * @param zipFile > >+ * The source ZIP file that contains the add-on. > >+ * @param dir > >+ * The nsIFile to extract to. > >+ */ > >+function extractFiles(zipFile, dir) { > >+ function getTargetFile(dir, entry) { > >+ let target = dir.clone(); > >+ entry.split("/").forEach(function(part) { > >+ target.append(part); > >+ }); > >+ return target; > >+ } > >+ > >+ let zipReader = Cc["@mozilla.org/libjar/zip-reader;1"]. > >+ createInstance(Ci.nsIZipReader); > >+ zipReader.open(zipFile); > >+ > >+ try { > >+ // create directories first > >+ let entries = zipReader.findEntries("*/"); > >+ while (entries.hasMore()) { > >+ var entryName = entries.getNext(); > >+ let target = getTargetFile(dir, entryName); > >+ if (!target.exists()) { > >+ try { > >+ target.create(Ci.nsILocalFile.DIRECTORY_TYPE, > >+ FileUtils.PERMS_DIRECTORY); > >+ } > >+ catch (e) { > >+ ERROR("extractFiles: failed to create target directory for " + > >+ "extraction file = " + target.path + ", exception = " + e + > >+ "\n"); > >+ } > >+ } > >+ } > >+ > >+ entries = zipReader.findEntries(null); > >+ while (entries.hasMore()) { > >+ let entryName = entries.getNext(); > >+ let target = getTargetFile(dir, entryName); > >+ if (target.exists()) > >+ continue; > >+ > >+ zipReader.extract(entryName, target); > >+ target.permissions |= FileUtils.PERMS_FILE; > >+ } > >+ } > >+ finally { > >+ zipReader.close(); > >+ } > >+} > I don't think it is all that likely with the changes to the install process but > this could fail at some point and still install successfully which is a little > weird. I think the install process warrants going over in a bit more detail to make sure we're catching all the potential problems and add the rolling back stuff. I have bug 553015 on file to do that afterwards. > >+ // If there is an updateType then replace the UPDATE_TYPE string > >+ if (updateType) > >+ uri = uri.replace(/%UPDATE_TYPE%/g, updateType); > This was present before... just wanted to note that %UPDATE_TYPE% will be left > in the url when updateType is not defined / null / etc. Yeah and I think this is correct, the only time there is no updateType is when escaping a URL for an add-ons release notes. > >+ > >+ // If this add-on has compatibility information for either the current > >+ // application or toolkit then replace the ITEM_MAXAPPVERSION with the > >+ // maxVersion > >+ let app = addon.matchingTargetApplication; > >+ if (app) > >+ uri = uri.replace(/%ITEM_MAXAPPVERSION%/g, app.maxVersion); > This was always replaced previously and now that it is conditional the url will > have %ITEM_MAXAPPVERSION% in the string if it specifies it. Corrected that to replace it with "" when there is no target app which is what the old code did. With that I think there shouldn't have been any changes to the update URL.
Assignee | ||
Comment 139•14 years ago
|
||
Added comments to the IDL as requested
Attachment #434396 -
Attachment is obsolete: true
Attachment #440553 -
Flags: review?(benjamin)
Attachment #434396 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #440553 -
Flags: review?(benjamin) → review+
Comment 140•14 years ago
|
||
Comment on attachment 440553 [details] [diff] [review] toolkit rev 2 Please make nsAppStartup::Get/SetNeedsRestart throw if you try to use them after startup.
Assignee | ||
Comment 141•14 years ago
|
||
Rob, I just realised I never passed this past you for the last time. This has the comments fixed and changes amManager.js to addonManager.js as you suggested in email.
Attachment #434397 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #442204 -
Flags: review+
Comment 142•14 years ago
|
||
Comment on attachment 437912 [details] [diff] [review] xpinstall rev 4 >-pref("xpinstall.dialog.confirm", "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul"); >-pref("xpinstall.dialog.progress.skin", "chrome://mozapps/content/extensions/extensions.xul"); >-pref("xpinstall.dialog.progress.chrome", "chrome://mozapps/content/extensions/extensions.xul"); These were originally to allow different apps to provide their own UI. Will the new rewrite be usable by other toolkit apps that might not have tabbed browser windows? >+++ b/toolkit/mozapps/extensions/amIInstallTrigger.idl >+ * The Original Code is the Extension Manager. ObNit: the "am" prefix on filenames seem to imply the code is "Add-on Manager" rather than "Extension Manager". >+ /** >+ * Tests if installation is enabled. This method is deprecated, please use >+ * "enabled" in the future. >+ */ >+ boolean updateEnabled(); Can we just drop it? Very sad, this has been deprecated since before Firefox 1.0 -- but I see it's still more popular than enabled() as a recommendation on the web. >+ boolean installChrome(in PRUint32 type, in AString url, in AString skin); >+ boolean startSoftwareUpdate(in AString url, [optional] in PRInt32 flags); Can we drop these, too? I guess that's out of scope for this bug. >+ // TODO show some better error >+ Services.prompt.alert(this.window, "Download Failed", "The download of " + install.sourceURL + " failed: " + error); Is there a bug filed on this TODO? Otherwise it could easily slip through the cracks. Obviously needs to be localized at the very least. > XPInstallConfirm.onOK = function () > { >- this._param.SetInt(0, 0); >+ args.installs.forEach(function(install) { >+ install.install(); >+ }); Does this mean everything happens synchronously on the UI thread? Originally we were concerned that unpacking zips and moving files around could make the browser unresponsive for too long if we did that. r=dveditz
Attachment #437912 -
Flags: review?(dveditz) → review+
Assignee | ||
Comment 143•14 years ago
|
||
(In reply to comment #142) > (From update of attachment 437912 [details] [diff] [review]) > >-pref("xpinstall.dialog.confirm", "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul"); > >-pref("xpinstall.dialog.progress.skin", "chrome://mozapps/content/extensions/extensions.xul"); > >-pref("xpinstall.dialog.progress.chrome", "chrome://mozapps/content/extensions/extensions.xul"); > > These were originally to allow different apps to provide their own UI. Will the > new rewrite be usable by other toolkit apps that might not have tabbed browser > windows? Yes, but rather than overriding a UI url they can override an XPCOM component to do what they like, including not showing any UI at all if that is what they want. > >+ /** > >+ * Tests if installation is enabled. This method is deprecated, please use > >+ * "enabled" in the future. > >+ */ > >+ boolean updateEnabled(); > > Can we just drop it? Very sad, this has been deprecated since before Firefox > 1.0 -- but I see it's still more popular than enabled() as a recommendation on > the web. Going to leave this in for now for expediency, perhaps we should make this method log an error to the console and publish that it is going away then remove it at a later time. > >+ boolean installChrome(in PRUint32 type, in AString url, in AString skin); > >+ boolean startSoftwareUpdate(in AString url, [optional] in PRInt32 flags); > > Can we drop these, too? I guess that's out of scope for this bug. Again same as above I think. > > >+ // TODO show some better error > >+ Services.prompt.alert(this.window, "Download Failed", "The download of " + install.sourceURL + " failed: " + error); > > Is there a bug filed on this TODO? Otherwise it could easily slip through the > cracks. Obviously needs to be localized at the very least. Filed as bug 552965, I'll add that to the comment though. > Does this mean everything happens synchronously on the UI thread? Originally we > were concerned that unpacking zips and moving files around could make the > browser unresponsive for too long if we did that. For now yes, though that is the same as what happens on trunk now anyway (unless there were multiple installs in the InstallTrigger). But that can be handled better by making the xpi extraction in the extension manager code asynchronous, filed bug 562674 to do that.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [rewrite][needs-review] → [rewrite][needs-landing]
Updated•14 years ago
|
Blocks: SMAddonMgr
Comment 144•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f746d2eca199 Implement the basic extension manager API and platform integration. + Backed out in bug 562679 comment 7.
Comment 145•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d2a2b786fcc9 Implement InstallTrigger, the XPI content handler and confirmation for web triggered installs. + Backed out in bug 562679 comment 7.
Assignee | ||
Comment 146•14 years ago
|
||
(In reply to comment #145) > http://hg.mozilla.org/mozilla-central/rev/d2a2b786fcc9 > Implement InstallTrigger, the XPI content handler and confirmation for web > triggered installs. > + > Backed out in bug 562679 comment 7. Please do not do this for every bug involved in that landing/backout. I specifically didn't mark them to avoid the bugspam.
Assignee | ||
Comment 147•14 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/a95ae7a5a591 (updatechecker) http://hg.mozilla.org/mozilla-central/rev/1cbc8bb078ab (toolkit) http://hg.mozilla.org/mozilla-central/rev/efb43e07fb76 (base) http://hg.mozilla.org/mozilla-central/rev/a9ba8d3ee1d9 (xpiprovider) http://hg.mozilla.org/mozilla-central/rev/9d7f76acff02 (lwtheme) http://hg.mozilla.org/mozilla-central/rev/704046d2ddea (xpinstall) http://hg.mozilla.org/mozilla-central/rev/bf6b3103e6a9 (browsertests) http://hg.mozilla.org/mozilla-central/rev/307c1ea0576a (xpcshelltests) http://hg.mozilla.org/mozilla-central/rev/31718148a888 (xpinstalltests) http://hg.mozilla.org/mozilla-central/rev/e28abfd58111 (file removals)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [rewrite][needs-landing] → [rewrite]
Assignee | ||
Updated•14 years ago
|
Target Milestone: --- → mozilla1.9.3a5
You need to log in
before you can comment on or make changes to this bug.
Description
•