580656 - Add new places core for bookmarks (and history)

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

For the new places bookmarks system, we should be re-porting the places core from Firefox, and later make our history implementation base on that same core.

This is a split-off of parts 1+2 of bug 498596, for easier review (as now that part is isolated here).

Comment 1

[Robert Kaiser]

Attachment: part1, v2: PlacesUIUtils

I splitted the core into smaller chunks for better review, here's the first, which is, as in the other bug, the PlacesUIUtils module.

Comment 2

[Robert Kaiser]

Attachment: part 2, v2: controller, sidebar utils, content CSS

The second part adds the controller, sidebar utils (which can be shared by both sidebars), and the content CSS file.

Comment 3

[Robert Kaiser]

Attachment: part 3, v2: tree

Part 3 adds the places tree XBL and view files.

Comment 4

[Robert Kaiser]

Attachment: part 4, v2: toolbar and menu files

Part 4 is adding the files for toolbar and menu implementations, which in the new version have a "view" similar to the tree view so those implementations could be made more similar.

Comment 5

[Robert Kaiser]

Attachment: diff of Firefox places files vs. SeaMonkey places files

Here's a diff of the Firefox files against ours, for easier review.

Comment 6

[Robert Kaiser]

Comment on attachment 459031 [details] [diff] [review]
part1, v2: PlacesUIUtils

It seems that Neil lacks time, energy and/or motivation right now to get those reviews done, so trying Ian as well and taking review from whoever gets to do it first.

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 461275 [details] [diff] [review]
diff of Firefox places files vs. SeaMonkey places files

Part 1 Review...

