Last Comment Bug 528884 - Remove places' menu and toolbar bindings
: Remove places' menu and toolbar bindings
Status: RESOLVED FIXED
[ts]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 3.7a5
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 610187 625778 795827 388880 564188 564885 571288 573302 612230 632411 741066
Blocks: 447581 SMPlacesBMarks 528006 565631 603583
  Show dependency treegraph
 
Reported: 2009-11-16 02:48 PST by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2012-10-01 01:13 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip - half working (45.11 KB, patch)
2010-02-01 09:29 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
almost done (just toolbar) (128.12 KB, patch)
2010-04-03 16:38 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
not in a follow up, actually (158.92 KB, patch)
2010-04-04 03:59 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
checkpoint (181.78 KB, patch)
2010-04-05 10:44 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
For review (192.15 KB, patch)
2010-04-19 07:21 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
patch (194.17 KB, patch)
2010-04-29 03:32 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: feedback+
Details | Diff | Splinter Review
Should be final (all tests are now passing) (217.85 KB, patch)
2010-05-02 18:22 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
another "final" version (227.54 KB, patch)
2010-05-05 03:13 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
rename selectionNodes to selectedNodes (227.52 KB, patch)
2010-05-05 04:18 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
var -> let (227.67 KB, patch)
2010-05-05 04:21 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Splinter Review
Fix few issues found by Marco (228.78 KB, patch)
2010-05-05 05:35 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
fix chevron styling (228.83 KB, patch)
2010-05-05 05:47 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-11-16 02:48:42 PST
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.
Comment 1 Gervase Markham [:gerv] 2009-11-26 05:46:19 PST
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
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-02-01 09:29:13 PST
Created attachment 424611 [details] [diff] [review]
wip - half working
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-03 16:38:23 PDT
Created attachment 436887 [details] [diff] [review]
almost done (just toolbar)

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)
Comment 4 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-04 03:59:49 PDT
Created attachment 436917 [details] [diff] [review]
not in a follow up, actually

The menu mostly works too now. I still need to work on the issues mentioned above.
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-05 10:44:23 PDT
Created attachment 437071 [details] [diff] [review]
checkpoint
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-19 07:21:33 PDT
Created attachment 439911 [details] [diff] [review]
For review

