467890 - Support automatic updates for app-shipped search plugins

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

See https://wiki.mozilla.org/User:Mconnor/SearchUpdates and http://groups.google.com/group/opensearch/browse_thread/thread/6ff025baa2159832

Comment 1

[Mike Connor [:mconnor]]

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)]

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]]

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)]

Attachment: Patch

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)]

Attachment: Patch v2 + tests

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2.1

Bah, wrong patch.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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)]

Attachment: Patch v3

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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)]

Attachment: Patch v3.1

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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)]

http://hg.mozilla.org/mozilla-central/rev/ecba698e87b5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/24b24956c83b

The tests will be done in bug 458810.

Comment 13

[Ryan Flint [:rflint] (ping via IRC for reviews)]

The OpenSearch page is protected on MDC, so see http://www.opensearch.org/Specifications/OpenSearch/1.1/Draft_4#The_.22Url.22_element (in particular the rel="self" bits) for reference on the new URL type.

Comment 14

[Eric Shepherd [:sheppy]]

Now documented here:

https://developer.mozilla.org/en/Creating_OpenSearch_plugins_for_Firefox#Supporting_automatic_updates_for_OpenSearch_plugins

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> http://hg.mozilla.org/mozilla-central/rev/ecba698e87b5
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/24b24956c83b
> 
> 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?