Bug 1479445 - Return a new PageInfo object from _recordToPageInfo so that validateItemProperties can validate the object. r=lina
authorSiddhant085 <dpsrkp.sid@gmail.com>
Sat, 06 Oct 2018 19:07:12 +0000
changeset 495668 aa2bf2716f9bbc3e5c200edae3ed347f7d423d6d
parent 495667 fa8c1d18270920a5a98263c9836f4c3b240140aa
child 495669 45da44f3bfe569bc23cdfa53b2f7775863ccbe5f
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslina
bugs1479445
milestone64.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 1479445 - Return a new PageInfo object from _recordToPageInfo so that validateItemProperties can validate the object. r=lina D5831 refactors validatePageInfo to use validateItemProperties. The record generated by _recordToPlaceInfo makes use of getters. validateItemProperties does a shallow copy of the properties and the visits properties is being left out because of the use of getters. Therefore as discussed we are returning a new object from _recordToPlaceInfo. Differential Revision: https://phabricator.services.mozilla.com/D7467
services/sync/modules/engines/history.js
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -210,18 +210,19 @@ HistoryStore.prototype = {
     let failed = [];
     let toAdd = [];
     let toRemove = [];
     for await (let record of Async.yieldingIterator(records)) {
       if (record.deleted) {
         toRemove.push(record);
       } else {
         try {
-          if (await this._recordToPlaceInfo(record)) {
-            toAdd.push(record);
+          let pageInfo = await this._recordToPlaceInfo(record);
+          if (pageInfo) {
+            toAdd.push(pageInfo);
           }
         } catch (ex) {
           if (Async.isShutdownException(ex)) {
             throw ex;
           }
           this._log.error("Failed to create a place info", ex);
           this._log.trace("The record that failed", record);
           failed.push(record.id);
@@ -325,35 +326,35 @@ HistoryStore.prototype = {
   _canAddURI(uri) {
     return PlacesUtils.history.canAddURI(uri);
   },
 
   /**
    * Converts a Sync history record to a mozIPlaceInfo.
    *
    * Throws if an invalid record is encountered (invalid URI, etc.),
-   * returns true if the record is to be applied, false otherwise
-   * (no visits to add, etc.),
+   * returns a new PageInfo object if the record is to be applied, null
+   * otherwise (no visits to add, etc.),
    */
   async _recordToPlaceInfo(record) {
     // Sort out invalid URIs and ones Places just simply doesn't want.
     record.url = PlacesUtils.normalizeToURLOrGUID(record.histUri);
     record.uri = CommonUtils.makeURI(record.histUri);
 
     if (!Utils.checkGUID(record.id)) {
       this._log.warn("Encountered record with invalid GUID: " + record.id);
-      return false;
+      return null;
     }
     record.guid = record.id;
 
     if (!this._canAddURI(record.uri) ||
         !this.engine.shouldSyncURL(record.uri.spec)) {
       this._log.trace("Ignoring record " + record.id + " with URI "
                       + record.uri.spec + ": can't add this URI.");
-      return false;
+      return null;
     }
 
     // We dupe visits by date and type. So an incoming visit that has
     // the same timestamp and type as a local one won't get applied.
     // To avoid creating new objects, we rewrite the query result so we
     // can simply check for containment below.
     let curVisitsAsArray = [];
     let curVisits = new Set();
@@ -424,20 +425,30 @@ HistoryStore.prototype = {
 
     // No update if there aren't any visits to apply.
     // History wants at least one visit.
     // In any case, the only thing we could change would be the title
     // and that shouldn't change without a visit.
     if (!record.visits.length) {
       this._log.trace("Ignoring record " + record.id + " with URI "
                       + record.uri.spec + ": no visits to add.");
-      return false;
+      return null;
     }
 
-    return true;
+    // PageInfo is validated using validateItemProperties which does a shallow
+    // copy of the properties. Since record uses getters some of the properties
+    // are not copied over. Thus we create and return a new object.
+    let pageInfo = {
+      title: record.title,
+      url: record.url,
+      guid: record.guid,
+      visits: record.visits,
+    };
+
+    return pageInfo;
   },
 
   async remove(record) {
     this._log.trace("Removing page: " + record.id);
     let removed = await PlacesUtils.history.remove(record.id);
     if (removed) {
       this._log.trace("Removed page: " + record.id);
     } else {