I might need to update some tests, but that's the final code, for now.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-19 07:34:41 PDT
Note to self: use https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Global_Objects/Object/create in the next round.
Comment 8 Marco Bonardo [::mak] 2010-04-21 07:45:56 PDT
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
Comment 9 Marco Bonardo [::mak] 2010-04-21 07:49:26 PDT
oh, i like the "merge common code" idea, globally.
Comment 10 Marco Bonardo [::mak] 2010-04-21 09:54:35 PDT
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 :)
Comment 11 Marco Bonardo [::mak] 2010-04-21 10:57:43 PDT
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.
Comment 12 Marco Bonardo [::mak] 2010-04-21 11:12:48 PDT
>+/**
>+ * 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 13 Marco Bonardo [::mak] 2010-04-21 11:13:09 PDT
Comment on attachment 439911 [details] [diff] [review]
For review

not ready for prime-time, clearing request.
Comment 14 Marco Bonardo [::mak] 2010-04-21 12:27:13 PDT
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!
Comment 15 Marco Bonardo [::mak] 2010-04-21 16:52:30 PDT
(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.
Comment 16 Marco Bonardo [::mak] 2010-04-22 05:15:24 PDT
about the __proto__ inheritance, maybe adding __proto__: under the prototype definition would make that more readable
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-22 07:12:02 PDT
I'm changing it to use the new create() method...
Comment 18 Marco Bonardo [::mak] 2010-04-22 07:15:49 PDT
cool, just make that thing readable :)
Comment 19 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-29 03:18:32 PDT
> >+  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.
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-29 03:19:25 PDT
>>+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.
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-29 03:28:14 PDT
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.
Comment 22 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-04-29 03:32:45 PDT
Created attachment 442360 [details] [diff] [review]
patch

By the way, I never said that I like those js asserts.
Comment 23 Marco Bonardo [::mak] 2010-04-29 08:18:47 PDT
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
Comment 24 Marco Bonardo [::mak] 2010-04-29 09:48:04 PDT
>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 25 Marco Bonardo [::mak] 2010-04-29 09:50:02 PDT
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
Comment 26 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-02 18:22:11 PDT
Created attachment 443020 [details] [diff] [review]
Should be final (all tests are now passing)

Marco, could you land this on the try-repository for me? Thanks in Advance
Comment 27 Marco Bonardo [::mak] 2010-05-03 02:50:48 PDT
sure, i will shortly.
Comment 28 Marco Bonardo [::mak] 2010-05-03 04:03:21 PDT
+  _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?
Comment 29 Marco Bonardo [::mak] 2010-05-03 04:32:27 PDT
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
Comment 30 Marco Bonardo [::mak] 2010-05-03 04:37:19 PDT
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)
Comment 31 Marco Bonardo [::mak] 2010-05-03 05:22:18 PDT
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
Comment 32 Marco Bonardo [::mak] 2010-05-03 05:27:58 PDT
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
Comment 33 Marco Bonardo [::mak] 2010-05-04 08:01:05 PDT
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.
Comment 34 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-05 03:13:36 PDT
Created attachment 443580 [details] [diff] [review]
another "final" version
Comment 35 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-05 04:18:32 PDT
Created attachment 443583 [details] [diff] [review]
rename selectionNodes to selectedNodes

Per Marco's request.
Comment 36 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-05 04:21:09 PDT
Created attachment 443584 [details] [diff] [review]
var -> let
Comment 37 Marco Bonardo [::mak] 2010-05-05 04:37:03 PDT
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
Comment 38 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-05 05:35:42 PDT
Created attachment 443592 [details] [diff] [review]
Fix few issues found by Marco
Comment 39 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-05 05:47:19 PDT
Created attachment 443594 [details] [diff] [review]
fix chevron styling
Comment 40 Marco Bonardo [::mak] 2010-05-05 06:37:00 PDT
http://hg.mozilla.org/mozilla-central/rev/d7393e28fb2d
Comment 41 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-05 13:52:00 PDT
Ts seem to be improved by 1%-2%.
Comment 42 Marco Bonardo [::mak] 2010-05-05 16:24:28 PDT
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.
Comment 43 Dão Gottwald [:dao] 2010-05-06 00:19:17 PDT
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.
Comment 44 Ria Klaassen (not reading all bugmail) 2010-05-06 11:00:09 PDT
Several extensions broken: UserChrome.js and more. I see several errors.
Comment 45 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2010-05-06 12:22:54 PDT
That's expected.
Comment 46 Mark Smith (:mcs) 2010-10-26 12:17:59 PDT
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?
Comment 47 Marco Bonardo [::mak] 2010-10-26 12:30:25 PDT
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.
Comment 48 Eric Shepherd [:sheppy] 2010-11-19 10:15:38 PST
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.
Comment 49 Eric Shepherd [:sheppy] 2010-11-19 10:19:47 PST
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.
Comment 50 Eric Shepherd [:sheppy] 2010-11-19 11:01:36 PST
I've updated:

https://developer.mozilla.org/en/Displaying_Places_information_using_views#Menu_view

Hopefully reasonably accurately. :)
Comment 51 Mark Smith (:mcs) 2010-11-19 12:34:42 PST
(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
Comment 52 Eric Shepherd [:sheppy] 2010-11-19 13:22:38 PST
Re comment #51 -- done. Thanks.
Comment 53 Alice0775 White 2011-01-17 17:50:09 PST
Is the Toolbar view available in Firefox4.0?
https://developer.mozilla.org/en/Displaying_Places_information_using_views#Toolbar_view
Comment 54 Alice0775 White 2011-02-27 16:45:06 PST
*** Bug 637223 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.