Last Comment Bug 467890 - Support automatic updates for app-shipped search plugins
: Support automatic updates for app-shipped search plugins
: dev-doc-complete, fixed1.9.1
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: 3.5 Branch
: All All
: P1 normal (vote)
: Firefox 3.1b3
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
Depends on:
  Show dependency treegraph
Reported: 2008-12-04 01:18 PST by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2009-02-18 00:13 PST (History)
13 users (show)
mconnor: blocking‑firefox3.5+
hskupin: in‑testsuite?
rflint: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (21.81 KB, patch)
2009-01-22 22:02 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Review
Patch v2 + tests (31.09 KB, patch)
2009-01-24 05:23 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Review
Patch v2.1 (31.29 KB, patch)
2009-01-24 05:31 PST, Ryan Flint [:rflint] (ping via IRC for reviews) review-
Details | Diff | Review
Patch v3 (21.45 KB, patch)
2009-01-27 22:02 PST, Ryan Flint [:rflint] (ping via IRC for reviews) review+
Details | Diff | Review
Patch v3.1 (22.48 KB, patch)
2009-01-28 18:43 PST, Ryan Flint [:rflint] (ping via IRC for reviews) review+
Details | Diff | Review

Comment 1 Mike Connor [:mconnor] 2008-12-04 01:19:23 PST
This is needed to improve performance and user experience, especially for Google which does smart redirects.
Comment 2 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-13 11:15:03 PST
Spec's now (hopefully) finalized and landing this after the test suite (bug 458810) is ideal but not required. I should have a WIP up for this by EOD today.
Comment 3 Mike Beltzner [:beltzner, not reading bugmail] 2009-01-21 14:49:18 PST
Ryan: any update here? I'd like to close out our P1s by tomorrow if possible, and this has been sitting for a while.
Comment 4 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-22 22:02:58 PST
Created attachment 358339 [details] [diff] [review]

This is built on top of the test suite patch, so I'll need to separate them if this lands first. The test patch also increments CACHE_VERSION, so that's why it hasn't been done here.
Comment 5 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-24 05:23:27 PST
Created attachment 358595 [details] [diff] [review]
Patch v2 + tests
Comment 6 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-24 05:31:31 PST
Created attachment 358597 [details] [diff] [review]
Patch v2.1

Bah, wrong patch.
Comment 7 :Gavin Sharp [email:] 2009-01-26 14:45:48 PST
Comment on attachment 358597 [details] [diff] [review]
Patch v2.1

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+      if (engineToUpdate._isInAppDir) {

>+        newFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, PERMS_FILE);
>+        aEngine._file = newFile;
>+        aEngine._originalFile = engineToUpdate._file;

Doesn't this need to deal with an existing override? create will throw in that case...

I'm not sure I understand why we can't just keep _file pointing to the actual file on disk, but just avoid calls to _serializeToFile in favor of _buildCache. What am I missing? Would have been best if we'd agreed on an approach before you did the work, I guess...

>+  // Whether or not the updateURL needs to be expanded prior to use
>+  _isSelfUpdateURL: false,

>+      // If we're updating an app-shipped engine, ensure the updateURLs are the same
>+      let oldUpdateURL = engineToUpdate._updateURL;
>+      let newUpdateURL = aEngine._updateURL;
>+      if (engineToUpdate._isSelfUpdateURL) {
>+        oldUpdateURL = engineToUpdate._updateURL.template;
>+        newUpdateURL = aEngine._updateURL.template;
>+      }

Using _updateURL for either an update URL string or an update EngineURL object and using _isSelfUpdateURL to disambiguate is confusing. Can't you just use _getURLOfType(URLTYPE_OPENSEARCH) for everywhere that you currently use the EngineURL-_updateURL, and leave _updateURL null? Would also avoid the need for special hacks in the cache loading/serialization.

