Bug 1432434 - Remove Bookmarks::getItemDateAdded and setItemDateAdded. r=kitcambridge draft
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 08 Feb 2018 23:24:38 +0100
changeset 753543 18213447f00177686dbc56d263674fa4e630663e
parent 753522 a8e153c55eeee93a11e87d325fb20c644421036f
push id98586
push usermak77@bonardo.net
push dateSat, 10 Feb 2018 15:34:49 +0000
reviewerskitcambridge
bugs1432434
milestone60.0a1
Bug 1432434 - Remove Bookmarks::getItemDateAdded and setItemDateAdded. r=kitcambridge MozReview-Commit-ID: XVoil1CK7c
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/nsINavBookmarksService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/tests/legacy/test_bookmarks.js
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -554,38 +554,53 @@ add_task(async function test_async_onIte
 
 add_task(async function test_onItemChanged_itemDates() {
   _("Changes to item dates should be tracked");
 
   try {
     await tracker.stop();
 
     _("Insert a bookmark");
-    let fx_id = PlacesUtils.bookmarks.insertBookmark(
-      PlacesUtils.bookmarks.bookmarksMenuFolder,
-      CommonUtils.makeURI("http://getfirefox.com"),
-      PlacesUtils.bookmarks.DEFAULT_INDEX,
-      "Get Firefox!");
-    let fx_guid = await PlacesUtils.promiseItemGuid(fx_id);
-    _(`Firefox GUID: ${fx_guid}`);
+    let fx_bm = await PlacesUtils.bookmarks.insert({
+      parentGuid: PlacesUtils.bookmarks.menuGuid,
+      url: "http://getfirefox.com",
+      title: "Get Firefox!"
+    });
+    _(`Firefox GUID: ${fx_bm.guid}`);
 
     await startTracking();
 
-    _("Reset the bookmark's added date");
-    // Convert to microseconds for PRTime.
-    let dateAdded = (Date.now() - DAY_IN_MS) * 1000;
-    PlacesUtils.bookmarks.setItemDateAdded(fx_id, dateAdded);
-    await verifyTrackedItems([fx_guid]);
+    _("Reset the bookmark's added date, should not be tracked");
+    let dateAdded = new Date(Date.now() - DAY_IN_MS);
+    await PlacesUtils.bookmarks.update({
+      guid: fx_bm.guid,
+      dateAdded
+    });
+    await verifyTrackedCount(0);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+
+    await resetTracker();
+
+    _("Reset the bookmark's added date and another property, should be tracked");
+    dateAdded = new Date();
+    await PlacesUtils.bookmarks.update({
+      guid: fx_bm.guid,
+      dateAdded,
+      title: "test"
+    });
+    await verifyTrackedItems([fx_bm.guid]);
+    Assert.equal(tracker.score, 2 * SCORE_INCREMENT_XLARGE);
+
     await resetTracker();
 
     _("Set the bookmark's last modified date");
+    let fx_id = await PlacesUtils.promiseItemId(fx_bm.guid);
     let dateModified = Date.now() * 1000;
     PlacesUtils.bookmarks.setItemLastModified(fx_id, dateModified);
-    await verifyTrackedItems([fx_guid]);
+    await verifyTrackedItems([fx_bm.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
   } finally {
     _("Clean up.");
     await cleanup();
   }
 });
 
 add_task(async function test_onItemTagged() {
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -499,42 +499,16 @@ interface nsINavBookmarksService : nsISu
    *
    *  @param aItemId
    *         The id of the item whose title should be retrieved
    *  @return The title of the item.
    */
   AUTF8String getItemTitle(in long long aItemId);
 
   /**
-   * Set the date added time for an item.
-   *
-   * @param aItemId
-   *        the id of the item whose date added time should be updated.
-   * @param aDateAdded
-   *        the new date added value in microseconds.  Note that it is rounded
-   *        down to milliseconds precision.
-   *  @param [optional] aSource
-   *         The change source, forwarded to all bookmark observers. Defaults
-   *         to SOURCE_DEFAULT.
-   */
-  void setItemDateAdded(in long long aItemId,
-                        in PRTime aDateAdded,
-                        [optional] in unsigned short aSource);
-
-  /**
-   * Get the date added time for an item.
-   *
-   * @param aItemId
-   *        the id of the item whose date added time should be retrieved.
-   *
-   * @return the date added value in microseconds.
-   */
-  PRTime getItemDateAdded(in long long aItemId);
-
-  /**
    * Set the last modified time for an item.
    *
    * @param aItemId
    *        the id of the item whose last modified time should be updated.
    * @param aLastModified
    *        the new last modified value in microseconds.  Note that it is
    *        rounded down to milliseconds precision.
    * @param [optional] aSource
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -1500,34 +1500,21 @@ nsNavBookmarks::FetchItemInfo(int64_t aI
 nsresult
 nsNavBookmarks::SetItemDateInternal(enum BookmarkDate aDateType,
                                     int64_t aSyncChangeDelta,
                                     int64_t aItemId,
                                     PRTime aValue)
 {
   aValue = RoundToMilliseconds(aValue);
 
-  nsCOMPtr<mozIStorageStatement> stmt;
-  if (aDateType == DATE_ADDED) {
-    // lastModified is set to the same value as dateAdded.  We do this for
-    // performance reasons, since it will allow us to use an index to sort items
-    // by date.
-    stmt = mDB->GetStatement(
-      "UPDATE moz_bookmarks SET dateAdded = :date, lastModified = :date, "
-       "syncChangeCounter = syncChangeCounter + :delta "
-      "WHERE id = :item_id"
-    );
-  }
-  else {
-    stmt = mDB->GetStatement(
-      "UPDATE moz_bookmarks SET lastModified = :date, "
-       "syncChangeCounter = syncChangeCounter + :delta "
-      "WHERE id = :item_id"
-    );
-  }
+  nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
+    "UPDATE moz_bookmarks SET lastModified = :date, "
+      "syncChangeCounter = syncChangeCounter + :delta "
+    "WHERE id = :item_id"
+  );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
 
   nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("date"), aValue);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("delta"), aSyncChangeDelta);
@@ -1537,86 +1524,16 @@ nsNavBookmarks::SetItemDateInternal(enum
   NS_ENSURE_SUCCESS(rv, rv);
 
   // note, we are not notifying the observers
   // that the item has changed.
 
   return NS_OK;
 }
 
-
-NS_IMETHODIMP
-nsNavBookmarks::SetItemDateAdded(int64_t aItemId, PRTime aDateAdded,
-                                 uint16_t aSource)
-{
-  NS_ENSURE_ARG_MIN(aItemId, 1);
-
-  BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark);
-  NS_ENSURE_SUCCESS(rv, rv);
-  int64_t tagsRootId = TagsRootId();
-  bool isTagging = bookmark.grandParentId == tagsRootId;
-  int64_t syncChangeDelta = DetermineSyncChangeDelta(aSource);
-
-  // Round here so that we notify with the right value.
-  bookmark.dateAdded = RoundToMilliseconds(aDateAdded);
-
-  if (isTagging) {
-    // If we're changing a tag, bump the change counter for all tagged
-    // bookmarks. We use a separate code path to avoid a transaction for
-    // non-tags.
-    mozStorageTransaction transaction(mDB->MainConn(), false);
-
-    rv = SetItemDateInternal(DATE_ADDED, syncChangeDelta, bookmark.id,
-                             bookmark.dateAdded);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = AddSyncChangesForBookmarksWithURL(bookmark.url, syncChangeDelta);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = transaction.Commit();
-    NS_ENSURE_SUCCESS(rv, rv);
-  } else {
-    rv = SetItemDateInternal(DATE_ADDED, syncChangeDelta, bookmark.id,
-                             bookmark.dateAdded);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  // Note: mDBSetItemDateAdded also sets lastModified to aDateAdded.
-  NOTIFY_OBSERVERS(mCanNotify, mObservers, nsINavBookmarkObserver,
-                   OnItemChanged(bookmark.id,
-                                 NS_LITERAL_CSTRING("dateAdded"),
-                                 false,
-                                 nsPrintfCString("%" PRId64, bookmark.dateAdded),
-                                 bookmark.dateAdded,
-                                 bookmark.type,
-                                 bookmark.parentId,
-                                 bookmark.guid,
-                                 bookmark.parentGuid,
-                                 EmptyCString(),
-                                 aSource));
-  return NS_OK;
-}
-
-
-NS_IMETHODIMP
-nsNavBookmarks::GetItemDateAdded(int64_t aItemId, PRTime* _dateAdded)
-{
-  NS_ENSURE_ARG_MIN(aItemId, 1);
-  NS_ENSURE_ARG_POINTER(_dateAdded);
-
-  BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  *_dateAdded = bookmark.dateAdded;
-  return NS_OK;
-}
-
-
 NS_IMETHODIMP
 nsNavBookmarks::SetItemLastModified(int64_t aItemId, PRTime aLastModified,
                                     uint16_t aSource)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
 
   BookmarkData bookmark;
   nsresult rv = FetchItemInfo(aItemId, bookmark);
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -64,18 +64,17 @@ namespace places {
     nsCString guid;
     PRTime dateRemoved;
   };
 
   typedef void (nsNavBookmarks::*ItemVisitMethod)(const ItemVisitData&);
   typedef void (nsNavBookmarks::*ItemChangeMethod)(const ItemChangeData&);
 
   enum BookmarkDate {
-    DATE_ADDED = 0
-  , LAST_MODIFIED
+    LAST_MODIFIED
   };
 
 } // namespace places
 } // namespace mozilla
 
 class nsNavBookmarks final : public nsINavBookmarksService
                            , public nsINavHistoryObserver
                            , public nsIObserver
--- a/toolkit/components/places/tests/legacy/test_bookmarks.js
+++ b/toolkit/components/places/tests/legacy/test_bookmarks.js
@@ -124,56 +124,43 @@ add_task(async function test_bookmarks()
   let newId = bs.insertBookmark(testRoot, uri("http://google.com/"),
                                 bs.DEFAULT_INDEX, "");
   Assert.equal(bookmarksObserver._itemAddedId, newId);
   Assert.equal(bookmarksObserver._itemAddedParent, testRoot);
   Assert.equal(bookmarksObserver._itemAddedIndex, testStartIndex);
   Assert.ok(bookmarksObserver._itemAddedURI.equals(uri("http://google.com/")));
   Assert.equal(bs.getBookmarkURI(newId).spec, "http://google.com/");
 
-  let dateAdded = bs.getItemDateAdded(newId);
-  // dateAdded can equal beforeInsert
-  Assert.ok(is_time_ordered(beforeInsert, dateAdded));
-
   // after just inserting, modified should not be set
   let lastModified = PlacesUtils.toPRTime((await PlacesUtils.bookmarks.fetch(
     await PlacesUtils.promiseItemGuid(newId))).lastModified);
-  Assert.equal(lastModified, dateAdded);
 
   // The time before we set the title, in microseconds.
   let beforeSetTitle = Date.now() * 1000;
   Assert.ok(beforeSetTitle >= beforeInsert);
 
   // Workaround possible VM timers issues moving lastModified and dateAdded
   // to the past.
   lastModified -= 1000;
   bs.setItemLastModified(newId, lastModified);
-  dateAdded -= 1000;
-  bs.setItemDateAdded(newId, dateAdded);
 
   // set bookmark title
   bs.setItemTitle(newId, "Google");
   Assert.equal(bookmarksObserver._itemChangedId, newId);
   Assert.equal(bookmarksObserver._itemChangedProperty, "title");
   Assert.equal(bookmarksObserver._itemChangedValue, "Google");
 
-  // check that dateAdded hasn't changed
-  let dateAdded2 = bs.getItemDateAdded(newId);
-  Assert.equal(dateAdded2, dateAdded);
-
   // check lastModified after we set the title
   let lastModified2 = PlacesUtils.toPRTime((await PlacesUtils.bookmarks.fetch(
     await PlacesUtils.promiseItemGuid(newId))).lastModified);
   info("test setItemTitle");
-  info("dateAdded = " + dateAdded);
   info("beforeSetTitle = " + beforeSetTitle);
   info("lastModified = " + lastModified);
   info("lastModified2 = " + lastModified2);
   Assert.ok(is_time_ordered(lastModified, lastModified2));
-  Assert.ok(is_time_ordered(dateAdded, lastModified2));
 
   // get item title
   let title = bs.getItemTitle(newId);
   Assert.equal(title, "Google");
 
   // get item title bad input
   try {
     bs.getItemTitle(-3);
@@ -392,28 +379,21 @@ add_task(async function test_bookmarks()
     rootNode.containerOpen = false;
   } catch (ex) {
     do_throw("bookmarks query: " + ex);
   }
 
   // test change bookmark uri
   let newId10 = bs.insertBookmark(testRoot, uri("http://foo10.com/"),
                                   bs.DEFAULT_INDEX, "");
-  dateAdded = bs.getItemDateAdded(newId10);
-  // after just inserting, modified should not be set
-  lastModified = PlacesUtils.toPRTime((await PlacesUtils.bookmarks.fetch(
-    await PlacesUtils.promiseItemGuid(newId10))).lastModified);
-  Assert.equal(lastModified, dateAdded);
 
   // Workaround possible VM timers issues moving lastModified and dateAdded
   // to the past.
   lastModified -= 1000;
   bs.setItemLastModified(newId10, lastModified);
-  dateAdded -= 1000;
-  bs.setItemDateAdded(newId10, dateAdded);
 
   // test getBookmarkURI
   let newId11 = bs.insertBookmark(testRoot, uri("http://foo10.com/"),
                                   bs.DEFAULT_INDEX, "");
   let bmURI = bs.getBookmarkURI(newId11);
   Assert.equal("http://foo10.com/", bmURI.spec);
 
   // test getBookmarkURI with non-bookmark items
@@ -520,30 +500,23 @@ add_task(async function test_bookmarks()
         break;
       }
     }
     rootNode.containerOpen = false;
   } catch (ex) {
     do_throw("bookmarks query: " + ex);
   }
 
-  // check setItemLastModified() and setItemDateAdded()
+  // check setItemLastModified()
   let newId14 = bs.insertBookmark(testRoot, uri("http://bar.tld/"),
                                   bs.DEFAULT_INDEX, "");
-  dateAdded = bs.getItemDateAdded(newId14);
-  lastModified = PlacesUtils.toPRTime((await PlacesUtils.bookmarks.fetch(
-    await PlacesUtils.promiseItemGuid(newId14))).lastModified);
-  Assert.equal(lastModified, dateAdded);
   bs.setItemLastModified(newId14, 1234000000000000);
   let fakeLastModified = PlacesUtils.toPRTime((await PlacesUtils.bookmarks.fetch(
     await PlacesUtils.promiseItemGuid(newId14))).lastModified);
   Assert.equal(fakeLastModified, 1234000000000000);
-  bs.setItemDateAdded(newId14, 4321000000000000);
-  let fakeDateAdded = bs.getItemDateAdded(newId14);
-  Assert.equal(fakeDateAdded, 4321000000000000);
 
   // ensure that removing an item removes its annotations
   Assert.ok(anno.itemHasAnnotation(newId3, "test-annotation"));
   bs.removeItem(newId3);
   Assert.ok(!anno.itemHasAnnotation(newId3, "test-annotation"));
 
   // bug 378820
   let uri1 = uri("http://foo.tld/a");
@@ -574,36 +547,24 @@ function testSimpleFolderResult() {
   // the time before we create a folder, in microseconds
   // Workaround possible VM timers issues subtracting 1us.
   let beforeCreate = Date.now() * 1000 - 1;
   Assert.ok(beforeCreate > 0);
 
   // create a folder
   let parent = bs.createFolder(root, "test", bs.DEFAULT_INDEX);
 
-  let dateCreated = bs.getItemDateAdded(parent);
-  info("check that the folder was created with a valid dateAdded");
-  info("beforeCreate = " + beforeCreate);
-  info("dateCreated = " + dateCreated);
-  Assert.ok(is_time_ordered(beforeCreate, dateCreated));
-
   // the time before we insert, in microseconds
   // Workaround possible VM timers issues subtracting 1ms.
   let beforeInsert = Date.now() * 1000 - 1;
   Assert.ok(beforeInsert > 0);
 
   // insert a separator
   let sep = bs.insertSeparator(parent, bs.DEFAULT_INDEX);
 
-  let dateAdded = bs.getItemDateAdded(sep);
-  info("check that the separator was created with a valid dateAdded");
-  info("beforeInsert = " + beforeInsert);
-  info("dateAdded = " + dateAdded);
-  Assert.ok(is_time_ordered(beforeInsert, dateAdded));
-
   // re-set item title separately so can test nodes' last modified
   let item = bs.insertBookmark(parent, uri("about:blank"),
                                bs.DEFAULT_INDEX, "");
   bs.setItemTitle(item, "test bookmark");
 
   // see above
   let folder = bs.createFolder(parent, "test folder", bs.DEFAULT_INDEX);
   bs.setItemTitle(folder, "test folder");