Bug 969780 - Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, r=mak

--HG--
extra : rebase_source : dca1ab1dd901468f9dacaff4b7466c27bef84b47

browser/base/content/browser-customization.js

@@ -41,7 +41,6 @@ let CustomizationHandler = {
     CombinedStopReload.uninit();
     CombinedBackForward.uninit();
     PlacesToolbarHelper.customizeStart();
-    BookmarkingUI.customizeStart();
     DownloadsButton.customizeStart();
 
     // The additional padding on the sides of the browser
@@ -55,7 +54,6 @@ let CustomizationHandler = {
 
   _customizationChange: function() {
     gHomeButton.updatePersonalToolbarStyle();
-    BookmarkingUI.customizeChange();
     PlacesToolbarHelper.customizeChange();
   },
 
@@ -84,7 +82,6 @@ let CustomizationHandler = {
     }
 
     PlacesToolbarHelper.customizeDone();
-    BookmarkingUI.customizeDone();
     DownloadsButton.customizeDone();
 
     // The url bar splitter state is dependent on whether stop/reload

browser/base/content/browser-places.js

@@ -1153,24 +1153,54 @@ let BookmarkingUI = {
       this.button._placesView.uninit();
   },
 
-  customizeStart: function BUI_customizeStart() {
-    this._uninitView();
+  onCustomizeStart: function BUI_customizeStart(aWindow) {
+    if (aWindow == window) {
+      this._uninitView();
+      this._isCustomizing = true;
+    }
   },
 
-  customizeChange: function BUI_customizeChange() {
+  onWidgetAdded: function BUI_widgetAdded(aWidgetId) {
+    if (aWidgetId != "bookmarks-menu-button") {
+      return;
+    }
+
     let usedToUpdateStarState = this._shouldUpdateStarState();
     this._updateCustomizationState();
-    if (usedToUpdateStarState != this._shouldUpdateStarState()) {
+    if (!usedToUpdateStarState && this._shouldUpdateStarState()) {
       this.updateStarState();
+    } else if (usedToUpdateStarState && !this._shouldUpdateStarState()) {
+      this._updateStar();
+    }
+    // If we're moved outside of customize mode, we need to uninit
+    // our view so it gets reconstructed.
+    if (!this._isCustomizing) {
+      this._uninitView();
     }
     this._updateToolbarStyle();
   },
 
-  customizeDone: function BUI_customizeDone() {
-    this.onToolbarVisibilityChange();
+  onWidgetRemoved: function BUI_widgetRemoved(aWidgetId) {
+    if (aWidgetId != "bookmarks-menu-button") {
+      return;
+    }
+    // If we're moved outside of customize mode, we need to uninit
+    // our view so it gets reconstructed.
+    if (!this._isCustomizing) {
+      this._uninitView();
+    }
+    this._updateCustomizationState();
     this._updateToolbarStyle();
   },
 
+  onCustomizeEnd: function BUI_customizeEnd(aWindow) {
+    if (aWindow == window) {
+      this._isCustomizing = false;
+      this.onToolbarVisibilityChange();
+      this._updateToolbarStyle();
+    }
+  },
+
   init: function() {
     CustomizableUI.addListener(this);
     this._updateCustomizationState();
@@ -1194,11 +1224,14 @@ let BookmarkingUI = {
     }
   },
 
-  updateStarState: function BUI_updateStarState() {
+  onLocationChange: function BUI_onLocationChange() {
     if (this._uri && gBrowser.currentURI.equals(this._uri)) {
       return;
     }
+    this.updateStarState();
+  },
 
+  updateStarState: function BUI_updateStarState() {
     // Reset tracked values.
     this._uri = gBrowser.currentURI;
     this._itemIds = [];
@@ -1487,10 +1520,9 @@ let BookmarkingUI = {
     if (aNode.id != "bookmarks-menu-button" || win != window)
       return;
 
-    // If the button hasn't been in the overflow panel before, we may ignore
-    // this event.
-    if (!this._starButtonLabel)
-      return;
+    // The view gets broken by being removed and reinserted. Uninit
+    // here so popupshowing will generate a new one:
+    this._uninitView();
 
     if (aNode.getAttribute("label") != this._starButtonLabel)
       aNode.setAttribute("label", this._starButtonLabel);

browser/base/content/browser.js

@@ -3610,8 +3610,7 @@ var XULBrowserWindow = {
       if (gURLBar) {
         URLBarSetURI(aLocationURI);
 
-        // Update starring UI
-        BookmarkingUI.updateStarState();
+        BookmarkingUI.onLocationChange();
         SocialUI.updateState();
       }
 

browser/components/places/content/browserPlacesViews.js

@@ -721,8 +721,16 @@ PlacesViewBase.prototype = {
 
     if (this._controller) {
       this._controller.terminate();
-      this._viewElt.controllers.removeController(this._controller);
-      this._controller = null;
+      // Removing the controller will fail if it is already no longer there.
+      // This can happen if the view element was removed/reinserted without
+      // our knowledge. There is no way to check for that having happened
+      // without the possibility of an exception. :-(
+      try {
+        this._viewElt.controllers.removeController(this._controller);
+      } catch (ex) {
+      } finally {
+        this._controller = null;
+      }
     }
 
     delete this._viewElt._placesView;

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

@@ -45,3 +45,4 @@ skip-if = true
 [browser_416459_cut.js]
 [browser_library_downloads.js]
 [browser_library_left_pane_select_hierarchy.js]
+[browser_toolbarbutton_menu_context.js]

browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js

@@ -0,0 +1,51 @@
+let bookmarksMenuButton = document.getElementById("bookmarks-menu-button");
+let BMB_menuPopup = document.getElementById("BMB_bookmarksPopup");
+let BMB_showAllBookmarks = document.getElementById("BMB_bookmarksShowAll");
+let contextMenu = document.getElementById("placesContext");
+let newBookmarkItem = document.getElementById("placesContext_new:bookmark");
+
+waitForExplicitFinish();
+add_task(function testPopup() {
+  info("Checking popup context menu before moving the bookmarks button");
+  yield checkPopupContextMenu();
+  let pos = CustomizableUI.getPlacementOfWidget("bookmarks-menu-button").position;
+  CustomizableUI.addWidgetToArea("bookmarks-menu-button", CustomizableUI.AREA_PANEL);
+  CustomizableUI.addWidgetToArea("bookmarks-menu-button", CustomizableUI.AREA_NAVBAR, pos);
+  info("Checking popup context menu after moving the bookmarks button");
+  yield checkPopupContextMenu();
+});
+
+function* checkPopupContextMenu() {
+  let dropmarker = document.getAnonymousElementByAttribute(bookmarksMenuButton, "anonid", "dropmarker");
+  let popupShownPromise = onPopupEvent(BMB_menuPopup, "shown");
+  EventUtils.synthesizeMouseAtCenter(dropmarker, {});
+  info("Waiting for bookmarks menu to be shown.");
+  yield popupShownPromise;
+  let contextMenuShownPromise = onPopupEvent(contextMenu, "shown");
+  EventUtils.synthesizeMouseAtCenter(BMB_showAllBookmarks, {type: "contextmenu", button: 2 });
+  info("Waiting for context menu on bookmarks menu to be shown.");
+  yield contextMenuShownPromise;
+  ok(!newBookmarkItem.hasAttribute("disabled"), "New bookmark item shouldn't be disabled");
+  let contextMenuHiddenPromise = onPopupEvent(contextMenu, "hidden");
+  contextMenu.hidePopup();
+  info("Waiting for context menu on bookmarks menu to be hidden.");
+  yield contextMenuHiddenPromise;
+  let popupHiddenPromise = onPopupEvent(BMB_menuPopup, "hidden");
+  // Can't use synthesizeMouseAtCenter because the dropdown panel is in the way
+  EventUtils.synthesizeMouse(dropmarker, 2, 2, {});
+  info("Waiting for bookmarks menu to be hidden.");
+  yield popupHiddenPromise;
+}
+
+function onPopupEvent(popup, evt) {
+  let fullEvent = "popup" + evt;
+  let deferred = new Promise.defer();
+  let onPopupHandler = (e) => {
+    if (e.target == popup) {
+      popup.removeEventListener(fullEvent, onPopupHandler);
+      deferred.resolve();
+    }
+  };
+  popup.addEventListener(fullEvent, onPopupHandler);
+  return deferred.promise;
+}