Bug 1356440 - Favicons of bookmarks views don't update on visit. r=mrbkap,past,enndeakin

MozReview-Commit-ID: 8j5yLqr7MTc

--HG--
extra : rebase_source : 0da3b4bf0fca0462e22f76c1392f1d9e69f0e71c
extra : amend_source : d6c92db62af2b62671cf13f0b5385b2bc2c8b81e

browser/components/places/content/browserPlacesViews.js

@@ -511,6 +511,8 @@ PlacesViewBase.prototype = {
     if (elt.localName == "menupopup") {
       elt = elt.parentNode;
     }
+    // We must remove and reset the attribute to force an update.
+    elt.removeAttribute("image");
     elt.setAttribute("image", aPlacesNode.icon);
   },
 

browser/components/places/content/treeView.js

@@ -781,8 +781,11 @@ PlacesTreeView.prototype = {
       return;
 
     let column = this._findColumnByType(aColumnType);
-    if (column && !column.element.hidden)
+    if (column && !column.element.hidden) {
+      if (aColumnType == this.COLUMN_TYPE_TITLE)
+        this._tree.removeImageCacheEntry(row, column);
       this._tree.invalidateCell(row, column);
+    }
 
     // Last modified time is altered for almost all node changes.
     if (aColumnType != this.COLUMN_TYPE_LASTMODIFIED) {

browser/components/places/tests/browser/browser.ini

@@ -53,6 +53,9 @@ support-files =
 skip-if = true # temporarily disabled for breaking the treeview - bug 658744
 [browser_sort_in_library.js]
 [browser_toolbarbutton_menu_context.js]
+[browser_views_iconsupdate.js]
+support-files =
+  favicon-normal16.png
 [browser_views_liveupdate.js]
 [browser_bookmark_all_tabs.js]
 support-files =

browser/components/places/tests/browser/browser_views_iconsupdate.js

@@ -0,0 +1,119 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests Places views (toolbar, tree) for icons update.
+ * The menu is not tested since it uses the same code as the toolbar.
+ */
+
+add_task(function* () {
+  const PAGE_URI = NetUtil.newURI("http://places.test/");
+  const ICON_URI = NetUtil.newURI("http://mochi.test:8888/browser/browser/components/places/tests/browser/favicon-normal16.png");
+
+  info("Uncollapse the personal toolbar if needed");
+  let toolbar = document.getElementById("PersonalToolbar");
+  let wasCollapsed = toolbar.collapsed;
+  if (wasCollapsed) {
+    yield promiseSetToolbarVisibility(toolbar, true);
+    registerCleanupFunction(function* () {
+      yield promiseSetToolbarVisibility(toolbar, false);
+    });
+  }
+
+  info("Open the bookmarks sidebar");
+  let sidebar = document.getElementById("sidebar");
+  let promiseSidebarLoaded = new Promise(resolve => {
+    sidebar.addEventListener("load", resolve, {capture: true, once: true});
+  });
+  SidebarUI.show("viewBookmarksSidebar");
+  registerCleanupFunction(() => {
+    SidebarUI.hide();
+  });
+  yield promiseSidebarLoaded;
+
+  // Add a bookmark to the bookmarks toolbar.
+  let bm = yield PlacesUtils.bookmarks.insert({
+    url: PAGE_URI,
+    title: "test icon",
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid
+  });
+  registerCleanupFunction(function* () {
+    yield PlacesUtils.bookmarks.remove(bm);
+  });
+
+  // The icon is read asynchronously from the network, we don't have an easy way
+  // to wait for that.
+  yield new Promise(resolve => {
+    setTimeout(resolve, 3000);
+  });
+
+  let toolbarElt = getNodeForToolbarItem(bm.guid);
+  let toolbarShot1 = TestUtils.screenshotArea(toolbarElt, window);
+  let sidebarRect = yield getRectForSidebarItem(bm.guid);
+  let sidebarShot1 = TestUtils.screenshotArea(sidebarRect, window);
+
+  yield new Promise(resolve => {
+    PlacesUtils.favicons.setAndFetchFaviconForPage(
+      PAGE_URI, ICON_URI, true,
+      PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
+      resolve,
+      Services.scriptSecurityManager.getSystemPrincipal()
+    );
+  });
+
+  // The icon is read asynchronously from the network, we don't have an easy way
+  // to wait for that.
+  yield new Promise(resolve => {
+    setTimeout(resolve, 3000);
+  });
+
+  // Assert.notEqual truncates the strings, so it is unusable here for failure
+  // debugging purposes.
+  let toolbarShot2 = TestUtils.screenshotArea(toolbarElt, window);
+  if (toolbarShot1 != toolbarShot2) {
+    info("Before toolbar: " + toolbarShot1);
+    info("After toolbar: " + toolbarShot2);
+  }
+  Assert.notEqual(toolbarShot1, toolbarShot2, "The UI should have updated");
+
+  let sidebarShot2 = TestUtils.screenshotArea(sidebarRect, window);
+  if (sidebarShot1 != sidebarShot2) {
+    info("Before sidebar: " + sidebarShot1);
+    info("After sidebar: " + sidebarShot2);
+  }
+  Assert.notEqual(sidebarShot1, sidebarShot2, "The UI should have updated");
+});
+
+/**
+ * Get Element for a bookmark in the bookmarks toolbar.
+ *
+ * @param guid
+ *        GUID of the item to search.
+ * @returns DOM Node of the element.
+ */
+function getNodeForToolbarItem(guid) {
+  return Array.from(document.getElementById("PlacesToolbarItems").childNodes)
+              .find(child => child._placesNode && child._placesNode.bookmarkGuid == guid);
+}
+
+/**
+ * Get a rect for a bookmark in the bookmarks sidebar
+ *
+ * @param guid
+ *        GUID of the item to search.
+ * @returns DOM Node of the element.
+ */
+function* getRectForSidebarItem(guid) {
+  let itemId = yield PlacesUtils.promiseItemId(guid);
+  let sidebar = document.getElementById("sidebar");
+  let tree = sidebar.contentDocument.getElementById("bookmarks-view");
+  tree.selectItems([itemId]);
+  let rect = {};
+  [rect.left, rect.top, rect.width, rect.height] = tree.treeBoxObject
+                                                       .selectionRegion
+                                                       .getRects();
+  // Adjust the position for the sidebar.
+  rect.left += sidebar.getBoundingClientRect().left;
+  rect.top += sidebar.getBoundingClientRect().top;
+  return rect;
+}

browser/components/places/tests/browser/head.js

@@ -4,6 +4,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "Promise",
   "resource://gre/modules/Promise.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
   "resource://testing-common/PlacesTestUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "TestUtils",
+  "resource://testing-common/TestUtils.jsm");
 
 // Imported via PlacesOverlay.xul
 /* global doGetPlacesControllerForCommand:false, PlacesControllerDragHelper:false,

dom/webidl/TreeBoxObject.webidl

@@ -204,4 +204,10 @@ interface TreeBoxObject : BoxObject {
    * Called on a theme switch to flush out the tree's style and image caches.
    */
   void clearStyleAndImageCaches();
+
+  /**
+   * Remove an image source from the image cache to allow its invalidation.
+   */
+  [Throws]
+  void removeImageCacheEntry(long row, TreeColumn col);
 };