>--- mozilla/browser/components/places/src/PlacesUIUtils.jsm	2010-07-28 20:56:04.445218412 +0200
>+++ suite/common/src/PlacesUIUtils.jsm	2010-07-29 13:55:50.140041160 +0200
>   _showBookmarkDialog: function PUIU__showBookmarkDialog(aInfo, aMinimalUI) {
>     var dialogURL = aMinimalUI ?
>-                    "chrome://browser/content/places/bookmarkProperties2.xul" :
>-                    "chrome://browser/content/places/bookmarkProperties.xul";
>+                    "chrome://communicator/content/bookmarks/bm-props2.xul" :
>+                    "chrome://communicator/content/bookmarks/bm-props.xul";
> 

Nit: override is not declared in this bug, and only exists in Bug 580662. [ok not to fix in this changeset IFF you land that bug in the same push, otherwise I'd like it in this changeset instead.]

>-        // History Query.
>-        this.create_query("History", leftPaneRoot,
>-                          "place:type=" +
>-                          Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY +
>-                          "&sort=" +
>-                          Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING);
>-
>-        // XXX: Downloads.
>-

Looks like we DO setup (above) History as a valid query, but we don't actually CREATE the history query. I think we should have this here.

Comment 8

[Justin Wood (:Callek)]

Comment on attachment 459031 [details] [diff] [review]
part1, v2: PlacesUIUtils

>+++ b/suite/locales/en-US/chrome/common/places/places.properties

This patch alone does not use MOST of these properties. However all the required ones are here.

Ones missing here vs. the Firefox one:

deleteHost, deleteDomain, sortByName, sortByNameGeneric, searchHistory, OrganizerQueryDownloads, 

ONLY searchHistory seems to even be used by Firefox.

So, I'm happy keeping the others out, but probably good to add-in searchHistory.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 459031 [details] [diff] [review]
part1, v2: PlacesUIUtils

fyi I merely skimmed the Firefox code which we didn't change here, so that aspect is mostly an rs+. I feel safe letting you land this, but could still do good by a Neil or IanN review.

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 459032 [details] [diff] [review]
part 2, v2: controller, sidebar utils, content CSS

Basically every note I have for part 2 is nits, and none of which is blocking. again a mostly rs+ review, so I think IanN or Neil looking this over is good, but I also feel its safe to land.

>--- mozilla/browser/components/places/content/controller.js	2010-07-09 22:05:09.891212321 +0200
>+++ suite/common/places/controller.js	2010-07-29 13:55:50.181047677 +0200
>+// XXX: should be defined more globally
>+var TAB_DROP_TYPE = "application/x-moz-tabbrowser-tab";
>+

FYI Firefox defines this in utilityOverlay, (solving the defined more globally) and because of that it gets used from there _and_ in their tabbrowser impl. We probably need a tabbrowser bit of code on our end for this |type| as well. But easily done in a followup.

>   moveSelectedBookmarks: function PC_moveBookmarks() {
>-    window.openDialog("chrome://browser/content/places/moveBookmarks.xul",
>+    window.openDialog("chrome://communicator/content/bookmarks/moveBookmarks.xul",
>                       "", "chrome, modal",

File moveBookmarks.xul is not defined as of this Part [2].

>--- mozilla/browser/components/places/content/places.css	2010-07-09 22:05:09.899262950 +0200
>+++ suite/common/places/places.css	2010-07-29 13:55:50.181047677 +0200
>@@ -1,5 +1,5 @@
> tree[type="places"] {
>-  -moz-binding: url("chrome://browser/content/places/tree.xml#places-tree");
>+  -moz-binding: url("chrome://communicator/content/places/tree.xml#places-tree");
> }

Not moved here yet as of this part (still under history/)

> menupopup[placespopup="true"] {
>-  -moz-binding: url("chrome://browser/content/places/menu.xml#places-popup-base");
>+  -moz-binding: url("chrome://communicator/content/places/menu.xml#places-popup-base");
> }

Does not exist yet as of this part.

>--- mozilla/browser/components/places/content/sidebarUtils.js	2010-07-09 22:05:09.911252374 +0200
>+++ suite/common/places/sidebarUtils.js	2010-07-29 13:55:50.182047836 +0200
- *   Robert Kaiser <[e-mail]>

Do you really mean to do this?

>-#ifdef XP_MACOSX
>-    var modifKey = aEvent.metaKey || aEvent.shiftKey;
>-#else
>-    var modifKey = aEvent.ctrlKey || aEvent.shiftKey;
>-#endif
>+    var openWhere = whereToOpenLink(aEvent, false, true);

Nice catch ;-)

Comment 11

[Robert Kaiser]

(In reply to comment #7)
> Looks like we DO setup (above) History as a valid query, but we don't actually
> CREATE the history query. I think we should have this here.

Need to look into that, but should not be used in the bookmarks patches anyhow. The bookmarks manager intentionally doesn't display history on our side.

(In reply to comment #8)
> This patch alone does not use MOST of these properties. However all the
> required ones are here.

The others are used by later patches, I didn't want to slice this file between them.

> So, I'm happy keeping the others out, but probably good to add-in
> searchHistory.

We never use searchHistory, that's only used in Places Organizer in their side, which is Bookmarks Manager on our side and doesn't support history.

(In reply to comment #10)
> FYI Firefox defines this in utilityOverlay,

Actually, good catch, as I _have_ it there in one of the other patches, defning it in two places is too much, I guess. ;-)

> >   moveSelectedBookmarks: function PC_moveBookmarks() {
> >-    window.openDialog("chrome://browser/content/places/moveBookmarks.xul",
> >+    window.openDialog("chrome://communicator/content/bookmarks/moveBookmarks.xul",
> >                       "", "chrome, modal",
> 
> File moveBookmarks.xul is not defined as of this Part [2].

Yes, same as with bm-props*.xul, all part of the management UI patch - but then, all the patches in here don't make us use any of this new code, only the later patches make us use this all.

> >--- mozilla/browser/components/places/content/places.css    2010-07-09 22:05:09.899262950 +0200
> >+++ suite/common/places/places.css    2010-07-29 13:55:50.181047677 +0200
> >@@ -1,5 +1,5 @@
> > tree[type="places"] {
> >-  -moz-binding: url("chrome://browser/content/places/tree.xml#places-tree");
> >+  -moz-binding: url("chrome://communicator/content/places/tree.xml#places-tree");
> > }
> 
> Not moved here yet as of this part (still under history/)

This is in part 3.

> > menupopup[placespopup="true"] {
> >-  -moz-binding: url("chrome://browser/content/places/menu.xml#places-popup-base");
> >+  -moz-binding: url("chrome://communicator/content/places/menu.xml#places-popup-base");
> > }
> 
> Does not exist yet as of this part.

This is in part 4.

The splitting into parts in here is _only_ for easier review, not for staged landing or anything.

> >--- mozilla/browser/components/places/content/sidebarUtils.js    2010-07-09 22:05:09.911252374 +0200
> >+++ suite/common/places/sidebarUtils.js    2010-07-29 13:55:50.182047836 +0200
> - *   Robert Kaiser <[e-mail]>
> 
> Do you really mean to do this?

Well, I didn't care much about those as my own contributions to those files are minimal at best. But I surely can re-add myself here if you like. :)

Comment 12

[Ian Neal]

Comment on attachment 459031 [details] [diff] [review]
part1, v2: PlacesUIUtils

>diff --git a/suite/common/history/utils.js b/suite/common/src/PlacesUIUtils.jsm
>+XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() {
>+  Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
>+  return PlacesUtils;
> });
Any reason why the rest of the lazy getters are not next to this one?

>+  createFixedURI: function PUIU_createFixedURI(aSpec) {
>+    return URIFixup.createFixupURI(aSpec, 0);
>   },
If we mean Components.interfaces.nsIURIFixup.FIXUP_FLAG_NONE then we should probably use it.
> 
>+  _wrapString: function PUIU__wrapString(aString) {
>     var s = Components.classes["@mozilla.org/supports-string;1"]
>                       .createInstance(Components.interfaces.nsISupportsString);
>     s.data = aString;
>     return s;
>   },
It would be useful to have something like this on a more global level (I count about 5 or so places which use something similar), but that is outside the scope of this bug.

>+  /**
>+   * Get a transaction for copying a uri item from one container to another
>+   * as a bookmark.
>+   * @param   aData
>+   *          JSON object of dropped or pasted item properties
>+   * @param   aContainer
>+   *          The container being copied into
>+   * @param   aIndex
>+   *          The index within the container the item is copied to
>+   * @returns A nsITransaction object that performs the copy.
>+   */
>+  _getURIItemCopyTransaction: function (aData, aContainer, aIndex) {
>+    return this.ptm.createItem(PlacesUtils._uri(aData.uri), aContainer, aIndex,
>+                               aData.title, "");
>+  },
Shouldn't we be using the transactions objects exported by the PlacesUtils.jsm module (in this case PlacesCreateBookmarkTransaction) directly rather than using the compatibility shim? Lots of other uses of ptm in this patch too. Probably a follow-up bug though.

>+  /**
>+   * Methods to show the bookmarkProperties dialog in its various modes.
>+   *
>+   * The showMinimalAdd* methods open the dialog by its alternative URI. Thus
>+   * they persist the dialog dimensions separately from the showAdd* methods.
>+   * Note these variants also do not return the dialog "performed" state since
>+   * they may not open the dialog modally.
>+   */
Seems to be a lot of code duplication within the various methods, but again probably worthy of a follow-up bug.

>   /**
>    * Allows opening of javascript/data URI only if the given node is
>    * bookmarked (see bug 224521).
>    * @param aURINode
>    *        a URI node
>    * @return true if it's safe to open the node in the browser, false otherwise.
>    *
>    */
>-  checkURLSecurity: function PU_checkURLSecurity(aURINode) {
>+  checkURLSecurity: function PUIU_checkURLSecurity(aURINode, aWindow) {
The function has an extra param but it is not mentioned in the comment.

>+  /** aItemsToOpen needs to be an array of objects of the form:
>+    * {uri: string, isBookmark: boolean}
>+    */
>+  _openTabset: function PUIU__openTabset(aItemsToOpen, aEvent) {
>+    if (!aItemsToOpen.length)
>+      return;
>+
>+    var urls = [];
>+    for (var i = 0; i < aItemsToOpen.length; i++) {
>+      var item = aItemsToOpen[i];
>+      if (item.isBookmark)
>+        this.markPageAsFollowedBookmark(item.uri);
>+      else
>+        this.markPageAsTyped(item.uri);
>+
>+      urls.push(item.uri);
>+    }
>+
>+    var browserWindow = this._getTopBrowserWin();
>+    var where = browserWindow ?
>+                browserWindow.whereToOpenLink(aEvent, false, true) : "window";
>+    if (where == "window") {
>+      let win = this._getCurrentActiveWin();
>+      win.openDialog(win.getBrowserURL(), "_blank",
>+                     "chrome,all,dialog=no", urls.join("\n"));
>+      return;
>+    }
>+
>+    var loadInBackground = where == "tabshifted" ? true : false;
>+    var replaceCurrentTab = where == "tab" ? false : true;
>+    browserWindow.gBrowser.loadTabs(urls, loadInBackground, replaceCurrentTab);
>+  },
Can we not use openUILinkArrayIn as we did in utils.js?

r=me with those addressed.

Comment 13

[Ian Neal]

Comment on attachment 459032 [details] [diff] [review]
part 2, v2: controller, sidebar utils, content CSS

>diff --git a/suite/common/history/controller.js b/suite/common/places/controller.js

>-    // All other Places Commands are prefixed with "placesCmd_" ... this
>+    // All other Places Commands are prefixed with "placesCmd_" ... this 
Nit: unneeded added whitespace at end of comment.

>     case "cmd_selectAll":
>       if (this._view.selType != "single") {
>-        var rootNode = this._view.getResultNode();
>+        var rootNode = this._view.result.root;
>         if (rootNode.containerOpen && rootNode.childCount > 0)
>-          return true;
>+            return true;
Nit: wrong indentation.

>-    for (var i = 0; i < nodes.length; i++) {
>+    for (var i=0; i < nodes.length; i++) {
Nit: lost spaces round =

>+      // annotations
>+      if (uri) {
>+        var names = PlacesUtils.annotations.getPageAnnotationNames(uri);
>+        for (var j = 0; j < names.length; ++j)
>+          nodeData[names[j]] = true;
>+      }
>+
>+      // For items also include the item-specific annotations
>+      if (node.itemId != -1) {
>+        names = PlacesUtils.annotations
>+                           .getItemAnnotationNames(node.itemId);
>+        for (j = 0; j < names.length; ++j)
>+          nodeData[names[j]] = true;
>+      }
Are j and names always going to be defined, would using let be better than var here?

>+    if (forceHideAttr) {
>+      var forceHideRules = forceHideAttr.split("|");
>+      for (var i = 0; i < aMetaData.length; ++i) {
>+        for (var j=0; j < forceHideRules.length; ++j) {
Nit: more lost spaces around =

>   /**
>-   * Select all links in the current view.
>+   * Select all links in the current view. 
Nit: unneeded added whitespace at end of comment.

>         var buttonPressed = promptService.confirmEx(window,
>           PlacesUIUtils.getString("tabs.openWarningTitle"),
>-          PlacesUIUtils.getFormattedString(messageKey,
>+          PlacesUIUtils.getFormattedString(messageKey, 
Nit: unneeded whitespace at end of line.

>    * Walk the list of folders we're removing in this delete operation, and
>-   * see if the selected node specified is already implicitly being removed
>-   * because it is a child of that folder.
>+   * see if the selected node specified is already implicitly being removed 
>+   * because it is a child of that folder. 
Nit: more unneeded whitespaces at the end of the comments.
>    * @param   node
>-   *          Node to check for containment.
>+   *          Node to check for containment. 
Nit: unneeded whitespace at the end of the comment.
>    * @param   pastFolders
>    *          List of folders the calling function has already traversed
>-   * @returns true if the node should be skipped, false otherwise.
>+   * @returns true if the node should be skipped, false otherwise. 
Nit: unneeded whitespace at the end of the comment.
>    */
>   _shouldSkipNode: function PC_shouldSkipNode(node, pastFolders) {
>     /**
>-     * Determines if a node is contained by another node within a resultset.
>+     * Determines if a node is contained by another node within a resultset. 
Nit: unneeded whitespace at the end of the comment.

>-      let xferable = Components.classes["@mozilla.org/widget/transferable;1"]
>-                               .createInstance(Components.interfaces.nsITransferable);
>+      let xferable =  Components.classes["@mozilla.org/widget/transferable;1"]
>+                                .createInstance(Components.interfaces.nsITransferable);
Nit: unneeded extra space after = and in line below.

>-      // This order is _important_! It controls how this and other applications
>+      // This order is _important_! It controls how this and other applications 
Nit: unneeded whitespace at the end of the comment.

>+XPCOMUtils.defineLazyServiceGetter(PlacesControllerDragHelper, "dragService",
>+                                   "@mozilla.org/widget/dragservice;1",
>+                                   "nsIDragService");
>+
I prefer this at towards the top of the file, but I'll not fall out about it.

>diff --git a/suite/common/history/sidebarUtils.js b/suite/common/places/sidebarUtils.js
>-      window.top.XULBrowserWindow.setOverLink("", null);
>+    window.top.XULBrowserWindow.setOverLink("", null);  
Nit: unneeded whitespace at the end of the line.

I've only highlighted some of the whitespaces at the end of the lines, but there are a LOT more.
There is also an inconsistency on the formatting of the brackets/braces with try/catch, could that be fixed too.

r=me with the above addressed.

Comment 14

[Ian Neal]

Comment on attachment 459033 [details] [diff] [review]
part 3, v2: tree

>diff --git a/suite/common/history/tree.xml b/suite/common/places/tree.xml
> 
> </bindings>
>+
Nit: Remove this unneeded additional line.

>diff --git a/suite/common/history/treeView.js b/suite/common/places/treeView.js
>-function PlacesTreeView() {
>-  this._tree = null;
>-  this._result = null;
>-  this._selection = null;
>-  this._rows = [];
>-  this._rootNode = null;
>-}
I'd be happier keeping this at the start of the file than moving it to the end.

> PlacesTreeView.prototype = {
>   _makeAtom: function PTV__makeAtom(aString) {
>-    return Components.classes["@mozilla.org/atom-service;1"]
>-                     .getService(Components.interfaces.nsIAtomService)
>-                     .getAtom(aString);
>+    return  Components.classes["@mozilla.org/atom-service;1"]
>+                    .getService(Components.interfaces.nsIAtomService)
>+                    .getAtom(aString);
Nit: no longer aligned.

>   },
> 
>   _atoms: [],
>   _getAtomFor: function PTV__getAtomFor(aName) {
>     if (!this._atoms[aName])
>       this._atoms[aName] = this._makeAtom(aName);
> 
>     return this._atoms[aName];
>   },
> 
>   __dateService: null,
>   get _dateService() {
>     if (!this.__dateService) {
>       this.__dateService = Components.classes["@mozilla.org/intl/scriptabledateformat;1"]
>-                                     .getService(Components.interfaces.nsIScriptableDateFormat);
>+                                   .getService(Components.interfaces.nsIScriptableDateFormat);
Nit: no longer aligned.

>+    let resultType = aContainer.queryOptions.resultType;
>+    switch (resultType) {
>+      case Components.interfaces.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY:
>+      case Components.interfaces.nsINavHistoryQueryOptions.RESULTS_AS_SITE_QUERY:
>+      case Components.interfaces.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY:
>+      case Components.interfaces.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY:
>+        return aContainer._plainContainer = false;
We only seem to use resultType the once, so just inline it.

>+    let dateFormat = timeMs >= midnight ?
>+                      Components.interfaces.nsIScriptableDateFormat.dateFormatNone :
>+                      Components.interfaces.nsIScriptableDateFormat.dateFormatShort;
> 
>+    let timeObj = new Date(timeMs);
>     return (this._dateService.FormatDateTime("", dateFormat,
>       Components.interfaces.nsIScriptableDateFormat.timeFormatNoSeconds,
>       timeObj.getFullYear(), timeObj.getMonth() + 1,
>       timeObj.getDate(), timeObj.getHours(),
>       timeObj.getMinutes(), timeObj.getSeconds()));
Is it worth setting a variable to Components.interfaces.nsIScriptableDateFormat?

>+    // Bail out for hidden separators.
>+    if (PlacesUtils.nodeIsSeparator(aNode) &&
>+        this._result.sortingMode != Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE)
>+      return;
Can't we use isSorted here?

>+      let cc = aParentNode.childCount;
>+      let separatorsAreHidden = PlacesUtils.nodeIsSeparator(aNode) &&
>+        this._result.sortingMode != Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
and here? (is Ci even defined?)

>+    // Bail out for hidden separators.
>+    if (PlacesUtils.nodeIsSeparator(aNode) &&
>+        this._result.sortingMode != Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE)
>+      return;
and here?

>+    // Bail out for hidden separators.
>+    if (PlacesUtils.nodeIsSeparator(aNode) &&
>+        this._result.sortingMode != Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE)
>+      return;
and here?

>+    let columnType = this._getColumnType(aColumn);
>+    switch (columnType) {
>+      case this.COLUMN_TYPE_TITLE:
>+        // normally, this is just the title, but we don't want empty items in
We only seem to use columnType the once, so just inline it.

>   setTree: function PTV_setTree(aTree) {
>+    let hasOldTree = this._tree != null;
>     this._tree = aTree;
> 
>     if (this._result) {
>       if (hasOldTree) {
>         // detach from result when we are detaching from the tree.
>         // This breaks the reference cycle between us and the result.
>+        if (!aTree) {
>+          this._result.removeObserver(this);
>+          this._rootNode.containerOpen = false;
>+        }
>       }
>       if (aTree)
>         this._finishInit();
>     }
Is it possible to set this._tree = aTree later in the function, then you could inline hasOldTree?

>+    let columnType = this._getColumnType(aColumn);
>+    switch (columnType) {
>+      case this.COLUMN_TYPE_TITLE:
>         if (oldSort == NHQO.SORT_BY_TITLE_ASCENDING)
We only seem to use columnType the once, so just inline it.

r=me with those addressed.

Comment 15

[Ian Neal]

Comment on attachment 459038 [details] [diff] [review]
part 4, v2: toolbar and menu files

>diff --git a/suite/common/places/browserPlacesViews.js b/suite/common/places/browserPlacesViews.js

>+  get insertionPoint() {
There seems to be a lot of code duplication between here and places/tree.xml is that part of a future code clean-up bug?

>diff --git a/suite/common/places/menu.xml b/suite/common/places/menu.xml
I presume this a direct copy of /mozilla/browser/components/places/content/menu.xml?

r=me with those addressed.

Overall, comparing against the toolkit versions of the files, the main difference other than their use of Ci and Cc, is the use of let vs var - any reason why you've not done it consistently in these patches, you seem to have done for some files and not others?

Comment 16

[Robert Kaiser]

(In reply to comment #11)
> (In reply to comment #7)
> > Looks like we DO setup (above) History as a valid query, but we don't actually
> > CREATE the history query. I think we should have this here.
> 
> Need to look into that

Actually, this is the code that loads the "left pane" of the bookmarks manager, so not having the History query being set up there is really the intention.

Comment 17

[Robert Kaiser]

(In reply to comment #12)
> Comment on attachment 459031 [details] [diff] [review]
> part1, v2: PlacesUIUtils
> 
> >diff --git a/suite/common/history/utils.js b/suite/common/src/PlacesUIUtils.jsm
> >+XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() {
> >+  Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> >+  return PlacesUtils;
> > });
> Any reason why the rest of the lazy getters are not next to this one?

Actually, reading the code more closely, I think this one is the only one meant to be defined globally in the future as well (and I think those defining a getter on PlacesUIUtils might need that object defined first) - and this one is the only one doing an import. It seems to be common style in the places stuff to have the normal lazy getters at the end, but it's of course the usual thing to have imports at the top.

> >+  _wrapString: function PUIU__wrapString(aString) {
> It would be useful to have something like this on a more global level (I count
> about 5 or so places which use something similar), but that is outside the
> scope of this bug.

As you know what exactly you mean with that, could you file the followup for that, please?

> Shouldn't we be using the transactions objects exported by the PlacesUtils.jsm
> module (in this case PlacesCreateBookmarkTransaction) directly rather than
> using the compatibility shim? Lots of other uses of ptm in this patch too.
> Probably a follow-up bug though.

Yes, the places team has plans to rework the transactions stuff to be nicer and do away with the ptm shim they created for moving this into PlacesUtils as seamlessly as possible. I'll follow their progress there closely and port over their improvements. As noted on IRC, they are trying to improve and clean up the code they inherited.

> >+   * Methods to show the bookmarkProperties dialog in its various modes.
> Seems to be a lot of code duplication within the various methods, but again
> probably worthy of a follow-up bug.

The whole bookmarks properties stuff is another of those things they have told me they are planning to clean up.
And, BTW, this all isn't just talk, they already did some good cleanups and improvements in this cycle, but I think most of those cleanups will probably go into an upcoming development cycle.

> >-  checkURLSecurity: function PU_checkURLSecurity(aURINode) {
> >+  checkURLSecurity: function PUIU_checkURLSecurity(aURINode, aWindow) {
> The function has an extra param but it is not mentioned in the comment.

Thanks, good catch! I knew there's a reason to show enough context in patches. ;-)

> Can we not use openUILinkArrayIn as we did in utils.js?

Oh, yes, we can, when browserWindow is defined, and go the "window" branch when it's not. Maybe we should port openUILinkArrayIn() to Firefox so they can as well? ;-)

Comment 18

[Robert Kaiser]

(In reply to comment #13)
> I've only highlighted some of the whitespaces at the end of the lines, but
> there are a LOT more.
> There is also an inconsistency on the formatting of the brackets/braces with
> try/catch, could that be fixed too.

Thanks, did a search through all of controller.js and fixed all places that had this. I also fixed trailing whitespace in PUIU (part 1), and did a Firefox patch for those as well (in bug 570788).

Comment 19

[Robert Kaiser]

(In reply to comment #14)

> >diff --git a/suite/common/history/treeView.js b/suite/common/places/treeView.js
> >-function PlacesTreeView() {
> >-  this._tree = null;
> >-  this._result = null;
> >-  this._selection = null;
> >-  this._rows = [];
> >-  this._rootNode = null;
> >-}
> I'd be happier keeping this at the start of the file than moving it to the end.

I'd like to be consistent with Firefox here, let's see what they say when I propose to move it there.

> Is it worth setting a variable to
> Components.interfaces.nsIScriptableDateFormat?

Here's why Ci.* is so much nicer in the FF case. ;-)

> >+    // Bail out for hidden separators.
> >+    if (PlacesUtils.nodeIsSeparator(aNode) &&
> >+        this._result.sortingMode != Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE)
> >+      return;
> Can't we use isSorted here?

Nice!

> >   setTree: function PTV_setTree(aTree) {
> >+    let hasOldTree = this._tree != null;
> >     this._tree = aTree;
> > 
> >     if (this._result) {
> >       if (hasOldTree) {
> >         // detach from result when we are detaching from the tree.
> >         // This breaks the reference cycle between us and the result.
> >+        if (!aTree) {
> >+          this._result.removeObserver(this);
> >+          this._rootNode.containerOpen = false;
> >+        }
> >       }
> >       if (aTree)
> >         this._finishInit();
> >     }
> Is it possible to set this._tree = aTree later in the function, then you could
> inline hasOldTree?

Hmm, that's a hard one because I don't want to change the logic here, and this this._result if is in between. Isn't that call cheap enough that we shouldn't bother too much?

Comment 20

[Robert Kaiser]

(In reply to comment #15)
> Comment on attachment 459038 [details] [diff] [review]
> part 4, v2: toolbar and menu files
> 
> >diff --git a/suite/common/places/browserPlacesViews.js b/suite/common/places/browserPlacesViews.js
> 
> >+  get insertionPoint() {
> There seems to be a lot of code duplication between here and places/tree.xml is
> that part of a future code clean-up bug?

You mean between this and treeViews.js, I guess - not sure if the places team has planned something here, but it's relatively clear that this new browserPlacesViews.js has been very much modeled after the tree view to at least reduce the differences between the tree and toolbar/menu code. You're right that we could go further with this and merge good parts of them.

> >diff --git a/suite/common/places/menu.xml b/suite/common/places/menu.xml
> I presume this a direct copy of
> /mozilla/browser/components/places/content/menu.xml?

With the usual Ci replacement and getting rid of the preprocessed license header, yes.

> Overall, comparing against the toolkit versions of the files, the main
> difference other than their use of Ci and Cc, is the use of let vs var - any
> reason why you've not done it consistently in these patches, you seem to have
> done for some files and not others?

I first thought I need to align us with Neil's style somewhat, but gave up fast due to my concern of making future merges much harder. Also, I did not follow some var/let cleanup the Firefox places team did on their side, maybe I should still do that.

Thanks a ton for those reviews, you made my day!

Comment 21

[Robert Kaiser]

After checking with Neil, we have a go to push those and the rest of the places stuff!

Pushed core as:
http://hg.mozilla.org/comm-central/rev/6e9656086f33
http://hg.mozilla.org/comm-central/rev/f97acf98fde7
http://hg.mozilla.org/comm-central/rev/6140689b3faf
http://hg.mozilla.org/comm-central/rev/874e4cd85e20

Comment 22

[Robert Kaiser]

(In reply to comment #10)
> >+++ suite/common/places/controller.js	2010-07-29 13:55:50.181047677 +0200
> >+// XXX: should be defined more globally
> >+var TAB_DROP_TYPE = "application/x-moz-tabbrowser-tab";
> >+
> 
> FYI Firefox defines this in utilityOverlay, (solving the defined more globally)
> and because of that it gets used from there _and_ in their tabbrowser impl. We
> probably need a tabbrowser bit of code on our end for this |type| as well. But
> easily done in a followup.

Hrm, need this after all for now see bug 585500.