Bug 1148201 - Fix desktop reading list client use of Last-Modified, If-Unmodified-Since, If-Modified-Since headers. r=rnewman
authorDrew Willcoxon <adw@mozilla.com>
Fri, 27 Mar 2015 17:14:57 -0700
changeset 266540 5fde3c10ff0128f0e872373198f613c1a3bb22c8
parent 266539 70a0676ca7c69f38ccdcbf1a01be98169d5805b1
child 266541 9511efa9a8ebab205ba24fa467179aaa3849f770
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1148201
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1148201 - Fix desktop reading list client use of Last-Modified, If-Unmodified-Since, If-Modified-Since headers. r=rnewman
browser/components/readinglist/Sync.jsm
--- a/browser/components/readinglist/Sync.jsm
+++ b/browser/components/readinglist/Sync.jsm
@@ -183,47 +183,45 @@ SyncImpl.prototype = {
       method: "POST",
       path: "/batch",
       body: {
         defaults: {
           method: "PATCH",
         },
         requests: requests,
       },
-      headers: {},
     };
-    if (this._serverLastModifiedHeader) {
-      request.headers["If-Unmodified-Since"] = this._serverLastModifiedHeader;
-    }
-
     let batchResponse = yield this._sendRequest(request);
     if (batchResponse.status != 200) {
       this._handleUnexpectedResponse("uploading changes", batchResponse);
       return;
     }
 
     // Update local items based on the response.
     for (let response of batchResponse.body.responses) {
       if (response.status == 404) {
         // item deleted
         yield this._deleteItemForGUID(response.body.id);
         continue;
       }
-      if (response.status == 412 || response.status == 409) {
-        // 412 Precondition failed: The item was modified since the last sync.
-        // 409 Conflict: A change violated a uniqueness constraint.
-        // In either case, mark the item as having material changes, and
-        // reconcile and upload it in the material-changes phase.
+      if (response.status == 409) {
+        // "Conflict": A change violated a uniqueness constraint.  Mark the item
+        // as having material changes, and reconcile and upload it in the
+        // material-changes phase.
         // TODO
         continue;
       }
       if (response.status != 200) {
         this._handleUnexpectedResponse("uploading a change", response);
         continue;
       }
+      // Don't assume the local record and the server record aren't materially
+      // different.  Reconcile the differences.
+      // TODO
+
       let item = yield this._itemForGUID(response.body.id);
       yield this._updateItemWithServerRecord(item, response.body);
     }
   }),
 
   /**
    * Phase 1 part 2
    *
@@ -250,22 +248,17 @@ SyncImpl.prototype = {
       path: "/batch",
       body: {
         defaults: {
           method: "POST",
           path: "/articles",
         },
         requests: requests,
       },
-      headers: {},
     };
-    if (this._serverLastModifiedHeader) {
-      request.headers["If-Unmodified-Since"] = this._serverLastModifiedHeader;
-    }
-
     let batchResponse = yield this._sendRequest(request);
     if (batchResponse.status != 200) {
       this._handleUnexpectedResponse("uploading new items", batchResponse);
       return;
     }
 
     // Update local items based on the response.
     for (let response of batchResponse.body.responses) {
@@ -318,37 +311,25 @@ SyncImpl.prototype = {
       method: "POST",
       path: "/batch",
       body: {
         defaults: {
           method: "DELETE",
         },
         requests: requests,
       },
-      headers: {},
     };
-    if (this._serverLastModifiedHeader) {
-      request.headers["If-Unmodified-Since"] = this._serverLastModifiedHeader;
-    }
-
     let batchResponse = yield this._sendRequest(request);
     if (batchResponse.status != 200) {
       this._handleUnexpectedResponse("uploading deleted items", batchResponse);
       return;
     }
 
     // Delete local items based on the response.
     for (let response of batchResponse.body.responses) {
-      if (response.status == 412) {
-        // "Precondition failed": The item was modified since the last sync.
-        // Mark the item as having material changes, and reconcile and upload it
-        // in the material-changes phase.
-        // TODO
-        continue;
-      }
       // A 404 means the item was already deleted on the server, which is OK.
       // We still need to make sure it's deleted locally, though.
       if (response.status != 200 && response.status != 404) {
         this._handleUnexpectedResponse("uploading a deleted item", response);
         continue;
       }
       yield this._deleteItemForGUID(response.body.id);
     }
@@ -365,28 +346,18 @@ SyncImpl.prototype = {
     // Get modified items from the server.
     let path = "/articles";
     if (this._serverLastModifiedHeader) {
       path += "?_since=" + this._serverLastModifiedHeader;
     }
     let request = {
       method: "GET",
       path: path,
-      headers: {},
     };
-    if (this._serverLastModifiedHeader) {
-      request.headers["If-Modified-Since"] = this._serverLastModifiedHeader;
-    }
-
     let response = yield this._sendRequest(request);
-    if (response.status == 304) {
-      // not modified
-      log.debug("No server changes");
-      return;
-    }
     if (response.status != 200) {
       this._handleUnexpectedResponse("downloading modified items", response);
       return;
     }
 
     // Update local items based on the response.
     for (let serverRecord of response.body.items) {
       let localItem = yield this._itemForGUID(serverRecord.id);
@@ -412,16 +383,23 @@ SyncImpl.prototype = {
       let localRecord = localRecordFromServerRecord(serverRecord);
       try {
         yield this.list.addItem(localRecord);
       } catch (ex) {
         log.warn("Failed to add a new item from server record ${serverRecord}: ${ex}",
                  {serverRecord, ex});
       }
     }
+
+    // Now that changes have been successfully applied, advance the server
+    // last-modified timestamp so that next time we fetch items starting from
+    // the current point.  Response header names are lowercase.
+    if (response.headers && "last-modified" in response.headers) {
+      this._serverLastModifiedHeader = response.headers["last-modified"];
+    }
   }),
 
   /**
    * Phase 3 (material changes)
    *
    * Uploads not-new items with material changes.
    */
   _uploadMaterialChanges: Task.async(function* () {
@@ -501,20 +479,16 @@ SyncImpl.prototype = {
    * @param req The request object: { method, path, body, headers }.
    * @return Promise<response> Resolved with the server's response object:
    *         { status, body, headers }.
    */
   _sendRequest: Task.async(function* (req) {
     log.debug("Sending request", req);
     let response = yield this._client.request(req);
     log.debug("Received response", response);
-    // Response header names are lowercase.
-    if (response.headers && "last-modified" in response.headers) {
-      this._serverLastModifiedHeader = response.headers["last-modified"];
-    }
     return response;
   }),
 
   _handleUnexpectedResponse(contextMsgFragment, response) {
     log.warn(`Unexpected response ${contextMsgFragment}`, response);
   },
 
   // TODO: Wipe this pref when user logs out.