Closed Bug 467890 Opened 12 years ago Closed 11 years ago

Support automatic updates for app-shipped search plugins


(Firefox :: Search, defect, P1)

3.5 Branch



Firefox 3.1b3


(Reporter: rflint, Assigned: rflint)




(Keywords: dev-doc-complete, fixed1.9.1)


(1 file, 4 obsolete files)

This is needed to improve performance and user experience, especially for Google which does smart redirects.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P1
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.
Ryan: any update here? I'd like to close out our P1s by tomorrow if possible, and this has been sitting for a while.
Whiteboard: [eta?]
Attached patch Patch (obsolete) — Splinter 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.
Attachment #358339 - Flags: review?(
Whiteboard: [eta?]
Attached patch Patch v2 + tests (obsolete) — Splinter Review
Attachment #358339 - Attachment is obsolete: true
Attachment #358595 - Flags: review?(
Attachment #358339 - Flags: review?(
Summary: Support automatic updates for search plugins → Support automatic updates for app-shipped search plugins
Attached patch Patch v2.1 (obsolete) — Splinter Review
Bah, wrong patch.
Attachment #358595 - Attachment is obsolete: true
Attachment #358597 - Flags: review?(
Attachment #358595 - Flags: review?(
Whiteboard: [needs review gavin]
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.
Attachment #358597 - Flags: review?( → review-
Whiteboard: [needs review gavin] → [needs new patch]
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #358597 - Attachment is obsolete: true
Attachment #359212 - Flags: review?(
Whiteboard: [needs new patch] → [needs review gavin]
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.
Attachment #359212 - Flags: review?( → review+
Whiteboard: [needs review gavin] → [needs new patch]
Attached patch Patch v3.1Splinter Review
Attachment #359212 - Attachment is obsolete: true
Attachment #359438 - Flags: review?(
Whiteboard: [needs new patch]
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.
Attachment #359438 - Flags: review?( → review+

The tests will be done in bug 458810.
Closed: 11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Keywords: fixed1.9.1
Resolution: --- → FIXED
The OpenSearch page is protected on MDC, so see (in particular the rel="self" bits) for reference on the new URL type.
Keywords: dev-doc-needed
(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?
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.