Support targetPlatforms from install manifests

VERIFIED FIXED in mozilla2.0b2

Status

()

Toolkit
Add-ons Manager
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({APIchange})

Trunk
mozilla2.0b2
APIchange
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta2+)

Details

(Whiteboard: [rewrite][schemachange])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
We currently read the information but then don't put it into the database or make compatibility judgements based on it.
(Assignee)

Updated

8 years ago
blocking2.0: --- → beta1+
(Assignee)

Updated

8 years ago
Assignee: dtownsend → nobody
(Assignee)

Updated

8 years ago
Duplicate of this bug: 569141
Flags: in-testsuite?
Flags: in-litmus?
(Assignee)

Updated

8 years ago
blocking2.0: beta1+ → final+
(Assignee)

Updated

8 years ago
blocking2.0: final+ → beta2+
Whiteboard: [rewrite] → [rewrite][apichange][schemachange]
(Assignee)

Updated

8 years ago
Blocks: 553492
(Assignee)

Updated

8 years ago
Blocks: 574970
(Assignee)

Comment 2

8 years ago
Created attachment 454619 [details] [diff] [review]
DB patch rev 1

This adds DB support for the targetPlatform and skinnable properties from install.rdf and also adds a size property to the DB that is calculated at install time. I want to do all these in one go so it is only one schema update. I've also fixed a problem where switching between different DB schemas will throw away any updated compatibility information from updates. This means that right now downgrading from schema 2 to a version running schema 1 will lose that info, upgrading will retain it which I think is fine.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #454619 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 3

8 years ago
Created attachment 454621 [details] [diff] [review]
targetPlatform support rev 1

This adds a new property to addons, isPlatformCompatible that is based on the data in targetPlatforms and uses it to app enable/disable add-ons as appropriate.
Attachment #454621 - Flags: review?(robert.bugzilla)
Keywords: APIchange
Whiteboard: [rewrite][apichange][schemachange] → [rewrite][schemachange]
(Assignee)

Comment 4

8 years ago
Comment on attachment 454619 [details] [diff] [review]
DB patch rev 1

Actually cancel that, one other property we're going to add while we're here.
Attachment #454619 - Attachment is obsolete: true
Attachment #454619 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 5

8 years ago
Created attachment 454709 [details] [diff] [review]
DB patch rev 2

This is the previously mentioned DB patch with an added property sourceURI for addons that holds the URI that the XPI file came from if known. I'm going to add tests for this in bug 575463 which will also rename AddonInstall.sourceURL to AddonInstall.sourceURI for consistency with Addon.getResourceURI.
Attachment #454709 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 6

8 years ago
Comment on attachment 454709 [details] [diff] [review]
DB patch rev 2

Gah, wrong patch
Attachment #454709 - Attachment is obsolete: true
Attachment #454709 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 7

8 years ago
Created attachment 454710 [details] [diff] [review]
DB patch rev 2
Attachment #454710 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 8

8 years ago
*sigh* and please ignore the hunk in AddonManager.jsm, it slipped in while debugging and I've removed it locally.
(Assignee)

Comment 9

8 years ago
Comment on attachment 454710 [details] [diff] [review]
DB patch rev 2

Ok cancel this again, Blair has pointed out that we want some more info in the DB, will upload a new patch tomorrow.
Attachment #454710 - Attachment is obsolete: true
Attachment #454710 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

8 years ago
Blocks: 576089
(Assignee)

Comment 10

8 years ago
Created attachment 455264 [details] [diff] [review]
DB patch rev 3

Hopefully the last set of DB changes. This now also includes an infoURI property for add-ons which will hold the release notes for installed versions (i.e. when we detect a new version and see its updateInfoURL then download and install we keep the info URL to be viewed with the new version at a later time). Feeding that property in is covered in bug 576089
Attachment #455264 - Flags: review?(robert.bugzilla)
Comment on attachment 455264 [details] [diff] [review]
DB patch rev 3

