The toolbar binding was added to make our life easier. However, xbl1 has some bugs Here's a quick list of the current issues: 1. We leak the world there on toolbar customization 2. We cannot easily disable it in popup windows. 3. We cannot easily disable it when it is invisible. 4. 1+3 causes random bugs like bug 528006. 5. It might as well cause at least some of the slowness that we see in bug 504858. I suggest that we try to move the binding implementation into a simple js object.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv

todo: 1. fix toolbar customization 2. bring back some #ifdefs 3. update tests 4. fix themes (I haven't tested anything but pinstripe yet) 5. fix menus (in a follow up)

The menu mostly works too now. I still need to work on the issues mentioned above.

I might need to update some tests, but that's the final code, for now.

Note to self: use https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Global_Objects/Object/create in the next round.

Due to the size i've temporarly left out browserPlacesViews, i have to do that in a comment apart or i'll get crazy. >diff -r c6e489a7c8f8 browser/base/content/browser-menubar.inc >--- a/browser/base/content/browser-menubar.inc Mon Apr 19 02:12:01 2010 -0400 >+++ b/browser/base/content/browser-menubar.inc Mon Apr 19 17:19:53 2010 +0300 >@@ -455,21 +455,24 @@ > </menu> > > <menu id="history-menu" > oncommand="var node = event.target.node; if (node) { PlacesUIUtils.markPageAsTyped(node.uri); openUILink(node.uri, event, false, true); }" > onclick="checkForMiddleClick(this, event);" > label="&historyMenu.label;" > accesskey="&historyMenu.accesskey;"> > <menupopup id="goPopup" >- type="places" >- onpopupshowing="HistoryMenu.onPopupShowing(event);" >- onpopuphidden="HistoryMenu.onPopupHidden(event);" >- place="place:redirectsMode=2&amp;sort=4&amp;maxResults=10" >- tooltip="bhTooltip" popupsinherittooltip="true"> >+#ifndef XP_MACOSX >+ placespopup="true" >+#endif >+ onpopupshowing="if (!this.parentNode._placesView) >+ new HistoryMenu(event);" >+ disableopenintabs="true" >+ tooltip="bhTooltip" >+ popupsinherittooltip="true"> So, you removed onpopuphidden, where do you destroy the result when the popup is closed? If you don't, that's bad because it will stay in memory and update at each visit, and we really don't want that. Actually we probably don't do the same for bookmarks menu, and this could explain why after opening bookmarks menu i still get errors from it on bookmarks updating. If we could always close the root node when a menu associated with a result it is closed, would be great, for history menu, bookmarks menu and the chevron. about disableOpenInTabs, if we are going to add one property for each specific case we have, will end up with a bunch of properties. Can't we create a global property like disablePlacesCommands="ListOfCommands"? Or if this is only about the final "Open All In tabs" menuitem, i guess we can disable it for any popup that is also the rootnode? Do we have different cases? > disabled="true"> >- <menupopup id="historyUndoPopup" placespopup="true" >- onpopupshowing="HistoryMenu.populateUndoSubmenu();"/> >+ <menupopup id="historyUndoPopup" >+#ifndef XP_MACOSX >+ placespopup="true" >+#endif >+ onpopupshowing="this.parentNode.parentNode.parentNode._placesView.populateUndoSubmenu();"/> can't we use getElementById instead of all this parentNode recursion? Having a sessionstore method as part of placesView sounds a bit strange, but i don't see a better solution, if you find one, this code would be less surprising. >- <menupopup id="historyUndoWindowPopup" placespopup="true" >- onpopupshowing="HistoryMenu.populateUndoWindowSubmenu();"/> >+ <menupopup id="historyUndoWindowPopup" >+#ifndef XP_MACOSX >+ placespopup="true" >+#endif >+ onpopupshowing="this.parentNode.parentNode.parentNode._placesView.populateUndoWindowSubmenu();"/> ditto >+#ifndef XP_MACOSX >+ placespopup="true" >+#endif > context="placesContext" >- onpopupshowing="BookmarksEventHandler.onPopupShowing(event);"/> >+ onpopupshowing="if (!this.parentNode._placesView) >+ new PlacesMenu(event, 'place:folder=TOOLBAR');"/> this sux a lot, i still don't get why we did not simply add a smart bookmark pointing to the toolbar, so that: a) we don't have bugs with context menus pointing to the parent view b) users can get rid of it if they want, as well as regenerate it >diff -r c6e489a7c8f8 browser/base/content/browser-places.js >--- a/browser/base/content/browser-places.js Mon Apr 19 02:12:01 2010 -0400 >+++ b/browser/base/content/browser-places.js Mon Apr 19 17:19:53 2010 +0300 >@@ -563,25 +563,30 @@ var PlacesCommandHook = { > > // remove all tags for the associated url > PlacesUtils.tagging.untagURI(gEditItemOverlay._uri, null); > > this.panel.hidePopup(); > } > }; > >-// Helper object for the history menu. >-var HistoryMenu = { >- get _ss() { >+// Histroy view for the history menu. >+function HistoryMenu(aInitialPopupShowingEvent) { Since you're at it, what about renaming this to HistoryMenuHelper? can we get any other popupShowingEvent here? why specifying "initial"? >+HistoryMenu.prototype = { >+ get _ss HM__ss() { > delete this._ss; > return this._ss = Cc["@mozilla.org/browser/sessionstore;1"]. > getService(Ci.nsISessionStore); > }, please while here, use defineLazyServiceGetter >- onPopupShowing: function PHM_onPopupShowing(aEvent) { >+ _onPopupShowing: function HM_onPopupShowing(aEvent) { hm, i'm not sure marking this as private (_) is really useful >+ PlacesMenu.prototype._onPopupShowing.apply(this, arguments); >+ > // Don't handle events for submenus. > if (aEvent.target != aEvent.currentTarget) > return; > >- var menuPopup = aEvent.target; >- var resultNode = menuPopup.getResultNode(); >+ let resultNode = this.resultNode; >+HistoryMenu.prototype.__proto__ = PlacesMenu.prototype; >+ sigh, how much unreadable is to extend an object after having defined it with an assignment. I hope ecmascript 5 is briging some clarification. >diff -r c6e489a7c8f8 browser/base/content/browser.xul >- <hbox id="bookmarksBarContent" flex="1" >- type="places" >- place="place:folder=TOOLBAR" >- context="placesContext" >- onclick="BookmarksEventHandler.onClick(event);" >- oncommand="BookmarksEventHandler.onCommand(event);" >- onpopupshowing="BookmarksEventHandler.onPopupShowing(event);" >- tooltip="bhTooltip" popupsinherittooltip="true"/> >+ <hbox flex="1" >+ id="PlacesToolbar" >+ context="placesContext" >+ onclick="BookmarksEventHandler.onClick(event);" >+ oncommand="BookmarksEventHandler.onCommand(event);" >+ tooltip="bhTooltip" popupsinherittooltip="true"> i'm unsure if the removal of onPopupShowing is wanted, was that only serving the chevron or also containers on the toolbar? >diff -r c6e489a7c8f8 browser/base/content/global-scripts.inc >--- a/browser/base/content/global-scripts.inc Mon Apr 19 02:12:01 2010 -0400 >+++ b/browser/base/content/global-scripts.inc Mon Apr 19 17:19:53 2010 +0300 >@@ -34,11 +34,12 @@ > # and other provisions required by the GPL or the LGPL. If you do not delete > # the provisions above, a recipient may use your version of this file under > # the terms of any one of the MPL, the GPL or the LGPL. > # > # ***** END LICENSE BLOCK ***** > > <script type="application/javascript" src="chrome://global/content/printUtils.js"/> > <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/> >+<script type="application/javascript" src="chrome://browser/content/places/browserPlacesViews.js"/> Why here and not in PlacesOverlay.xul? we have windows and other objects without Places needs, no? >diff -r c6e489a7c8f8 browser/components/places/content/browserPlacesViews.js >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 >+++ b/browser/components/places/content/browserPlacesViews.js Mon Apr 19 17:19:53 2010 +0300 I don't know if this file can be obtained by copying another view file, but just creating it you are throwing away all blame information... >diff -r c6e489a7c8f8 browser/components/places/content/controller.js >@@ -191,17 +191,17 @@ PlacesController.prototype = { > // Livemark containers > var selectedNode = this._view.selectedNode; > return selectedNode && PlacesUtils.nodeIsLivemarkContainer(selectedNode); > case "placesCmd_sortBy:name": > var selectedNode = this._view.selectedNode; > return selectedNode && > PlacesUtils.nodeIsFolder(selectedNode) && > !PlacesUtils.nodeIsReadOnly(selectedNode) && >- this._view.getResult().sortingMode == >+ this._view.result.sortingMode == > Ci.nsINavHistoryQueryOptions.SORT_BY_NONE; fancy indentations >diff -r c6e489a7c8f8 browser/components/places/content/menu.xml >--- a/browser/components/places/content/menu.xml Mon Apr 19 02:12:01 2010 -0400 >+++ b/browser/components/places/content/menu.xml Mon Apr 19 17:19:53 2010 +0300 >@@ -101,17 +101,17 @@ > </method> > > <!-- This function returns information about where to drop when > dragging over this popup insertion point --> > <method name="_getDropPoint"> > <parameter name="aEvent"/> > <body><![CDATA[ > // Can't drop if the menu isn't a folder >- var resultNode = this._resultNode; >+ var resultNode = this.node; i find "node" a bit too generic, there are so many kind of nodes, resultNode was more descriptive