>-    this._urls.push(url);
>+    if (aElement.hasAttribute("rel")) {
>+      let rels = aElement.getAttribute("rel").toLowerCase().split(" ");

.split(/\s+/) ?

>+      for (let i = 0; i < rels.length; i++) {
>+        if (rels[i] == "self" && type == URLTYPE_OPENSEARCH) {

Hoist the type check up to avoid splitting/iterating at all if we're going to ignore the results anyways?

I didn't review the tests yet.
Comment 8 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-27 22:02:26 PST
Created attachment 359212 [details] [diff] [review]
Patch v3
Comment 9 :Gavin Sharp [email:] 2009-01-28 14:18:26 PST
Comment on attachment 359212 [details] [diff] [review]
Patch v3

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+  this.rels     = null;

Doesn't this need to be [] to avoid throwing in _hasRelation without it set? Might be good to get a test for this case (opensearch/blabla URL that has no rel?).

>+  _hasRelation: function SRC_EURL__hasRelation(aRel)
>+    this.rels.some(function(e) e == aRel.toLowerCase()),

>+    if (aJson.rels)
>+      this.rels = aJson.rels;

>+    if (this.rels)
>+      json.rels = this.rels;

With that changed, these can just assign unconditionally right?

>+    if (this.rels)
>+      url.setAttribute("rel", this.rels.join(" "));

Though this will need to check rels.length.

>     // Write the engine to file
>-    aEngine._serializeToFile();
>+    if (!aEngine._isInAppDir)
>+      aEngine._serializeToFile();

>-            if (aEngine._file)
>+            if (aEngine._file && !aEngine._isInAppDir)

I think these checks should actually be checking _readOnly, since we can have non-appdir readOnly engines (e.g. sherlock files that failed conversion for some reason). Would probably also be good to add a comment explaining that we don't need to worry about triggering a cache rebuild in these cases since the _LOADED and _CHANGED notifications that come after this take care of it.

>+    // Check for updates on the first use of an app-shipped engine
>+    if (this._isInAppDir && !engineMetadataService.getAttr(this, "used")) {

getSubmission can be called fairly often (multiple times when using search suggest, for example). Perhaps we should limit this to only checking "used" when aResponseType == URLTYPE_SEARCH_HTML, and perhaps we should also check _hasUpdates here before the attr check, instead of relying on the check in update(). Could also cache the "used" value on the object itself to avoid hitting the DB each time (or even change the metaDataService to do that caching, though that's probably best done in a followup).

>+        if (engineMetadataService.getAttr(addedEngine, "used"))
>+          engineMetadataService.setAttr(addedEngine, "used", false);

Similarly, it would probably be best to wipe out all the "used" flags with a single statement once loading is complete, rather than hitting the DB for each engine load individually. Perhaps this is followup material as well.

>+    // We use the cache to store updated app engines, so refuse to update if the
>+    // cache is disabled.
>+    if (engine._isInAppDir &&
>+        !getBoolPref(BROWSER_SEARCH_PREF + "cache.enabled", true))
>+      return;

This should probably also be a _readOnly check rather than _isInAppDir.

>+    // We don't use _isDefault because we don't want to allow extensions to update
>+    // their components outside of the normal update process.
>+    if (engine._readOnly && !engine._isInAppDir) {

Hrm, these flags are starting to get a bit confusing, but I think checking __installLocation == SEARCH_IN_EXTENSION (or a _isInExtension getter that returns that) is what we really want, since there's no reason to avoid updates for readOnly profile directory plugins (however rare they might be). Though really, what's the reason to not allow updating extension shipped plugins? Might be useful for distro builds...

>+    let updateURL = engine._getURLOfType(URLTYPE_OPENSEARCH);
>+    let updateURI = (updateURL._hasRelation("self")) ? 
>+                     updateURL.getSubmission("", engine).uri :
>+                     makeURI(engine._updateURL);

Need to null check updateURL, right? Can we get a test that would have caught this?

>+    if (updateURI) {
>+      if (engine._isInAppDir && !updateURI.schemeIs("https")) {
>+        ULOG("Invalid scheme for default engine update");

... and if we do decide to allow updates to extension-shipped plugins, this should be a _isDefault check rather than _isInAppDir.

>diff --git a/toolkit/components/search/tests/unit/test_update.js b/toolkit/components/search/tests/unit/test_update.js

>+  observe: function(aSub, aTop, aData) {
>+    do_test_finished();
>+    do_test_pending();

Does the harness actually support this? Seems rather odd to finish a test then restart it... Still haven't reviewed the tests too closely, but this diff seems to be against a local changeset so that's kind of hard.

r=me with those addressed, but I'll want to take a look at the final patch again before landing.
Comment 10 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-28 18:43:23 PST
Created attachment 359438 [details] [diff] [review]
Patch v3.1
Comment 11 :Gavin Sharp [email:] 2009-01-28 18:50:55 PST
Comment on attachment 359438 [details] [diff] [review]
Patch v3.1

The _used setter should probably just call setAttr given that both of it's callers want that.

+    ULOG("update called");

include the name of the engine it was called for?

Are the tests ready to land? I'd like to review them completely, but I can do that after they land if they're ready.
Comment 12 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-28 21:36:50 PST

The tests will be done in bug 458810.
Comment 13 Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-28 22:46:54 PST
The OpenSearch page is protected on MDC, so see (in particular the rel="self" bits) for reference on the new URL type.
Comment 15 Henrik Skupin (:whimboo) 2009-02-18 00:13:00 PST
(In reply to comment #12)
> The tests will be done in bug 458810.

I wonder why in-testsuite already got a plus. There is no test checked-in yet. Ryan when can we expect the tests on bug 458810?

Note You need to log in before you can comment on or make changes to this bug.