Closed
Bug 562495
Opened 15 years ago
Closed 15 years ago
Support the new add-ons manager
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: mossop, Assigned: mfinkle)
References
Details
(Whiteboard: [AddonsRewriteTestday])
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
Landing the new add-ons manager on trunk will break Fennec builds currently, going to elaborate on exactly how broken you'll be once my test builds are complete.
Comment 1•15 years ago
|
||
One issue already found is that Clicking "Add to Firefox" on AMO doesn't perform an action on 2010-04-30 fennec trunk nightly build
Steps to Reproduce:
1. Go to addons.mozilla.org
2. Click on "Add to Firefox"
Comment 2•15 years ago
|
||
Another issue is that nothing shows up in our Addons Manager:
1. Go to the controls window
2. Click on the Addons button
Actual Results:
The default search engines are not listed there nor are any recommended addons.
Updated•15 years ago
|
Whiteboard: [AddonsRewriteTestday]
Reporter | ||
Comment 3•15 years ago
|
||
This is an initial WIP patch that I threw together, it should have the main UI work, only stuff missing is making installs work, you'll likely best do that by overriding the webinstalllistener component and making it display the UI you like there.
Reporter | ||
Comment 4•15 years ago
|
||
If you set the preference "extensions.enabledScopes" to 5 then the add-ons manager will not look in global install locations for extensions, just in the application and profile locations.
Assignee | ||
Comment 5•15 years ago
|
||
This patch builds on the first patch. New stuff includes:
* Displays installed addons and search plugins
* Handles add-on manager based installs
* Handles web based installs (uses "addon-install-blocked" and related changes)
* Handles updates
Left to do are:
* Remove the ugly dialog during web based installs and use our own
* Some cleanup of the richlistitems
Assignee: nobody → mark.finkle
Attachment #445048 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
Added support for web based installs using the Fennec UI
Attachment #445293 -
Attachment is obsolete: true
Assignee | ||
Comment 7•15 years ago
|
||
Updated for changes to bug 566020, which this bug now depends on.
Attachment #445435 -
Attachment is obsolete: true
Attachment #445498 -
Flags: review?(dtownsend)
Assignee | ||
Comment 8•15 years ago
|
||
Tests are coming up too
Reporter | ||
Comment 9•15 years ago
|
||
Comment on attachment 445498 [details] [diff] [review]
patch
First set of comments, will look over this more soon.
>diff --git a/app/mobile.js b/app/mobile.js
> /* extension manager and xpinstall */
>-pref("xpinstall.dialog.confirm", "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul");
>-pref("xpinstall.dialog.progress.skin", "chrome://browser/content/browser.xul");
>-pref("xpinstall.dialog.progress.chrome", "chrome://browser/content/browser.xul");
>-pref("xpinstall.dialog.progress.type.skin", "navigator:browser");
>-pref("xpinstall.dialog.progress.type.chrome", "navigator:browser");
> pref("xpinstall.whitelist.add", "addons.mozilla.org");
>
> pref("extensions.autoupdate.enabled", true);
> pref("extensions.autoupdate.interval", 86400);
The new manager is actually going to do auto-update for you, though it might be worth waiting a short time while we iron ut the specific behavior of that before you remove all your stuff.
>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
> buttons = [{
> label: strings.getString("xpinstallPromptAllowButton"),
> accessKey: null,
> popup: null,
> callback: function() {
> // Kick off the xpinstall
"Kick off the install" would be more appropriate now.
>diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js
> let self = this;
>- InstallTrigger.install(params, function(aURL, aStatus) { self._installCallback(aItem, aStatus); });
>+ AddonManager.getInstallForURL(aItem.getAttribute("xpiURL"),
>+ function(aInstall) { self._installCallback(aItem, aInstall); },
>+ "application/x-xpinstall",
>+ aItem.getAttribute("xpiHash"));
Intending is off here. Might as well just call aInstall.install() directly here rather than using _installCallback right?
>+ onInstallEnded: function(aInstall, aAddon) {
>+ // XXX fix updating stuff
>+ ExtensionsView.showRestart(this._updating ? "update" : "normal");
You should check if aAddon.pendingOperations has PENDING_INSTALL since we now support add-ons that can install without restarts.
>- if (!ExtensionsView.visible)
>- return;
>+ if (ExtensionsView.visible) {
Probably better to continue bailing out early when possible here.
>+ onInstallFailed: function(aInstall, aError) {
>+ this._showAlert(false);
>+
>+ if (ExtensionsView.visible) {
>+ let element = ExtensionsView.getElementForAddon(aInstall.sourceURL);
>+ if (!element)
>+ return;
>+
>+ element.removeAttribute("opType");
>+ let bundles = Cc["@mozilla.org/intl/stringbundle;1"].getService(Ci.nsIStringBundleService);
>+ let strings = bundles.createBundle("chrome://global/locale/xpinstall/xpinstall.properties");
>+
>+ try {
>+ var msg = strings.GetStringFromName("error" + aStatus);
>+ } catch (ex) {
>+ msg = strings.formatStringFromName("unknown.error", [aStatus]);
>+ }
This seems broken, where is aStatus coming from?
Assignee | ||
Comment 10•15 years ago
|
||
Updated patch per review comments
Attachment #445498 -
Attachment is obsolete: true
Attachment #445911 -
Flags: review?(dtownsend)
Attachment #445498 -
Flags: review?(dtownsend)
Assignee | ||
Comment 11•15 years ago
|
||
(In reply to comment #9)
> > popup: null,
> > callback: function() {
> > // Kick off the xpinstall
>
> "Kick off the install" would be more appropriate now.
Fixed
> >+ AddonManager.getInstallForURL(aItem.getAttribute("xpiURL"),
> >+ function(aInstall) { self._installCallback(aItem, aInstall); },
> >+ "application/x-xpinstall",
> >+ aItem.getAttribute("xpiHash"));
>
> Intending is off here. Might as well just call aInstall.install() directly here
> rather than using _installCallback right?
Fixed indenting and refactored _installCallback away.
> >+ onInstallEnded: function(aInstall, aAddon) {
> >+ // XXX fix updating stuff
> >+ ExtensionsView.showRestart(this._updating ? "update" : "normal");
>
> You should check if aAddon.pendingOperations has PENDING_INSTALL since we now
> support add-ons that can install without restarts.
Done
> >- if (!ExtensionsView.visible)
> >- return;
> >+ if (ExtensionsView.visible) {
>
> Probably better to continue bailing out early when possible here.
Done
> >+ onInstallFailed: function(aInstall, aError) {
>
> This seems broken, where is aStatus coming from?
Reworked the code to use a simple mapping of AddonManager errors to xpinstall errors. Also called onInstallError from onDownloadError so the code was shared.
Comment 12•15 years ago
|
||
This is just a sidenote as it would seem to be unrelated.
Current m-c/fennec doesn't list downloads in download manager anymore.
Applying this patch fixes it for me.
Updated•15 years ago
|
Severity: normal → major
tracking-fennec: --- → ?
Flags: in-testsuite?
Flags: in-litmus?
Assignee | ||
Comment 13•15 years ago
|
||
Removing blocking-? flag for now. THis only affects mobile-browser (trunk) and not mobile-1.1 (Fennec 1.1 release).
tracking-fennec: ? → ---
Reporter | ||
Comment 14•15 years ago
|
||
Comment on attachment 445911 [details] [diff] [review]
patch 2
Just one issue that needs to be changed a bit, should be a quick r+ next time around.
> var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>- os.addObserver(gXPInstallObserver, "xpinstall-install-blocked", false);
>+ os.addObserver(gXPInstallObserver, "addon-install-blocked", false);
Note: You may want to start using Services.jsm here and elsewhere. Doesn't need to happen for this patch though.
>+ let updateCheckListener = {
>+ _getJSON: function(aAddon, aInstall) {
>+ data = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+ if (aInstall)
>+ data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name, version: aInstall.version });
>+ else
>+ data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name });
>+ return data;
>+ },
>+ onCompatibilityUpdateAvailable: function(aAddon) {
>+ gObs.notifyObservers(this._getJSON(aAddon), "addon-update-ended", "compatibility");
>+ },
>+ onUpdateAvailable: function(aAddon, aInstall) {
>+ gObs.notifyObservers(this._getJSON(aAddon, aInstall), "addon-update-ended", "success");
>+ aInstall.install();
>+ },
>+ onNoUpdateAvailable: function(aAddon, aError) {
>+ gObs.notifyObservers(this._getJSON(aAddon), "addon-update-ended", aError ? "error" : "no-update");
>+ },
>+ onUpdateFinished: function(aAddon) {
>+ }
>+ };
I'm not sure if this is quite right. You'll end up dispatching addon-update-ended twice in the case where the update check finds a compatibility update, once for the compatibility update itself and again with either success or no-update depending on whether a new version is also available. This will obscure your status message about compatibility updates.
You probably want to keep creating a new instance of UpdateCheckListener for each check, and remembering the status as a property of it, dispatching addon-update-ended in onUpdateFinished.
Attachment #445911 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 15•15 years ago
|
||
* Using Services.jsm in Fennec is filed, I think
* I switched the update listener as you suggested. Creating a new instance for each call and only firing the "addon-updated-ended" notification once.
Attachment #445911 -
Attachment is obsolete: true
Attachment #446282 -
Flags: review?(dtownsend)
Assignee | ||
Comment 16•15 years ago
|
||
Oops. Forgot to qref the patch
Attachment #446282 -
Attachment is obsolete: true
Attachment #446284 -
Flags: review?(dtownsend)
Attachment #446282 -
Flags: review?(dtownsend)
Reporter | ||
Comment 17•15 years ago
|
||
Comment on attachment 446284 [details] [diff] [review]
patch 4
Still not quite right I think.
> function UpdateCheckListener() {
>- this._addons = [];
>+ this._status = null;
>+ this._version = null;
> }
>
> UpdateCheckListener.prototype = {
>- /////////////////////////////////////////////////////////////////////////////
>- // nsIAddonUpdateCheckListener
>- onUpdateStarted: function ucl_onUpdateStarted() {
>+ onCompatibilityUpdateAvailable: function(aAddon) {
>+ this._status = "compatibility";
> },
>
>- onUpdateEnded: function ucl_onUpdateEnded() {
>- if (!this._addons.length)
>- return;
>-
>- // If we have some updateable add-ons, let's download them
>- let items = [];
>- for (let i = 0; i < this._addons.length; i++)
>- items.push(gExtMgr.getItemForID(this._addons[i]));
>-
>- // Start the actual downloads
>- gExtMgr.addDownloads(items, items.length, null);
>-
>- // Remember that we downloaded new updates so we don't check again
>- gNeedsRestart = true;
>+ onUpdateAvailable: function(aAddon, aInstall) {
>+ this._status = "update";
>+ this._version = aInstall.version;
>+ aInstall.install();
> },
>
>- onAddonUpdateStarted: function ucl_onAddonUpdateStarted(aAddon) {
>- gObs.notifyObservers(aAddon, "addon-update-started", null);
>+ onNoUpdateAvailable: function(aAddon, aError) {
>+ this._status = aError ? "error" : "no-update";
> },
The docs are correct and the source is wrong here, we'll move the error parameter onto onUpdateFinished, I guess you can either leave this or change this, now, don't think it'd be a problem to change before the platform does.
I think you want to set this._status only if this._status is null, so that you still pass through the compatibility state.
Attachment #446284 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 18•15 years ago
|
||
* Don't overwrite a compatibility status
* Move aError to onUpdateEnded
Attachment #446284 -
Attachment is obsolete: true
Attachment #446300 -
Flags: review?(dtownsend)
Reporter | ||
Updated•15 years ago
|
Attachment #446300 -
Attachment is patch: true
Attachment #446300 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Updated•15 years ago
|
Attachment #446300 -
Attachment description: patch (m-192) → patch 5
Reporter | ||
Updated•15 years ago
|
Attachment #446300 -
Flags: review?(dtownsend) → review+
Reporter | ||
Comment 19•15 years ago
|
||
Comment on attachment 446300 [details] [diff] [review]
patch 5
Assuming that the status that you want to pass through in order of importance is "error" > "update" > "compatibility" > "no-update" then you want to tweak this as follows:
> function UpdateCheckListener() {
>- this._addons = [];
>+ this._status = null;
>+ this._version = null;
> }
>
> UpdateCheckListener.prototype = {
>- /////////////////////////////////////////////////////////////////////////////
>- // nsIAddonUpdateCheckListener
>- onUpdateStarted: function ucl_onUpdateStarted() {
>+ onCompatibilityUpdateAvailable: function(aAddon) {
>+ this._status = "compatibility";
> },
>
>- onUpdateEnded: function ucl_onUpdateEnded() {
>- if (!this._addons.length)
>- return;
>-
>- // If we have some updateable add-ons, let's download them
>- let items = [];
>- for (let i = 0; i < this._addons.length; i++)
>- items.push(gExtMgr.getItemForID(this._addons[i]));
>-
>- // Start the actual downloads
>- gExtMgr.addDownloads(items, items.length, null);
>-
>- // Remember that we downloaded new updates so we don't check again
>- gNeedsRestart = true;
>+ onUpdateAvailable: function(aAddon, aInstall) {
>+ // Don't overwrite a compatibility status
>+ if (!this._status)
>+ this._status = "update";
Always set this._status to "update" here overwriting whatever it is currently.
>+ this._version = aInstall.version;
>+ aInstall.install();
> },
>
>- onAddonUpdateStarted: function ucl_onAddonUpdateStarted(aAddon) {
>- gObs.notifyObservers(aAddon, "addon-update-started", null);
>+ onNoUpdateAvailable: function(aAddon) {
>+ this._status = "no-update";
Only set this._status if it is null.
> },
>
>- onAddonUpdateEnded: function ucl_onAddonUpdateEnded(aAddon, aStatus) {
>- gObs.notifyObservers(aAddon, "addon-update-ended", aStatus);
>-
>- // Save the add-on id if we can download an update
>- if (aStatus == Ci.nsIAddonUpdateCheckListener.STATUS_UPDATE)
>- this._addons.push(aAddon.id);
>- },
>+ onUpdateFinished: function(aAddon, aError) {
>+ let data = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+ if (this._version)
>+ data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name, version: this._version });
>+ else
>+ data.data = JSON.stringify({ id: aAddon.id, name: aAddon.name });
>
>- QueryInterface: function ucl_QueryInterface(aIID) {
>- if (!aIID.equals(Ci.nsIAddonUpdateCheckListener) &&
>- !aIID.equals(Ci.nsISupports))
>- throw Components.results.NS_ERROR_NO_INTERFACE;
>- return this;
>+ if (aError)
>+ this._status = "error";
>+
>+ gObs.notifyObservers(data, "addon-update-ended", this._status);
> }
> };
Assignee | ||
Comment 20•15 years ago
|
||
(In reply to comment #19)
> (From update of attachment 446300 [details] [diff] [review])
> Assuming that the status that you want to pass through in order of importance
> is "error" > "update" > "compatibility" > "no-update" then you want to tweak
> this as follows:
OK, that makes sense - and matches with what I see in XPIProvider.jsm (I shoulda looked there first)
Assignee | ||
Comment 21•15 years ago
|
||
pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/b3d6802584bc
need to add some tests for this
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 22•14 years ago
|
||
Aakash, can you please test if it works as expected for Fennec and verify this bug? Thanks.
Comment 23•14 years ago
|
||
verified FIXED On build:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/2010722 Namoroka/4.0b2pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Flags: in-litmus? → in-litmus+
Comment 24•14 years ago
|
||
Aakash, can you please mention the litmus testcase id? That makes it easier to finding the test later on.
Comment 25•14 years ago
|
||
There was nothing UI facing changed here. So, its the BFT's and FFT's for the Addons Manager within the Fennec 2.0 catch all test run. I'll mark it as minus, instead.
Flags: in-litmus+ → in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•