oh, i like the "merge common code" idea, globally.

another piece: >diff -r c6e489a7c8f8 browser/components/places/content/browserPlacesViews.js >+function PlacesViewBase(aPlace) { >+ this.place = aPlace; >+ this.controller; hm, if controller init is needed immediately, why not just inlining here its initialization? i don't see why it's useful to init it lazily, just init .controller and use it everywhere? >+PlacesViewBase.prototype = { >+ // The view's dom element (for menus, that's the <menu>). trailing space >+ _containingBox: null, >+ >+ // The xul element that represents the root container. >+ _rootElt: null, hm, so rootElt is inside containingBox. Can you point that out in the comment? So "The XUL element inside containingBox that..." actually you could __defineGetter__(_containingBox) as _rootElt.parentNode... so that you don't really care about setting it explicitly >+ >+ // Set to true for views that are represented by native widgets (i.e. >+ // the native mac menu). >+ _nativeView: false, _isNativeView maybe? >+ QueryInterface: function PVB_QueryInterface(aIID) { >+ if (aIID.equals(Ci.nsINavHistoryResultObserver) || >+ aIID.equals(Ci.nsISupportsWeakReference) | >+ aIID.equals(Ci.nsISupports)) >+ return this; >+ >+ throw Cr.NS_ERROR_NO_INTERFACE; >+ }, use XPCOMUtils.generateQI plz >+ _controller: null, >+ get controller PVB_controller() { >+ if (!this._controller) { >+ this._controller = new PlacesController(this); >+ this._containingBox.controllers.appendController(this._controller); >+ } >+ >+ return this._controller; >+ }, as i said above, ._controller is only used in one place here, and looks loke it should use .controller instead... does not look useful at first sight >+ // Temporary hack for doGetPlacesControllerForCommand. >+ get controllers PVB_controllers() this._containingBox.controllers, ok, i read this comment and i know as much as i knew before reading it :p please explain why having a .controllers property sux >+ get selType PVB_selType() "single", selType: "single"? do you plan to add multiple selections here in future? >+ get selectedNode PVB_selectedNode() { >+ if (this._contextMenuShown) { >+ let popupNode = document.popupNode; >+ if (popupNode == this._rootElt) >+ return this.resultNode; >+ >+ return popupNode.node || popupNode.parentNode._resultNode || null; ehr hm, if i read this correctly, this.resultNode if (popupNode == this._rootElt) is popupNode.node, so why the if before the return? >+ selectItems: function() { }, >+ selectAll: function() { }, hm? throw Cr.NS_NOT_IMPLEMENTED? not sure what these are here for? >+ _cleanPlacesPopup: function PVB_cleanPlacesPopup(aPopup) { well this is a Places file, and this method is only used by rebuildPopup, can we rename this just _cleanPopup? >+ /** >+ * Helper for the toolbar and menu views >+ */ Yes, we are there! (ok, this comment is useless) >+ _createMenuItemForNode: >+ function PVB__createMenuItemForNode(aNode) { please rename to _createMenuItemForPlacesNode >+ >+ _insertNewItemToPopup: >+ function PVB__insertNewItemToPopup(aChild, aParentPopup, aBefore) { rename aParentPopup to aPopup and aChild to aNewChild, the name of the function is clear enough about their roles >+ let element = this._createMenuItemForNode(aChild); >+ >+ if (aBefore) >+ aParentPopup.insertBefore(element, aBefore); >+ else { braces around the if, since the else is braced >+ // Add the new element to the menu. If there is static content at >+ // the end of the menu, add the element before that. Otherwise, >+ // just add to the end. >+ if (aParentPopup._endMarker != -1) { >+ let lastNode = aParentPopup.childNodes[aParentPopup._endMarker]; >+ aParentPopup.insertBefore(element, lastNode); >+ } >+ else >+ aParentPopup.appendChild(element); and viceversa here >+ _ensureLivemarkStatusMenuItem: >+ function PVB_ensureLivemarkStatusMenuItem(aPopup) { >+ let itemId = aPopup.node.itemId; >+ >+ let lmStatus = null; >+ if (PlacesUtils.annotations >+ .itemHasAnnotation(itemId, "livemark/loadfailed")) >+ lmStatus = "bookmarksLivemarkFailed"; >+ else if (PlacesUtils.annotations >+ .itemHasAnnotation(itemId, "livemark/loading")) >+ lmStatus = "bookmarksLivemarkLoading"; >+ cache let as = PlacesUtils.annotations to help readability here >+ if (lmStatus && !aPopup._lmStatusMenuItem) { >+ // Create the status menuitem and cache it in the popup object. >+ aPopup._lmStatusMenuItem = document.createElement("menuitem"); >+ aPopup._lmStatusMenuItem.setAttribute("lmStatus", lmStatus); >+ aPopup._lmStatusMenuItem.setAttribute("label", this.getString(lmStatus)); >+ aPopup._lmStatusMenuItem.setAttribute("disabled", true); >+ aPopup.insertBefore(aPopup._lmStatusMenuItem, >+ aPopup.childNodes.item(aPopup._startMarker + 1)); >+ aPopup._startMarker++; >+ } >+ else if (lmStatus && >+ aPopup._lmStatusMenuItem.getAttribute("lmStatus") != lmStatus) { >+ // Status has changed, update the cached status menuitem. >+ aPopup._lmStatusMenuItem.setAttribute("label", >+ this.getString(lmStatus)); >+ } >+ else if (!lmStatus && aPopup._lmStatusMenuItem) { >+ // No status, remove the cached menuitem. >+ aPopup.removeChild(aPopup._lmStatusMenuItem); >+ aPopup._lmStatusMenuItem = null; >+ aPopup._startMarker--; >+ } and here cache lmStatusElt = aPopup._lmStatusMenuItem; >+ nodeURIChanged: function PVB_nodeURIChanged(aNode, aURIString) { >+ let nodeElt = aNode._DOMElement; >+ NS_ASSERT(nodeElt, "node must have _DOMElement set"); i know you could love these, but they are giving us headaches, because if we do some wrong changes, we are nagging nightly users with a bunch of these assertions, at a point that browser becomes unusable... i think we should throw, and eventually NS_ASSERT only in DEBUG builds the throws are pretty often catched quickly by nightly testers. And this everywhere we assert this _DOMElement thing >+ nodeIconChanged: function PT_nodeIconChanged(aNode) { >+ let icon = aNode.icon; >+ if (icon) { >+ if (nodeElt.getAttribute("image") != icon) >+ nodeElt.setAttribute("image", icon); >+ } >+ else >+ nodeElt.removeAttribute("image"); brace else since if is braced >+ // Note: the base version assumes that aContainer is represented by a popup. >+ nodeInserted: function PVB_nodeInserted(aParentNode, aNode, aIndex) { Where is aContainer? >+ containerStateChanged: function() { }, You should use this one instead of opened/closed >+ // Note: the base version assumes that aContainer is represented by a popup. >+ invalidateContainer: function PVB_invalidateContainer(aContainer) { from the comment it's unclear if the base version is this one or another ones, better comment please. Something like "this version assumes that, instead that version..." >+ _mayAddCommandsItems: function PVB__mayAddCommandsItems(aPopup) { >+ if (aPopup.getAttribute("disableopenintabs") == "true") >+ return; as i said, if possible we should provider a better attribute allowed to be a list, or just check if this popup is a root, and disable in such a case >+ // Check if the popup contains at least 2 menuitems with places nodes >+ let numNodes = 0; rename to numURINodes >+ let hasMultipleURIs = false; move after the while as let hasMultipleURIs = numURINodes > 1; yeah the code following is crazy, and i probably wrote it. so i'm closing my eyes for now :)

the toolbar binding: >+function PlacesToolbar(aPlace) { >+ // Set smart-getters for our elements. >+ let self = this; i hate "self", can we give it a better name: toolbar, thisView, Whatever? :) >+ // Set event listeners. yes, do that, and also remove this comment, is not addEventListener clear enough? >+ this._containingBox.addEventListener("dragstart", this, false); >+ this._containingBox.addEventListener("dragover", this, false); >+ this._containingBox.addEventListener("dragleave", this, false); >+ this._containingBox.addEventListener("dragend", this, false); >+ this._containingBox.addEventListener("drop", this, false); >+ this._containingBox.addEventListener("mousemove", this, false); >+ this._containingBox.addEventListener("mouseover", this, false); >+ this._containingBox.addEventListener("mouseout", this, false); >+#ifdef XP_UNIX >+#ifndef XP_MACOSX >+ this._containingBox.addEventListener("mousedown", this, false); >+#endif >+#endif >+ this._rootElt.addEventListener("popupshowing", this, true); >+ this._rootElt.addEventListener("popuphidden", this, false); >+ this._rootElt.addEventListener("overflow", this, false); >+ this._rootElt.addEventListener("underflow", this, false); >+ window.addEventListener("resize", this, false); >+ window.addEventListener("unload", this, false); what about array [[topic, listener]] and forEach >+PlacesToolbar.prototype = { >+ QueryInterface: function PT_QueryInterface(aIID) { >+ if (aIID.equals(Ci.nsIDOMEventListener) || >+ aIID.equals(Ci.nsITimerCallback)) >+ return this; >+ >+ return PlacesViewBase.prototype.QueryInterface.apply(this, arguments); >+ }, XPCOMUtils! >+ uninit: function PT_uninit() { >+ this._containingBox.removeEventListener("dragstart", this, false); >+ this._containingBox.removeEventListener("dragover", this, false); >+ this._containingBox.removeEventListener("dragleave", this, false); >+ this._containingBox.removeEventListener("dragend", this, false); >+ this._containingBox.removeEventListener("drop", this, false); >+ this._containingBox.removeEventListener("mousemove", this, false); >+ this._containingBox.removeEventListener("mouseover", this, false); >+ this._containingBox.removeEventListener("mouseout", this, false); >+#ifdef XP_UNIX >+#ifndef XP_MACOSX >+ this._containingBox.removeEventListener("mousedown", this, false); >+#endif >+#endif >+ this._rootElt.removeEventListener("popupshowing", this, true); >+ this._rootElt.removeEventListener("popuphidden", this, false); >+ this._rootElt.removeEventListener("overflow", this, false); >+ this._rootElt.removeEventListener("underflow", this, false); >+ window.removeEventListener("resize", this, false); >+ window.removeEventListener("unload", this, false); ditto >+ restoreFromToolbarCustomization: function() { >+ // TODO >+ }, >+ _openedMenuButton: null, >+ _allowPopupShowing: true, >+ trailing spaces >+ _rebuild: function PT__rebuild() { >+ // Clear out references to existing nodes, since they will be removed >+ // and re-added. >+ if (this._overFolder.node) >+ this._clearOverFolder(); >+ >+ this._openedMenuButton = null; >+ while (this._rootElt.hasChildNodes()) >+ this._rootElt.removeChild(this._rootElt.firstChild); brace loops >+ _insertNewItem: >+ function PT__insertNewItem(aChild, aBefore) { >+ let type = aChild.type; >+ let button; >+ if (type == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) >+ button = document.createElement("toolbarseparator"); >+ else { brace if since else is braced >+ button = document.createElement("toolbarbutton"); >+ button.className = "bookmark-item"; >+ button.setAttribute("label", aChild.title); >+ let icon = aChild.icon; >+ if (icon) >+ button.setAttribute("image", icon); >+ >+ if (PlacesUtils.containerTypes.indexOf(type) != -1) { >+ button.setAttribute("type", "menu"); >+ button.setAttribute("container", "true"); >+ >+ if (PlacesUtils.nodeIsQuery(aChild)) { >+ button.setAttribute("query", "true"); >+ if (PlacesUtils.nodeIsTagQuery(aChild)) >+ button.setAttribute("tagContainer", "true"); >+ } >+ else if (PlacesUtils.nodeIsLivemarkContainer(aChild)) >+ button.setAttribute("livemark", "true"); brace else if >+ else if (PlacesUtils.nodeIsURI(aChild)) >+ button.setAttribute("scheme", PlacesUIUtils.guessUrlSchemeForUI(aChild.uri)); ditto >+ _onChevronPopupShowing: >+ function PT__updateChevronPopupNodesVisibility(aEvent) { the label does not match function's name >+ // Handle popupshowing only for the chevron popup, not for nested ones. >+ let popup = aEvent.target; >+ if (aEvent.target != this._chevronPopup) >+ return; >+ >+ if (!this._chevron._placesView) >+ this._chevron._placesView = new PlacesMenu(aEvent, this.place); >+ >+ this._updateChevronPopupNodesVisibility(); popup var is not used anywhere in the method >+ _overFolder: { node: null, openTimer: null, hoverTime: 350, >+ closeTimer: null }, please reindent this, no inline, each property on its line >+ _getDropPoint: function PT__getDropPoint(aEvent) { >+ // This function returns information about where to drop when >+ // dragging over the toolbar. >+ // The returned object has 3 properties: >+ // - ip: the insertion point for the bookmarks service. >+ // - beforeIndex: child index to drop before, for the drop indicator. >+ // - folderNode: the folder to drop into, if applicable. move documentation to a javadoc BEFORE the method please.

>+/** >+ * View for places menu. This object should be created during the first >+ * popupshowing that's dispatched on the menu. >+ */ nit: Places is uppercase! >+function PlacesMenu(aInitialPopupShowingEvent, aPlace) { i'd drop the "initial", the comment is already saying that i should have finished for this round, next please!

Comment on attachment 439911 [details] [diff] [review] For review not ready for prime-time, clearing request.

Mano, since we talked about that bug to move treeView.js into a lazy getter in placesOverlay.xul, i think we should do the same for editBookmarkOverlay.js that is currently in browser.xul. please, kthx!

(In reply to comment #14) > Mano, since we talked about that bug to move treeView.js into a lazy getter in > placesOverlay.xul, i think we should do the same for editBookmarkOverlay.js > that is currently in browser.xul. please, kthx! actually, loadSubscript bypasses fastload cache, thus we can't do that. nevermind for now.

about the __proto__ inheritance, maybe adding __proto__: under the prototype definition would make that more readable

I'm changing it to use the new create() method...

cool, just make that thing readable :)

> >+ selectItems: function() { }, > >+ selectAll: function() { }, > hm? throw Cr.NS_NOT_IMPLEMENTED? not sure what these are here for? > >+ get selType PVB_selType() "single", > selType: "single"? do you plan to add multiple selections here in future? Remember: there's one more view in tree.xml.

>>+PlacesToolbar.prototype = { >>+ QueryInterface: function PT_QueryInterface(aIID) { >>+ if (aIID.equals(Ci.nsIDOMEventListener) || >>+ aIID.equals(Ci.nsITimerCallback)) >>+ return this; >>+ >>+ return PlacesViewBase.prototype.QueryInterface.apply(this, arguments); >>+ }, > XPCOMUtils! No! I would like to use inheritance here.

Also: For Comment 8: 1. disableopenintabs - removed. 2. Removing live update for the bookmarks menu is out of the scope of this bug. Please file a new bug, so I can mark it WONTFIX. 3. special bookmarks-toolbar menu - that's also out of the scope of this bug. 4. "Since you're at it, what about renaming this to HistoryMenuHelper?" No, it inherits from "PlacesMenu". 5. placesOverlay.xul is not the right file for browser-window-only stuff (No, the new code is not expendable as far as I'm concerned). For Comment 11: 1. "What about array [[topic, listener]] and forEach?" Done, though I'm not sure what's the outcome here.

By the way, I never said that I like those js asserts.

Comment on attachment 442360 [details] [diff] [review] patch As usual this is all but browserPlacesViews, that will follow >diff -r b464cc1789bd browser/base/content/browser-menubar.inc >- <menupopup id="historyUndoPopup" placespopup="true" >- onpopupshowing="HistoryMenu.populateUndoSubmenu();"/> >+ <menupopup id="historyUndoPopup" >+#ifndef XP_MACOSX >+ placespopup="true" >+#endif >+ onpopupshowing="this.parentNode.parentNode.parentNode._placesView.populateUndoSubmenu();"/> no way to make this better eh? (getElById) > </menu> > <menu id="historyUndoWindowMenu" > label="&historyUndoWindowMenu.label;" > disabled="true"> >- <menupopup id="historyUndoWindowPopup" placespopup="true" >- onpopupshowing="HistoryMenu.populateUndoWindowSubmenu();"/> >+ <menupopup id="historyUndoWindowPopup" >+#ifndef XP_MACOSX >+ placespopup="true" >+#endif >+ onpopupshowing="this.parentNode.parentNode.parentNode._placesView.populateUndoWindowSubmenu();"/> ditto >diff -r b464cc1789bd browser/base/content/browser-places.js >+// Histroy view for the history menu. typo: Histroy Btw, "View for the history menu." seems to be enough here. >- onPopupShowing: function PHM_onPopupShowing(aEvent) { >+ _onPopupShowing: function HM_onPopupShowing(aEvent) { nit HM__onPopupShowing >+ updateState: function PTH_updateState() { Add some documentation to what this is doing, code is clear enough for me, but could not be for others. Also when you call the method in browser code, for example in case of it being removed, a oneline comment could help. >diff -r b464cc1789bd browser/base/content/browser.xul >+ <hbox flex="1" >+ id="PlacesToolbar" >+ context="placesContext" >+ onclick="BookmarksEventHandler.onClick(event);" >+ oncommand="BookmarksEventHandler.onCommand(event);" >+ tooltip="bhTooltip" popupsinherittooltip="true"> above you have put popupinherittooltip in a new line, be consistent, either give it a line or not >+ <image id="PlacesToolbarDropIndicator" mousethrough="always" >+ collapsed="true"/> >+ </hbox> >+ <scrollbox orient="horizontal" id="PlacesToolbarItems" >+ flex="1"/> mousethrough and id on their own lines, and id before orient please >+ <toolbarbutton type="menu" >+ id="PlacesChevron" >+ mousethrough="never" >+ collapsed="true" >+ tooltiptext="&bookmarksToolbarChevron.tooltip;" >+ onpopupshowing="this.parentNode.parentNode >+ ._placesView._onChevronPopupShowing(event);"> parentNode.parentNode :( it could be faster, but it's so prone to "what?" >diff -r b464cc1789bd browser/base/content/global-scripts.inc Please get approval from Gavin, this file is out of my approval powers >diff -r b464cc1789bd browser/components/places/content/controller.js >- var nodes = this._view.getSelectionNodes(); >+ var nodes = this._view.selectionNodes; reasons to not call this selectedNodes? is it to avoid typos with selectedNode? I guess we could have all the code in selectedNodes, and have selectedNode return selectedNodes[0]? > function doGetPlacesControllerForCommand(aCommand) > { >- var placesController = top.document.commandDispatcher >+ let placesController = top.document.commandDispatcher > .getControllerForCommand(aCommand); >+ if (placesController) >+ return placesController; can we rename placesController var to just controller? >diff -r b464cc1789bd browser/components/places/content/tree.xml >- </method> >+ ]]></getter> >+ </property> > > <!-- nsIPlacesView --> I see some trailing space there

>diff -r b464cc1789bd browser/components/places/content/browserPlacesViews.js >+ * The Original Code is Mozilla History System I did not even know we have an "History System" module... >+ QueryInterface: function PVB_QueryInterface(aIID) { >+ if (aIID.equals(Ci.nsINavHistoryResultObserver) || >+ aIID.equals(Ci.nsISupportsWeakReference) | >+ aIID.equals(Ci.nsISupports)) >+ return this; >+ >+ throw Cr.NS_ERROR_NO_INTERFACE; >+ }, Well, here you can use XPCOMUtils, since this does not inherit. >+ get selectedNode() { >+ if (this._contextMenuShown) { >+ let popupNode = document.popupNode; >+ if (popupNode == this._rootElt) >+ return this._resultNode; >+ >+ return popupNode._placesNode || popupNode.parentNode._resultNode || null; I still don't completely understand this if (popupNode == this._rootElt) In such a case i'd expect this._resultNode == popupNode._placesNode, thus the next return popupNode._placesNode should be fine already. >+ _cleanPlacesPopup: function PVB_cleanPlacesPopup(aPopup) { I still think you could name this just _cleanPopup >+ >+ /** >+ * Helper for the toolbar and menu views >+ */ kill this comment please, we are already in a sort of helper >+ _insertNewItemToPopup: >+ function PVB__insertNewItemToPopup(aNewChild, aPopup, aBefore) { >+ let element = this._createMenuItemForPlacesNode(aNewChild); >+ >+ if (aBefore) >+ aPopup.insertBefore(element, aBefore); >+ else { braces around if since else is braced >+ nodeIconChanged: function PT_nodeIconChanged(aNode) { >+ let nodeElt = aNode._DOMElement; >+ if (!nodeElt) >+ throw "aNode must have _DOMElement set"; >+ >+ // There's no UI representation for the root node, thus there's nothing to >+ // be done when the icon changes. >+ if (nodeElt == this._rootElt) >+ return; >+ >+ // If the node is a container, we need to update its <menu>. >+ if (nodeElt.localName == "menupopup") >+ nodeElt = nodeElt.parentNode; >+ >+ let icon = aNode.icon; >+ if (icon) { >+ if (nodeElt.getAttribute("image") != icon) >+ nodeElt.setAttribute("image", icon); >+ } >+ else >+ nodeElt.removeAttribute("image"); inverting should reduce code: if (!icon) removeAttr else if (attr != icon) setAttr >+ /** >+ * Adds an "Open All in Tabs" menuitem to the bottom of the popup. >+ * @param aPopup >+ * a places popup. >+ */ nit: Places should be uppercase >+ _onPopupShowing: function PVB_onPopupShowing(aEvent) { nit: PVB__onPopupShowing >+ _addEventListeners: >+ function PVB__addEventListeners(aObject, aEventNames, aCapturing) { >+ for (let i=0; i < aEventNames.length; i++) { add spaces in i=0 >+ _removeEventListeners: >+ function PVB__removeEventListeners(aObject, aEventNames, aCapturing) { >+ for (let i=0; i < aEventNames.length; i++) { ditto >+function PlacesToolbar(aPlace) { >+ // Set smart-getters for our elements. nit: s/set smart-getters/add some smart getter/ >+ let thisView = this; >+ [ >+ ["_viewElt", "PlacesToolbar"], >+ ["_rootElt", "PlacesToolbarItems"], bad indentation on the first one

Comment on attachment 442360 [details] [diff] [review] patch Globally looks good enough, we obviously need: - tests passing, please check - some basic manual functionality test (checking that there are no errors in the console for whatever reason). I can help here once we have a version that passes automated tests. - try server run, absolutely before going central

Marco, could you land this on the try-repository for me? Thanks in Advance

sure, i will shortly.

+ _getDropPoint: function PT__getDropPoint(aEvent) { + if (PlacesUtils.nodeIsFolder(xulNode.node) && + !PlacesUtils.nodeIsReadOnly(xulNode.node)) { should not these be xulNode._placesNode? + // Drop inside this folder. + dropPoint.ip = + new InsertionPoint(PlacesUtils.getConcreteItemId(xulNode._placesNode), + -1, Ci.nsITreeView.DROP_ON, + PlacesUtils.nodeIsTagQuery(xulNode.node)); ditto + _onMouseOver: function PT__onMouseOver(aEvent) { + let button = aEvent.target; + if (button.parentNode == this._rootElt && button._placesNode && + PlacesUtils.nodeIsURI(button.node)) + window.XULBrowserWindow.setOverLink(aEvent.target._placesNode.uri, null); + }, PlacesUtils.nodeIsURI(button._placesNode) closeParentMenus: function OF__closeParentMenus() { var popup = this._self; var parent = popup.parentNode; while (parent) { - if (parent.nodeName == "menupopup" && parent._resultNode) { + if (parent.nodeName == "menupopup" && parent.node) { parent._placesNode?

ugh this is why the previous system was ugly _getDropPoint: function PT__getDropPoint(aEvent) { let result = this.result; if (!PlacesUtils.nodeIsFolder(this._resultNode)) return null; let dropPoint = { ip: null, beforeIndex: null, folderNode: null }; let xulNode = aEvent.target; if (xulNode._placesNode && xulNode != this._rootElt && xulNode._placesNode.localName != "menupopup") { this ._placesNode looks wrong since i doubt it can have localName property

check drag&drop on toolbar, it's not working, but could just be due to the above node errors. If i go to History/Today and i try to delete the first entry, the view is not updated (could be not due to this patch, but please check)

in menu.xml // Drop below the item. dropPoint.ip = new InsertionPoint( PlacesUtils.getConcreteItemId(resultNode), -1, Ci.nsITreeView.DROP_AFTER, PlacesUtils.nodeIsTagQuery(xulNode.node), xulNode._placesNode.itemId); return dropPoint; xulNode.node is wrong // Helper function to close all parent menus of this menu, // as long as none of the parent's children are currently being // dragged over. closeParentMenus: function OF__closeParentMenus() { var popup = this._self; var parent = popup.parentNode; while (parent) { if (parent.nodeName == "menupopup" && parent.node) { parent.node looks wrong

when dragging on a toolbar popup i keep getting Error: this._folder.node.lastChild.hidePopup is not a function Source File: chrome://browser/content/places/menu.xml Line: 264

I pushed to tryserver a version addressing comment 28, comment 29 and comment 31, Linux ended up green, thus i suspect this is fine so far. I did not investigate comment 32 though.

Per Marco's request.

Comment on attachment 443584 [details] [diff] [review] var -> let ok, looks like everything has been fixed. I guess this could still need some manual testing but I'd say it's code complete

Ts seem to be improved by 1%-2%.

So, we are hit by bug 563836, that caused a Ts Shutdown increase of 100ms on all os x 10.5.8 boxes... I'm not sure how we'll proceed, that is not fault of this patch, but i could evaluate to back it out till we install resistors on on boxes, and reland it immediately after. The same thing happened for the new add-on manager landing, that has been backed out and is waiting for that install.

We should let it stick. The add-ons manager landing probably wouldn't have been backed out if it would have been clear what the culprit was.

Several extensions broken: UserChrome.js and more. I see several errors.

That's expected.

Because this bug removed support for <menupopup type="places">, documentation should be updated in at least these two places: https://developer.mozilla.org/en/Firefox_4_for_developers https://developer.mozilla.org/en/Displaying_Places_information_using_views#Menu_view One of the extensions my company maintains uses <menupopup type="places"> within a toolbarbutton in Firefox 3.x, and after reading this bug report we figured out how to use a combination of placespopup="true" and a call to new PlacesMenu() inside the onpopupshowing event handler. It took us a while to find this bug report though. What is the appropriate way to flag this bug so that the docs get updated?

it is dev-doc-needed keyword, but I guess people looking at it could lose the same time you lost to figure out the changes. One of the Places peer should write it (Mano would be the best, I could also do it in case, it is just we are head down on blockers now :(), otherwise, if you ended up writing sample code for this already, you could even update the docs, it's an open wiki after all. Sorry for the unreported change.

Is there anyone that can handle writing up this change or should I budget time to do it myself? If someone can at least provide a code snippet demonstrating how to create a places menu now that type="places" is gone that would save me a bunch of time.

I've added a note to Firefox 4 for developers about this change, but the https://developer.mozilla.org/en/Displaying_Places_information_using_views#Menu_view article still needs updating.

I've updated: https://developer.mozilla.org/en/Displaying_Places_information_using_views#Menu_view Hopefully reasonably accurately. :)

(In reply to comment #50) > I've updated: > > https://developer.mozilla.org/en/Displaying_Places_information_using_views#Menu_view > > Hopefully reasonably accurately. :) Thanks Eric. The code I used in my extension is very similar to what you included in the documentation, except I skipped the check for _placesView (probably a good optimization). It might make sense to revise the following section to note that the place attribute cannot be used directly in Firefox 4/Gecko 2 with menus. https://developer.mozilla.org/en/Displaying_Places_information_using_views#Connecting_a_view_to_its_data

Re comment #51 -- done. Thanks.

Is the Toolbar view available in Firefox4.0? https://developer.mozilla.org/en/Displaying_Places_information_using_views#Toolbar_view