Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak draft
authorbsilverberg <bsilverberg@mozilla.com>
Thu, 03 Mar 2016 08:07:16 -0500
changeset 336494 57a25f43d633ca87fceb567555943523fef0ae3d
parent 335498 04634ec900b2fb94962733148ecdeac7ae98e0e2
child 515417 dd6dd1b45425c1f60dbef941c731cfc5b4ec040f
push id12089
push userbmo:bob.silverberg@gmail.com
push dateThu, 03 Mar 2016 13:08:21 +0000
reviewerskmag, mak
bugs1251269
milestone47.0a1
Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak MozReview-Commit-ID: 7nYCplcQZuk
browser/components/extensions/ext-bookmarks.js
browser/components/extensions/schemas/bookmarks.json
toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_getRecent.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -110,16 +110,20 @@ extensions.registerSchemaAPI("bookmarks"
       getSubTree: function(id) {
         return getTree(id, false);
       },
 
       search: function(query) {
         return Bookmarks.search(query).then(result => result.map(convert));
       },
 
+      getRecent: function(numberOfItems) {
+        return Bookmarks.getRecent(numberOfItems).then(result => result.map(convert));
+      },
+
       create: function(bookmark) {
         let info = {
           title: bookmark.title || "",
         };
 
         // If url is NULL or missing, it will be a folder.
         if (bookmark.url !== null) {
           info.type = Bookmarks.TYPE_BOOKMARK;
--- a/browser/components/extensions/schemas/bookmarks.json
+++ b/browser/components/extensions/schemas/bookmarks.json
@@ -160,17 +160,16 @@
                 "items": { "$ref": "BookmarkTreeNode"}
               }
             ]
           }
         ]
       },
       {
         "name": "getRecent",
-        "unsupported": true,
         "type": "function",
         "description": "Retrieves the recently added bookmarks.",
         "async": "callback",
         "parameters": [
           {
             "type": "integer",
             "minimum": 1,
             "name": "numberOfItems",
--- a/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
@@ -302,17 +302,44 @@ function backgroundScript() {
       "Expected number of results returned for non-matching title and query fields"
     );
 
     // returns an empty array on item not found
     return browser.bookmarks.search("microsoft");
   }).then(results => {
     browser.test.assertEq(0, results.length, "Expected number of results returned for non-matching search");
 
-    browser.test.notifyPass("bookmarks");
+    return Promise.resolve().then(() => {
+      return browser.bookmarks.getRecent("");
+    }).then(expectedError, error => {
+      browser.test.assertTrue(
+        error.message.includes("Incorrect argument types for bookmarks.getRecent"),
+        "Expected error thrown when calling getRecent with an empty string"
+      );
+    });
+  }).then(() => {
+    return Promise.resolve().then(() => {
+      return browser.bookmarks.getRecent(1.234);
+    }).then(expectedError, error => {
+      browser.test.assertTrue(
+        error.message.includes("Incorrect argument types for bookmarks.getRecent"),
+        "Expected error thrown when calling getRecent with a decimal number"
+      );
+    });
+  }).then(() => {
+    return browser.bookmarks.getRecent(5);
+  }).then(results => {
+    browser.test.assertEq(5, results.length, "Expected number of results returned by getRecent");
+    browser.test.assertEq("Firefox", results[0].title, "Bookmark has the expected title");
+    browser.test.assertEq("Mozilla Corporation", results[1].title, "Bookmark has the expected title");
+    browser.test.assertEq("Mozilla", results[2].title, "Bookmark has the expected title");
+    browser.test.assertEq("Toolbar Item", results[3].title, "Bookmark has the expected title");
+    browser.test.assertEq("Menu Item", results[4].title, "Bookmark has the expected title");
+
+    return browser.test.notifyPass("bookmarks");
   }).catch(error => {
     browser.test.fail(`Error: ${String(error)} :: ${error.stack}`);
   });
 }
 
 let extensionData = {
   background: `(${backgroundScript})()`,
   manifest: {
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -496,16 +496,42 @@ var Bookmarks = Object.freeze({
     return Task.spawn(function* () {
       let results = yield queryBookmarks(query);
 
       return results;
     });
   },
 
   /**
+   * Returns a list of recently bookmarked items.
+   *
+   * @param {integer} numberOfItems
+   *        The maximum number of bookmark items to return.
+   *
+   * @return {Promise} resolved when the listing is complete.
+   * @resolves to an array of recent bookmark-items.
+   * @rejects if an error happens while querying.
+   */
+  getRecent(numberOfItems) {
+    if (numberOfItems === undefined) {
+      throw new Error("numberOfItems argument is required");
+    }
+    if (!typeof numberOfItems === 'number' || (numberOfItems % 1) !== 0) {
+      throw new Error("numberOfItems argument must be an integer");
+    }
+    if (numberOfItems <= 0) {
+      throw new Error("numberOfItems argument must be greater than zero");
+    }
+
+    return Task.spawn(function* () {
+      return yield fetchRecentBookmarks(numberOfItems);
+    });
+  },
+
+  /**
    * Fetches information about a bookmark-item.
    *
    * REMARK: any successful call to this method resolves to a single
    *         bookmark-item (or null), even when multiple bookmarks may exist
    *         (e.g. fetching by url).  If you wish to retrieve all of the
    *         bookmarks for a given match, use the callback instead.
    *
    * Input can be either a guid or an object with one, and only one, of these
@@ -993,16 +1019,36 @@ function fetchBookmarksByURL(info) {
        ORDER BY b.lastModified DESC
       `, { url: info.url.href,
            tags_folder: PlacesUtils.tagsFolderId });
 
     return rows.length ? rowsToItemsArray(rows) : null;
   }));
 }
 
+function fetchRecentBookmarks(numberOfItems) {
+  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchRecentBookmarks",
+    Task.async(function*(db) {
+
+    let rows = yield db.executeCached(
+      `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
+              b.dateAdded, b.lastModified, b.type, b.title, h.url AS url,
+              NULL AS _id, NULL AS _parentId, NULL AS _childCount, NULL AS _grandParentId
+       FROM moz_bookmarks b
+       LEFT JOIN moz_bookmarks p ON p.id = b.parent
+       LEFT JOIN moz_places h ON h.id = b.fk
+       WHERE p.parent <> :tags_folder
+       ORDER BY b.dateAdded DESC, b.ROWID DESC
+       LIMIT :numberOfItems
+      `, { tags_folder: PlacesUtils.tagsFolderId, numberOfItems });
+
+    return rows.length ? rowsToItemsArray(rows) : [];
+  }));
+}
+
 function fetchBookmarksByParent(info) {
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: fetchBookmarksByParent",
     Task.async(function*(db) {
 
     let rows = yield db.executeCached(
       `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
               b.dateAdded, b.lastModified, b.type, b.title, h.url AS url,
               b.id AS _id, b.parent AS _parentId,
copy from toolkit/components/places/tests/bookmarks/test_bookmarks_search.js
copy to toolkit/components/places/tests/bookmarks/test_bookmarks_getRecent.js
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_search.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_getRecent.js
@@ -1,223 +1,44 @@
 add_task(function* invalid_input_throws() {
-  Assert.throws(() => PlacesUtils.bookmarks.search(),
-                /Query object is required/);
-  Assert.throws(() => PlacesUtils.bookmarks.search(null),
-                /Query object is required/);
-  Assert.throws(() => PlacesUtils.bookmarks.search({title: 50}),
-                /Title option must be a string/);
-  Assert.throws(() => PlacesUtils.bookmarks.search({url: {url: "wombat"}}),
-                /Url option must be a string or a URL object/);
-  Assert.throws(() => PlacesUtils.bookmarks.search(50),
-                /Query must be an object or a string/);
-  Assert.throws(() => PlacesUtils.bookmarks.search(true),
-                /Query must be an object or a string/);
-});
-
-add_task(function* search_bookmark() {
-  yield PlacesUtils.bookmarks.eraseEverything();
-
-  let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.com/",
-                                                 title: "a bookmark" });
-  let bm2 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.org/",
-                                                 title: "another bookmark" });
-  let bm3 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.menuGuid,
-                                                 url: "http://menu.org/",
-                                                 title: "a menu bookmark" });
-  let bm4 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.toolbarGuid,
-                                                 url: "http://toolbar.org/",
-                                                 title: "a toolbar bookmark" });
-  checkBookmarkObject(bm1);
-  checkBookmarkObject(bm2);
-  checkBookmarkObject(bm3);
-  checkBookmarkObject(bm4);
-
-  // finds a result by query
-  let results = yield PlacesUtils.bookmarks.search("example.com");
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm1, results[0]);
-
-  // finds multiple results
-  results = yield PlacesUtils.bookmarks.search("example");
-  Assert.equal(results.length, 2);
-  checkBookmarkObject(results[0]);
-  checkBookmarkObject(results[1]);
-
-  // finds menu bookmarks
-  results = yield PlacesUtils.bookmarks.search("a menu bookmark");
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm3, results[0]);
-
-  // finds toolbar bookmarks
-  results = yield PlacesUtils.bookmarks.search("a toolbar bookmark");
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm4, results[0]);
-
-  yield PlacesUtils.bookmarks.eraseEverything();
+  Assert.throws(() => PlacesUtils.bookmarks.getRecent(),
+                /numberOfItems argument is required/);
+  Assert.throws(() => PlacesUtils.bookmarks.getRecent("abc"),
+                /numberOfItems argument must be an integer/);
+  Assert.throws(() => PlacesUtils.bookmarks.getRecent(1.2),
+                /numberOfItems argument must be an integer/);
+  Assert.throws(() => PlacesUtils.bookmarks.getRecent(0),
+                /numberOfItems argument must be greater than zero/);
+  Assert.throws(() => PlacesUtils.bookmarks.getRecent(-1),
+                /numberOfItems argument must be greater than zero/);
 });
 
-add_task(function* search_bookmark_by_query_object() {
-  let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.com/",
-                                                 title: "a bookmark" });
-  let bm2 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.org/",
-                                                 title: "another bookmark" });
-  checkBookmarkObject(bm1);
-  checkBookmarkObject(bm2);
-
-  let results = yield PlacesUtils.bookmarks.search({query: "example.com"});
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-
-  Assert.deepEqual(bm1, results[0]);
-
-  results = yield PlacesUtils.bookmarks.search({query: "example"});
-  Assert.equal(results.length, 2);
-  checkBookmarkObject(results[0]);
-  checkBookmarkObject(results[1]);
-
+add_task(function* getRecent_returns_recent_bookmarks() {
   yield PlacesUtils.bookmarks.eraseEverything();
-});
 
-add_task(function* search_bookmark_by_url() {
-  let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.com/",
-                                                 title: "a bookmark" });
-  let bm2 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.org/path",
-                                                 title: "another bookmark" });
-  let bm3 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.org/path",
-                                                 title: "third bookmark" });
-  checkBookmarkObject(bm1);
-  checkBookmarkObject(bm2);
-  checkBookmarkObject(bm3);
-
-  // finds the correct result by url
-  let results = yield PlacesUtils.bookmarks.search({url: "http://example.com/"});
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm1, results[0]);
-
-  // normalizes the url
-  results = yield PlacesUtils.bookmarks.search({url: "http:/example.com"});
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm1, results[0]);
-
-  // returns multiple matches
-  results = yield PlacesUtils.bookmarks.search({url: "http://example.org/path"});
-  Assert.equal(results.length, 2);
-
-  // requires exact match
-  results = yield PlacesUtils.bookmarks.search({url: "http://example.org/"});
-  Assert.equal(results.length, 0);
-
-  yield PlacesUtils.bookmarks.eraseEverything();
-});
-
-add_task(function* search_bookmark_by_title() {
   let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  url: "http://example.com/",
                                                  title: "a bookmark" });
   let bm2 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  url: "http://example.org/path",
                                                  title: "another bookmark" });
   let bm3 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  url: "http://example.net/",
                                                  title: "another bookmark" });
-  checkBookmarkObject(bm1);
-  checkBookmarkObject(bm2);
-  checkBookmarkObject(bm3);
-
-  // finds the correct result by title
-  let results = yield PlacesUtils.bookmarks.search({title: "a bookmark"});
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm1, results[0]);
-
-  // returns multiple matches
-  results = yield PlacesUtils.bookmarks.search({title: "another bookmark"});
-  Assert.equal(results.length, 2);
-
-  // requires exact match
-  results = yield PlacesUtils.bookmarks.search({title: "bookmark"});
-  Assert.equal(results.length, 0);
-
-  yield PlacesUtils.bookmarks.eraseEverything();
-});
-
-add_task(function* search_bookmark_combinations() {
-  let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.com/",
-                                                 title: "a bookmark" });
-  let bm2 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.org/path",
-                                                 title: "another bookmark" });
-  let bm3 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
-                                                 url: "http://example.net/",
-                                                 title: "third bookmark" });
+  let bm4 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                 url: "http://example.net/path",
+                                                 title: "yet another bookmark" });
   checkBookmarkObject(bm1);
   checkBookmarkObject(bm2);
   checkBookmarkObject(bm3);
-
-  // finds the correct result if title and url match
-  let results = yield PlacesUtils.bookmarks.search({url: "http://example.com/", title: "a bookmark"});
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm1, results[0]);
-
-  // does not match if query is not matching but url and title match
-  results = yield PlacesUtils.bookmarks.search({url: "http://example.com/", title: "a bookmark", query: "nonexistent"});
-  Assert.equal(results.length, 0);
+  checkBookmarkObject(bm4);
 
-  // does not match if one parameter is not matching
-  results = yield PlacesUtils.bookmarks.search({url: "http://what.ever", title: "a bookmark"});
-  Assert.equal(results.length, 0);
-
-  // query only matches if other fields match as well
-  results = yield PlacesUtils.bookmarks.search({query: "bookmark", url: "http://example.net/"});
-  Assert.equal(results.length, 1);
+  let results = yield PlacesUtils.bookmarks.getRecent(3);
+  Assert.equal(results.length, 3);
   checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm3, results[0]);
-
-  // non-matching query will also return no results
-  results = yield PlacesUtils.bookmarks.search({query: "nonexistent", url: "http://example.net/"});
-  Assert.equal(results.length, 0);
+  Assert.deepEqual(bm4, results[0]);
+  checkBookmarkObject(results[1]);
+  Assert.deepEqual(bm3, results[1]);
+  checkBookmarkObject(results[2]);
+  Assert.deepEqual(bm2, results[2]);
 
   yield PlacesUtils.bookmarks.eraseEverything();
 });
-
-add_task(function* search_folder() {
-  let folder = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.menuGuid,
-                                                 type: PlacesUtils.bookmarks.TYPE_FOLDER,
-                                                 title: "a test folder" });
-  let bm = yield PlacesUtils.bookmarks.insert({ parentGuid: folder.guid,
-                                                 url: "http://example.com/",
-                                                 title: "a bookmark" });
-  checkBookmarkObject(folder);
-  checkBookmarkObject(bm);
-
-  // also finds folders
-  let results = yield PlacesUtils.bookmarks.search("a test folder");
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.equal(folder.title, results[0].title);
-  Assert.equal(folder.type, results[0].type);
-  Assert.equal(folder.parentGuid, results[0].parentGuid);
-
-  // finds elements in folders
-  results = yield PlacesUtils.bookmarks.search("example.com");
-  Assert.equal(results.length, 1);
-  checkBookmarkObject(results[0]);
-  Assert.deepEqual(bm, results[0]);
-  Assert.equal(folder.guid, results[0].parentGuid);
-
-  yield PlacesUtils.bookmarks.eraseEverything();
-});
-
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -28,16 +28,17 @@ skip-if = toolkit == 'android' || toolki
 [test_997030-bookmarks-html-encode.js]
 [test_1129529.js]
 [test_async_observers.js]
 [test_bmindex.js]
 [test_bookmarkstree_cache.js]
 [test_bookmarks.js]
 [test_bookmarks_eraseEverything.js]
 [test_bookmarks_fetch.js]
+[test_bookmarks_getRecent.js]
 [test_bookmarks_insert.js]
 [test_bookmarks_notifications.js]
 [test_bookmarks_remove.js]
 [test_bookmarks_reorder.js]
 [test_bookmarks_search.js]
 [test_bookmarks_update.js]
 [test_changeBookmarkURI.js]
 [test_getBookmarkedURIFor.js]