layout/xul/tree/TreeBoxObject.cpp

@@ -667,6 +667,24 @@ TreeBoxObject::ClearStyleAndImageCaches()
   return NS_OK;
 }
 
+NS_IMETHODIMP
+TreeBoxObject::RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol)
+{
+  NS_ENSURE_ARG(aCol);
+  NS_ENSURE_TRUE(aRowIndex >= 0, NS_ERROR_INVALID_ARG);
+  nsTreeBodyFrame* body = GetTreeBodyFrame();
+  if (body) {
+    return body->RemoveImageCacheEntry(aRowIndex, aCol);
+  }
+  return NS_OK;
+}
+
+void
+TreeBoxObject::RemoveImageCacheEntry(int32_t row, nsITreeColumn& col, ErrorResult& aRv)
+{
+  aRv = RemoveImageCacheEntry(row, &col);
+}
+
 void
 TreeBoxObject::ClearCachedValues()
 {

layout/xul/tree/TreeBoxObject.h

@@ -75,6 +75,8 @@ public:
 
   bool IsCellCropped(int32_t row, nsITreeColumn* col, ErrorResult& aRv);
 
+  void RemoveImageCacheEntry(int32_t row, nsITreeColumn& col, ErrorResult& aRv);
+
   // Deprecated APIs from old IDL
   void GetCellAt(JSContext* cx,
                  int32_t x, int32_t y,

layout/xul/tree/nsITreeBoxObject.idl

@@ -48,7 +48,7 @@ interface nsITreeBoxObject : nsISupports
   readonly attribute long rowWidth;
 
   /**
-   * Get the pixel position of the horizontal scrollbar. 
+   * Get the pixel position of the horizontal scrollbar.
    */
   readonly attribute long horizontalPosition;
 
@@ -186,4 +186,12 @@ interface nsITreeBoxObject : nsISupports
    * Called on a theme switch to flush out the tree's style and image caches.
    */
   void clearStyleAndImageCaches();
+
+  /**
+   * Remove an image source from the image cache to allow its invalidation.
+   *
+   * @note This only affects images supplied by the view, not the ones supplied
+   *       through the styling context, like twisties or checkboxes.
+   */
+  void removeImageCacheEntry(in long row, in nsITreeColumn col);
 };

layout/xul/tree/nsTreeBodyFrame.cpp

@@ -1109,7 +1109,7 @@ nsTreeBodyFrame::GetCoordsForCellItem(int32_t aRow, nsITreeColumn* aCol, const n
   bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;
   nscoord currX = mInnerBox.x - mHorzPosition;
 
-  // The Rect for the requested item. 
+  // The Rect for the requested item.
   nsRect theRect;
 
   nsPresContext* presContext = PresContext();
@@ -4529,6 +4529,22 @@ nsTreeBodyFrame::ClearStyleAndImageCaches()
   return NS_OK;
 }
 
+nsresult
+nsTreeBodyFrame::RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol)
+{
+  nsAutoString imageSrc;
+  if (NS_SUCCEEDED(mView->GetImageSrc(aRowIndex, aCol, imageSrc))) {
+    nsTreeImageCacheEntry entry;
+    if (mImageCache.Get(imageSrc, &entry)) {
+      nsLayoutUtils::DeregisterImageRequest(PresContext(), entry.request,
+                                            nullptr);
+      entry.request->CancelAndForgetObserver(NS_BINDING_ABORTED);
+      mImageCache.Remove(imageSrc);
+    }
+  }
+  return NS_OK;
+}
+
 /* virtual */ void
 nsTreeBodyFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
 {

layout/xul/tree/nsTreeBodyFrame.h

@@ -118,6 +118,7 @@ public:
   nsresult BeginUpdateBatch();
   nsresult EndUpdateBatch();
   nsresult ClearStyleAndImageCaches();
+  nsresult RemoveImageCacheEntry(int32_t aRowIndex, nsITreeColumn* aCol);
 
   void CancelImageRequests();
 

testing/modules/TestUtils.jsm

@@ -61,4 +61,27 @@ this.TestUtils = {
       }, topic);
     });
   },
+
+  /**
+   * Takes a screenshot of an area and returns it as a data URL.
+   *
+   * @param eltOrRect
+   *        The DOM node or rect ({left, top, width, height}) to screenshot.
+   * @param win
+   *        The current window.
+   */
+  screenshotArea(eltOrRect, win) {
+    if (eltOrRect instanceof Ci.nsIDOMElement) {
+      eltOrRect = eltOrRect.getBoundingClientRect();
+    }
+    let { left, top, width, height } = eltOrRect;
+    let canvas = win.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
+    let ctx = canvas.getContext("2d");
+    let ratio = win.devicePixelRatio;
+    canvas.width = width * ratio;
+    canvas.height = height * ratio;
+    ctx.scale(ratio, ratio);
+    ctx.drawWindow(win, left, top, width, height, "#fff");
+    return canvas.toDataURL();
+  }
 };