Bug 1388863 - Bookmarking a link via Activity Stream context menu should give feedback r=adw

MozReview-Commit-ID: Bd2eed1rX91

--HG--
extra : rebase_source : 5a2cc568e78c04173d463b84647453d2ebdfedef

browser/base/content/browser-places.js

@@ -241,7 +241,7 @@ var StarUI = {
 
   _overlayLoaded: false,
   _overlayLoading: false,
-  async showEditBookmarkPopup(aNode, aAnchorElement, aPosition, aIsNewBookmark) {
+  async showEditBookmarkPopup(aNode, aAnchorElement, aPosition, aIsNewBookmark, aUrl) {
     // Slow double-clicks (not true double-clicks) shouldn't
     // cause the panel to flicker.
     if (this.panel.state == "showing" ||
@@ -266,7 +266,7 @@ var StarUI = {
       return;
 
     if (this._overlayLoaded) {
-      await this._doShowEditBookmarkPanel(aNode, aAnchorElement, aPosition);
+      await this._doShowEditBookmarkPanel(aNode, aAnchorElement, aPosition, aUrl);
       return;
     }
 
@@ -283,12 +283,12 @@ var StarUI = {
 
         this._overlayLoading = false;
         this._overlayLoaded = true;
-        this._doShowEditBookmarkPanel(aNode, aAnchorElement, aPosition);
+        this._doShowEditBookmarkPanel(aNode, aAnchorElement, aPosition, aUrl);
       }
     );
   },
 
-  async _doShowEditBookmarkPanel(aNode, aAnchorElement, aPosition) {
+  async _doShowEditBookmarkPanel(aNode, aAnchorElement, aPosition, aUrl) {
     if (this.panel.state != "closed")
       return;
 
@@ -308,7 +308,7 @@ var StarUI = {
     // multiple times.
     this._itemGuids = [];
 
-    await PlacesUtils.bookmarks.fetch({url: gBrowser.currentURI},
+    await PlacesUtils.bookmarks.fetch({url: aUrl},
       bookmark => this._itemGuids.push(bookmark.guid));
 
     if (!PlacesUIUtils.useAsyncTransactions) {
@@ -441,14 +441,21 @@ var PlacesCommandHook = {
    *        aBrowser isn't bookmarked yet, defaults to the unfiled root.
    * @param [optional] aShowEditUI
    *        whether or not to show the edit-bookmark UI for the bookmark item
+   * @param [optional] aUrl
+   *        Option to provide a URL to bookmark rather than the current page
+   * @param [optional] aTitle
+   *        Option to provide a title for a bookmark to use rather than the
+   *        getting the current page's title
    */
-  async bookmarkPage(aBrowser, aParent, aShowEditUI) {
+  async bookmarkPage(aBrowser, aParent, aShowEditUI, aUrl = null, aTitle = null) {
     if (PlacesUIUtils.useAsyncTransactions) {
-      await this._bookmarkPagePT(aBrowser, aParent, aShowEditUI);
+      await this._bookmarkPagePT(aBrowser, aParent, aShowEditUI, aUrl, aTitle);
       return;
     }
 
-    var uri = aBrowser.currentURI;
+    // If aUrl is provided, we want to bookmark that url rather than the
+    // the current page
+    var uri = aUrl ? Services.io.newURI(aUrl) : aBrowser.currentURI;
     var itemId = PlacesUtils.getMostRecentBookmarkForURI(uri);
     let isNewBookmark = itemId == -1;
     if (isNewBookmark) {
@@ -457,14 +464,15 @@ var PlacesCommandHook = {
       var description;
       var charset;
 
-      let docInfo = await this._getPageDetails(aBrowser);
+      let docInfo = aUrl ? {} : await this._getPageDetails(aBrowser);
 
       try {
-        title = docInfo.isErrorPage ? PlacesUtils.history.getPageTitle(uri)
-                                    : aBrowser.contentTitle;
-        title = title || uri.displaySpec;
+        title = aTitle ||
+                (docInfo.isErrorPage ? PlacesUtils.history.getPageTitle(uri)
+                                     : aBrowser.contentTitle) ||
+                uri.displaySpec;
         description = docInfo.description;
-        charset = aBrowser.characterSet;
+        charset = aUrl ? null : aBrowser.characterSet;
       } catch (e) { }
 
       if (aShowEditUI) {
@@ -500,23 +508,25 @@ var PlacesCommandHook = {
     // 3. the content area
     if (BookmarkingUI.anchor && isVisible(BookmarkingUI.anchor)) {
       await StarUI.showEditBookmarkPopup(itemId, BookmarkingUI.anchor,
-                                        "bottomcenter topright", isNewBookmark);
+                                        "bottomcenter topright", isNewBookmark, uri);
       return;
     }
 
     let identityIcon = document.getElementById("identity-icon");
     if (isVisible(identityIcon)) {
       await StarUI.showEditBookmarkPopup(itemId, identityIcon,
-                                        "bottomcenter topright", isNewBookmark);
+                                        "bottomcenter topright", isNewBookmark, uri);
     } else {
-      await StarUI.showEditBookmarkPopup(itemId, aBrowser, "overlap", isNewBookmark);
+      await StarUI.showEditBookmarkPopup(itemId, aBrowser, "overlap", isNewBookmark, uri);
     }
   },
 
   // TODO: Replace bookmarkPage code with this function once legacy
   // transactions are removed.
-  async _bookmarkPagePT(aBrowser, aParentId, aShowEditUI) {
-    let url = new URL(aBrowser.currentURI.spec);
+  async _bookmarkPagePT(aBrowser, aParentId, aShowEditUI, aUrl, aTitle) {
+    // If aUrl is provided, we want to bookmark that url rather than the
+    // the current page
+    let url = aUrl ? new URL(aUrl) : new URL(aBrowser.currentURI.spec);
     let info = await PlacesUtils.bookmarks.fetch({ url });
     let isNewBookmark = !info;
     if (!info) {
@@ -528,7 +538,7 @@ var PlacesCommandHook = {
       let description = null;
       let charset = null;
 
-      let docInfo = await this._getPageDetails(aBrowser);
+      let docInfo = aUrl ? {} : await this._getPageDetails(aBrowser);
 
       try {
         if (docInfo.isErrorPage) {
@@ -537,11 +547,11 @@ var PlacesCommandHook = {
             info.title = entry.title;
           }
         } else {
-          info.title = aBrowser.contentTitle;
+          info.title = aTitle || aBrowser.contentTitle;
         }
         info.title = info.title || url.href;
         description = docInfo.description;
-        charset = aBrowser.characterSet;
+        charset = aUrl ? null : aBrowser.characterSet;
       } catch (e) {
         Components.utils.reportError(e);
       }
@@ -580,16 +590,16 @@ var PlacesCommandHook = {
     // 3. the content area
     if (BookmarkingUI.anchor && isVisible(BookmarkingUI.anchor)) {
       await StarUI.showEditBookmarkPopup(node, BookmarkingUI.anchor,
-                                   "bottomcenter topright", isNewBookmark);
+                                   "bottomcenter topright", isNewBookmark, url);
       return;
     }
 
     let identityIcon = document.getElementById("identity-icon");
     if (isVisible(identityIcon)) {
       await StarUI.showEditBookmarkPopup(node, identityIcon,
-                                   "bottomcenter topright", isNewBookmark);
+                                   "bottomcenter topright", isNewBookmark, url);
     } else {
-      await StarUI.showEditBookmarkPopup(node, aBrowser, "overlap", isNewBookmark);
+      await StarUI.showEditBookmarkPopup(node, aBrowser, "overlap", isNewBookmark, url);
     }
   },
 

browser/base/content/test/general/browser_bookmark_titles.js

@@ -19,7 +19,7 @@ var tests = [
    "https://untrusted.example.com/somepage.html"]
 ];
 
-add_task(async function() {
+add_task(async function check_default_bookmark_title() {
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
   let browser = tab.linkedBrowser;
 
@@ -70,6 +70,47 @@ add_task(async function() {
   await BrowserTestUtils.removeTab(tab);
 });
 
+add_task(async function check_override_bookmark_title() {
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
+  let browser = tab.linkedBrowser;
+  let [url, default_title] = tests[1];
+
+  // We use promisePageLoaded rather than BrowserTestUtils.browserLoaded - see
+  // note on function definition below.
+  let promiseLoaded = promisePageLoaded(browser);
+  BrowserTestUtils.loadURI(browser, url);
+
+  await promiseLoaded;
+
+  // Test that a bookmark of this URI gets the correct title if we provide one
+  await checkBookmarkedPageTitle(url, default_title, "An overridden title");
+
+  await BrowserTestUtils.removeTab(tab);
+});
+
+// Bookmark a page and confirm that the new bookmark has the expected title.
+// (Then delete the bookmark.)
+async function checkBookmarkedPageTitle(url, default_title, overridden_title) {
+  let promiseBookmark = PlacesTestUtils.waitForNotification("onItemAdded",
+    (id, parentId, index, type, itemUrl) => itemUrl.equals(Services.io.newURI(url)));
+
+  // Here we test that if we provide a url and a title to bookmark, it will use the
+  // title provided rather than the one provided by the current page
+  PlacesCommandHook.bookmarkPage(gBrowser.selectedBrowser, undefined, false, url, overridden_title);
+  await promiseBookmark;
+
+  let bookmark = await PlacesUtils.bookmarks.fetch({url});
+
+  Assert.ok(bookmark, "Found the expected bookmark");
+  Assert.equal(bookmark.title, overridden_title, "Bookmark got a good overridden title.");
+  Assert.equal(default_title, gBrowser.selectedBrowser.contentTitle,
+    "Sanity check that the content is providing us with the correct title");
+  Assert.notEqual(bookmark.title, default_title,
+    "Make sure we picked the overridden one and not the default one.");
+
+  await PlacesUtils.bookmarks.remove(bookmark);
+}
+
 // Bookmark the current page and confirm that the new bookmark has the expected
 // title. (Then delete the bookmark.)
 async function checkBookmark(url, expected_title) {

browser/extensions/activity-stream/data/content/activity-stream.bundle.js

@@ -2582,7 +2582,7 @@ module.exports = {
     icon: "bookmark",
     action: ac.SendToMain({
       type: at.BOOKMARK_URL,
-      data: site.url
+      data: {url: site.url, title: site.title}
     }),
     userEvent: "BOOKMARK_ADD"
   }),
@@ -2732,4 +2732,4 @@ ReactDOM.render(React.createElement(
 addSnippetsSubscriber(store);
 
 /***/ })
-/******/ ]);
+/******/ ]);

browser/extensions/activity-stream/lib/PlacesFeed.jsm

@@ -207,7 +207,7 @@ class PlacesFeed {
         NewTabUtils.activityStreamLinks.blockURL({url: action.data});
         break;
       case at.BOOKMARK_URL:
-        NewTabUtils.activityStreamLinks.addBookmark(action.data);
+        NewTabUtils.activityStreamLinks.addBookmark(action.data, action._target.browser);
         break;
       case at.DELETE_BOOKMARK_BY_ID:
         NewTabUtils.activityStreamLinks.deleteBookmark(action.data);

toolkit/modules/NewTabUtils.jsm

@@ -1130,18 +1130,25 @@ var ActivityStreamLinks = {
   },
 
   /**
-   * Adds a bookmark
+   * Adds a bookmark and opens up the Bookmark Dialog to show feedback that
+   * the bookmarking action has been successful
    *
-   * @param {String} aUrl
-   *          The url to bookmark
+   * @param {Object} aData
+   *          aData.url The url to bookmark
+   *          aData.title The title of the page to bookmark
+   * @param {Browser} aBrowser
+   *          a <browser> element
    *
    * @returns {Promise} Returns a promise set to an object representing the bookmark
    */
-  addBookmark(aUrl) {
-    return PlacesUtils.bookmarks.insert({
-      url: aUrl,
-      parentGuid: PlacesUtils.bookmarks.unfiledGuid
-    });
+  addBookmark(aData, aBrowser) {
+      const {url, title} = aData;
+      return aBrowser.ownerGlobal.PlacesCommandHook.bookmarkPage(
+              aBrowser,
+              undefined,
+              true,
+              url,
+              title);
   },
 
   /**

toolkit/modules/tests/xpcshell/test_NewTabUtils.js

@@ -555,38 +555,6 @@ add_task(async function activitySteamProvider_deleteHistoryLink() {
   Assert.equal(size, 1, "expected history size");
 });
 
-add_task(async function activityStream_addBookmark() {
-  await setUpActivityStreamTest();
-
-  let provider = NewTabUtils.activityStreamLinks;
-  let bookmarks = [
-    "https://mozilla1.com/0",
-    "https://mozilla1.com/1"
-  ];
-
-  let bookmarksSize = await NewTabUtils.activityStreamProvider.getBookmarksSize();
-  Assert.equal(bookmarksSize, 0, "empty bookmarks yields 0 size");
-
-  for (let url of bookmarks) {
-    await provider.addBookmark(url);
-  }
-  bookmarksSize = await NewTabUtils.activityStreamProvider.getBookmarksSize();
-  Assert.equal(bookmarksSize, 2, "size 2 for 2 bookmarks added");
-});
-
-add_task(async function activityStream_getBookmark() {
-    await setUpActivityStreamTest();
-
-    let provider = NewTabUtils.activityStreamLinks;
-    let bookmark = await provider.addBookmark("https://mozilla1.com/0");
-
-    let result = await NewTabUtils.activityStreamProvider.getBookmark(bookmark.guid);
-    Assert.equal(result.bookmarkGuid, bookmark.guid, "got the correct bookmark guid");
-    Assert.equal(result.bookmarkTitle, bookmark.title, "got the correct bookmark title");
-    Assert.equal(result.lastModified, bookmark.lastModified.getTime(), "got the correct bookmark time");
-    Assert.equal(result.url, bookmark.url.href, "got the correct bookmark url");
-});
-
 add_task(async function activityStream_deleteBookmark() {
   await setUpActivityStreamTest();