>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
>...
>@@ -450,63 +463,125 @@ function loadManifestFromRDF(aUri, aStre
>       WARN("Ignoring duplicate targetApplication entry for " + targetAppInfo.id +
>            " in install manifest");
>       continue;
>     }
>     seenApplications.push(targetAppInfo.id);
>     addon.targetApplications.push(targetAppInfo);
>   }
> 
>-  addon.targetPlatforms = getPropertyArray(ds, root, "targetPlatform");
>+  // Note that we don't need to check for duplicate targetPlatform entries since
>+  // the RDF service coalesces them for us.
>+  let targetPlatforms = getPropertyArray(ds, root, "targetPlatform");
>+  addon.targetPlatforms = [];
>+  targetPlatforms.forEach(function(aPlatform) {
>+    let platform = {
>+      os: null,
>+      abi: null
>+    };
>+
>+    let pos = aPlatform.indexOf("_");
>+    if (pos >= 0) {
nit: != -1 or flip flop the code below and use == -1

>+      platform.os = aPlatform.substring(0, pos);
>+      platform.abi = aPlatform.substring(pos + 1);
>+    }
>+    else {
>+      platform.os = aPlatform;
>+    }
>+
>+    addon.targetPlatforms.push(platform);
>+  });
>...
> /**
>  * Loads an AddonInternal object from an add-on extracted in a directory.
>  *
>  * @param  aDir
>  *         The nsIFile directory holding the add-on
>  * @return an AddonInternal object
>  * @throws if the directory does not contain a valid install manifest
>  */
> function loadManifestFromDir(aDir) {
>+  function getFileSize(aFile) {
>+    if (aFile.isSymlink())
>+      return 0;
>+
>+    if (!aFile.isDirectory())
>+      return aFile.fileSize;
>+
>+    let size = 0;
>+    let entries = aFile.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
>+    let entry;
>+    while (entry = entries.nextFile)
>+      size += getFileSize(entry);
>+    entries.close();
>+    return size;
>+  }
>+
>   let file = aDir.clone();
>   file.append(FILE_INSTALL_MANIFEST);
>   if (!file.exists() || !file.isFile())
>     throw new Error("Directory " + dir.path + " does not contain a valid " +
>                     "install manifest");
> 
>   let fis = Cc["@mozilla.org/network/file-input-stream;1"].
>             createInstance(Ci.nsIFileInputStream);
>   fis.init(file, -1, -1, false);
>   let bis = Cc["@mozilla.org/network/buffered-input-stream;1"].
>             createInstance(Ci.nsIBufferedInputStream);
>   bis.init(fis, 4096);
> 
>   try {
>     let addon = loadManifestFromRDF(Services.io.newFileURI(file), bis);
>     addon._sourceBundle = aDir.clone().QueryInterface(Ci.nsILocalFile);
>+    addon.size = getFileSize(aDir);
>     return addon;
>   }
>   finally {
>     bis.close();
>     fis.close();
>   }
> }
> 
>+function loadManifestFromZipReader(aZipReader) {
Please add a javadoc comment for this function

>+  let zis = aZipReader.getInputStream(FILE_INSTALL_MANIFEST);
>+  let bis = Cc["@mozilla.org/network/buffered-input-stream;1"].
>+            createInstance(Ci.nsIBufferedInputStream);
>+  bis.init(zis, 4096);
>+
>+  try {
>+    let uri = buildJarURI(aZipReader.file, FILE_INSTALL_MANIFEST);
>+    let addon = loadManifestFromRDF(uri, bis);
>+    addon._sourceBundle = aZipReader.file;
>+
>+    addon.size = 0;
>+    let entries = aZipReader.findEntries(null);
>+    while (entries.hasMore())
>+      addon.size += aZipReader.getEntry(entries.getNext()).realSize;
>+
>+    return addon;
>+  }
>+  finally {
>+    bis.close();
>+    zis.close();
>+  }
>+}
Please verify that the zip can be deleted on Windows with this code since Windows file in use has prevented us in the past from doing so.

>@@ -2363,17 +2438,18 @@ var XPIProvider = {
>       AddonManagerPrivate.notifyAddonChanged(aAddon.id, aAddon.type, false);
>   }
> };
> 
> const FIELDS_ADDON = "internal_id, id, location, version, type, internalName, " +
>                      "updateURL, updateKey, optionsURL, aboutURL, iconURL, " +
>                      "defaultLocale, visible, active, userDisabled, appDisabled, " +
>                      "pendingUninstall, descriptor, installDate, updateDate, " +
>-                     "applyBackgroundUpdates, bootstrap";
>+                     "applyBackgroundUpdates, bootstrap, skinnable, size, " +
>+                     "sourceURI, infoURI";
I think I'm ok with infoURI though I keep thinking that it should be releaseNoteURI. I leave that up to you.

>@@ -2791,34 +2897,41 @@ var XPIDatabase = {
>                                   "type TEXT, internalName TEXT, updateURL TEXT, " +
>                                   "updateKey TEXT, optionsURL TEXT, aboutURL TEXT, " +
>                                   "iconURL TEXT, defaultLocale INTEGER, " +
>                                   "visible INTEGER, active INTEGER, " +
>                                   "userDisabled INTEGER, appDisabled INTEGER, " +
>                                   "pendingUninstall INTEGER, descriptor TEXT, " +
>                                   "installDate INTEGER, updateDate INTEGER, " +
>                                   "applyBackgroundUpdates INTEGER, " +
>-                                  "bootstrap INTEGER, UNIQUE (id, location)");
>+                                  "bootstrap INTEGER, skinnable INTEGER, " +
>+                                  "size INTEGER, sourceURI TEXT, infoURI TEXT," +
s/infoURI TEXT,"/infoURI TEXT, "/
Comment on attachment 455264 [details] [diff] [review]
DB patch rev 3

>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
>...
>@@ -4774,30 +4918,38 @@ function createWrapper(aAddon) {
> 
> /**
>  * The AddonWrapper wraps an Addon to provide the data visible to consumers of
>  * the public API.
>  */
> function AddonWrapper(aAddon) {
>   ["id", "version", "type", "isCompatible",
>    "providesUpdatesSecurely", "blocklistState", "appDisabled",
>-   "userDisabled"].forEach(function(aProp) {
>+   "userDisabled", "skinnable", "size"].forEach(function(aProp) {
>      this.__defineGetter__(aProp, function() aAddon[aProp]);
>   }, this);
> 
>   ["optionsURL", "aboutURL"].forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() {
>       return aAddon.active ? aAddon[aProp] : null;
>     });
>   }, this);
> 
>   ["installDate", "updateDate"].forEach(function(aProp) {
>     this.__defineGetter__(aProp, function() new Date(aAddon[aProp]));
>   }, this);
> 
>+  ["sourceURI", "infoURI"].forEach(function(aProp) {
>+    this.__defineGetter__(aProp, function() {
>+        if (!aAddon[aProp])
>+        return null;
nit: indentation if off

>+      return NetUtil.newURI(aAddon[aProp]);
>+    });
>+  }, this);
>+

Now onto the tests
Comment on attachment 455264 [details] [diff] [review]
DB patch rev 3

r=me with comments addressed
Attachment #455264 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 454621 [details] [diff] [review]
targetPlatform support rev 1

>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
>@@ -4736,16 +4745,50 @@ AddonInternal.prototype = {
>     return !!(this.updateKey || !this.updateURL ||
>               this.updateURL.substring(0, 6) == "https:");
>   },
> 
>   get isCompatible() {
>     return this.isCompatibleWith();
>   },
> 
>+  get isPlatformCompatible() {
>+    if (this.targetPlatforms.length == 0)
>+      return true;
>+
>+    let matchedOS = false;
>+
>+    // If any targetPlatform matches the OS and contains an ABI then we will
>+    // only match a targetPlatform that contains both the current OS and ABI
>+    let needsABI = false;
>+
>+    // Some platforms do not specify an ABI, test against null in that case.
>+    let abi = null; 
nit: trailing whitespace
Attachment #454621 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 15

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/8b3d47a7e2b9
http://hg.mozilla.org/mozilla-central/rev/fbb118bcd786
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
Blocks: 566598
Verified fixed with 

If an add-on is not supported by a platform the user wants to install it on, we do not show any failure notice. I have added this issue as another todo entry on bug 577048.

Regarding the schema update, do we really have to expose the schemaVersion to the prefs.js? No-one else, i.e. places.sqlite or signons.sqlite, is doing that.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
> Verified fixed with 
> 
> If an add-on is not supported by a platform the user wants to install it on, we
> do not show any failure notice. I have added this issue as another todo entry
> on bug 577048.
> 
> Regarding the schema update, do we really have to expose the schemaVersion to
> the prefs.js? No-one else, i.e. places.sqlite or signons.sqlite, is doing that.

signons and places store their schema version in the database, but we avoid loading the database on startup when possible so we have to hold the schema version in preferences in order to detect it is outdated without loading the database.
That makes sense then when looking at perf improvements for start-up. Thanks Dave.
You need to log in before you can comment on or make changes to this bug.