Currently SeaMonkey has the files suite/common/history/utils.js and suite/common/src/PlacesUIUtils.jsm. Both basically provide the PlacesUIUtils module, the utils.js one provides a simplified version of this (probably due to historic reasons). utils.js can probably go away and code using it should use the PlacesUIUtils.jsm module code.

See also Bug 560104 on this, there utils.js was changed to return some dummy PlacesUtils object (as it was available via utils.js). It looks like only placesOverlay.xul is using utils.js.

> <menupopup id="placesContext" > - onpopupshowing="this._view = PlacesUIUtils.getViewForNode(document.popupNode); > + onpopupshowing="this._view = PlacesUIUtils.historyGetViewForNode(document.popupNode); .... > + // XXXRatty: Temporary compat method for history view. Remove when porting Bug 528884 > + // to History. > + historyGetViewForNode: function (aNode) { .... History was using an older version of getViewForNode. I didn't want to port all of Bug 528884 as well so I created a temporary (I hope) fork of that function. > <menuitem id="addBookmarkContextItem" > label="&bookmarkLinkCmd.label;" > accesskey="&bookmarkLinkCmd.accesskey;" > selectiontype="single" > selection="link" > oncommand="historyAddBookmarks();"/> > - <menuitem id="addBookmarkContextItem" > + <menuitem id="addBookmarkContextItems" Having two menuitems witht the same ID means that neither will be visible in the context menu (a Neil typo). > rename from suite/common/src/PlacesUIUtils.jsm > rename to suite/common/places/PlacesUIUtils.jsm Not sure why this was in suite/common/src/. The most logical place would be suite/modules/. The second most logical place would be with the other /places/ files. > - return new PlacesAggregatedTransactions("addTags", transactions); > + return new PlacesAggregatedTransaction("addTags", transactions); Another typo. > - for (var i=0; i < aNodes.length; i++) { > + for (var i = 0; i < aNodes.length; i++) { Re-fix something that was fixed in history/utils.js > openNodeIn: function PUIU_openNodeIn(aNode, aWhere) { > - if (aNode && PlacesUtils.nodeIsURI(aNode) && > - this.checkURLSecurity(aNode, this._getCurrentActiveWin())) { > + var win = this._getCurrentActiveWin(); > + if (aNode && PlacesUtils.nodeIsContainer(aNode)) { > + if (aWhere != "current") { > + this.openContainerNodeInTabs(aNode, aWhere); > + } else if (this.checkURLSecurity(aNode, win)) { > + this.markPageAsTyped(aNode.uri); > + win.openUILinkIn(aNode.uri, aWhere); > + } > + } else if (aNode && PlacesUtils.nodeIsURI(aNode) && > + this.checkURLSecurity(aNode, win)) { > var isBookmark = PlacesUtils.nodeIsBookmark(aNode); > > if (isBookmark) > this.markPageAsFollowedBookmark(aNode.uri); > else > this.markPageAsTyped(aNode.uri); > > - this._getCurrentActiveWin().openUILinkIn(aNode.uri, aWhere); > + win.openUILinkIn(aNode.uri, aWhere); I tried to reconcile the history and places version of openNodeIn(). Not sure if this is correct.

Fixed typo.

Curious question on this change: - return new PlacesAggregatedTransactions("addTags", transactions); + return new PlacesAggregatedTransaction("addTags", transactions); So this code was always(?) wrong? As I don't see any mention of "PlacesAggregatedTransactions" in mxr search. But then I'm not really sure if this code was actively used/called by other code.

Comment on attachment 8530311 [details] [diff] [review] Patch v1.1 Proposed fix This patch is confusing as it appears to merge several unrelated or partially related fixes. >- if (asQuery(node.parent).queryOptions.resultType == >+ if (PlacesUtils.asQuery(node.parent).queryOptions.resultType == >- let suffix = i < (nodes.length - 1) ? NEWLINE : ""; >+ let suffix = i < (nodes.length - 1) ? PlacesUtils.endl : ""; [These fixes belong in a separate patch.] >- // HACK: as we're importing the actual PlacesUIUtils but that name is taken >- // by a cut-down history-specific version, store that latter one temporarily >- var HistoryUtils = PlacesUIUtils; >- Components.utils.import("resource:///modules/PlacesUIUtils.jsm"); [Huh, I wonder why we didn't use a temporary scope here.] >+ <script type="application/javascript"><![CDATA[ >+ Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); >+ Components.utils.import("resource:///modules/PlacesUIUtils.jsm"); >+ ]]></script> Eww. Can these be moved to an appropriate script (i.e. one that uses them)? >- onpopupshowing="this._view = PlacesUIUtils.getViewForNode(document.popupNode); >+ onpopupshowing="this._view = PlacesUIUtils.historyGetViewForNode(document.popupNode); [Bikeshedding: getViewForHistoryNode perhaps?] >+EXTRA_JS_MODULES += [ >+ 'places/PlacesUIUtils.jsm', >+] [Other modules live in suite/modules] >+// PlacesUtils exposes multiple symbols, so we can't use defineLazyModuleGetter. [Which ones are we using?] > openNodeIn: function PUIU_openNodeIn(aNode, aWhere) { >- if (aNode && PlacesUtils.nodeIsURI(aNode) && >- this.checkURLSecurity(aNode, this._getCurrentActiveWin())) { >+ var win = this._getCurrentActiveWin(); >+ if (aNode && PlacesUtils.nodeIsContainer(aNode)) { >+ if (aWhere != "current") { >+ this.openContainerNodeInTabs(aNode, aWhere); >+ } else if (this.checkURLSecurity(aNode, win)) { >+ this.markPageAsTyped(aNode.uri); >+ win.openUILinkIn(aNode.uri, aWhere); >+ } >+ } else if (aNode && PlacesUtils.nodeIsURI(aNode) && >+ this.checkURLSecurity(aNode, win)) { This looks overly complicated. Can't you do something like if (aNode && PlacesUtils.nodeIsContainer(aNode) && aWhere != "current") this.openContainerNodeInTabs(aNode, aWhere); else if (aNode && PlacesUtils.nodeIsURI(aNode) && this.checkURLSecurity(aNode, win)) ... >+XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "clipboard", >+ "@mozilla.org/widget/clipboard;1", >+ "nsIClipboard"); Where is this used? >diff --git a/suite/locales/en-US/chrome/common/history/places.properties b/suite/locales/en-US/chrome/common/history/places.properties >deleted file mode 100644 [Which source change makes this possible?]

(In reply to Frank Wein [:mcsmurf] from comment #4) > Curious question on this change: > - return new PlacesAggregatedTransactions("addTags", transactions); > + return new PlacesAggregatedTransaction("addTags", transactions); > > So this code was always(?) wrong? As I don't see any mention of > "PlacesAggregatedTransactions" in mxr search. But then I'm not really sure > if this code was actively used/called by other code. http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/places/PlacesUIUtils.jsm?rev=a3d02e44a476#1256 > * This Is A Compatibility Shim For Old Puiu.Ptm Users. > * > * If You'Re Looking For Transactions And Writing New Code Using Them, Directly > * Use The Transactions Objects Exported By The Placesutils.Jsm Module. > * > * This Object Will Be Removed Once Enough Users Are Converted To The New Api. This has been in Firefox for ages. If there are no more consumers of this API this can be killed everywhere (Firefox, SeaMonkey)

(In reply to Frank Wein [:mcsmurf] from comment #4) > So this code was always(?) wrong? As I don't see any mention of > "PlacesAggregatedTransactions" in mxr search. But then I'm not really sure > if this code was actively used/called by other code. Well there is https://hg.mozilla.org/comm-central/rev/fb6446c0d84f by someone called "Frank Wein" (See Bug 732027 - Port |Bug 575955 - Replace internal usage of old transactions shim)

(In reply to Philip Chee from comment #7) > (In reply to Frank Wein [:mcsmurf] from comment #4) > > > So this code was always(?) wrong? As I don't see any mention of > > "PlacesAggregatedTransactions" in mxr search. But then I'm not really sure > > if this code was actively used/called by other code. > > Well there is https://hg.mozilla.org/comm-central/rev/fb6446c0d84f by > someone called "Frank Wein" > (See Bug 732027 - Port |Bug 575955 - Replace internal usage of old > transactions shim) But I never added something like "PlacesAggregatedTransactions"...?

> This patch is confusing as it appears to merge several unrelated or partially related fixes. >>- if (asQuery(node.parent).queryOptions.resultType == >>+ if (PlacesUtils.asQuery(node.parent).queryOptions.resultType == >>- let suffix = i < (nodes.length - 1) ? NEWLINE : ""; >>+ let suffix = i < (nodes.length - 1) ? PlacesUtils.endl : ""; STR: Replace history/utils.js with PlacesUIUtils. Watch Error Console getting spammed. Fix errors. Watch Error Console still getting spammed. Keep fixing errors until the Error Console stops complaining. Unlike history/utils.js, our PlacesUIUtils.jsm does not expose global symbols asQuery(), NEWLINE, etc. > [These fixes belong in a separate patch.] I disagree. >>+ <script type="application/javascript"><![CDATA[ >>+ Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); >>+ Components.utils.import("resource:///modules/PlacesUIUtils.jsm"); >>+ ]]></script> > Eww. Can these be moved to an appropriate script (i.e. one that uses them)? Moved to history/history.js >>- onpopupshowing="this._view = PlacesUIUtils.getViewForNode(document.popupNode); >>+ onpopupshowing="this._view = PlacesUIUtils.historyGetViewForNode(document.popupNode); > [Bikeshedding: getViewForHistoryNode perhaps?] Reading through the history sources again I see that History doesn't have any menu type="places" only tree type="places". I think we can get away without having to fork getViewForNode() >>+EXTRA_JS_MODULES += [ >>+ 'places/PlacesUIUtils.jsm', >>+] > [Other modules live in suite/modules] OK. Moved. >>+// PlacesUtils exposes multiple symbols, so we can't use defineLazyModuleGetter. > [Which ones are we using?] I copied this from current Firefox code. Appears to be part of Bug 1067954 - Async Places Transactions Which we don't have at the moment. http://hg.mozilla.org/mozilla-central/diff/6ef192784187/browser/components/places/PlacesUIUtils.jsm#l1.23 http://hg.mozilla.org/mozilla-central/diff/6ef192784187/browser/components/places/PlacesUIUtils.jsm#l1.41 Asaf appears to have imported this twice: http://hg.mozilla.org/mozilla-central/diff/6ef192784187/browser/components/places/PlacesUIUtils.jsm#l1.12 >> openNodeIn: function PUIU_openNodeIn(aNode, aWhere) { >>- if (aNode && PlacesUtils.nodeIsURI(aNode) && >>- this.checkURLSecurity(aNode, this._getCurrentActiveWin())) { >>+ var win = this._getCurrentActiveWin(); >>+ if (aNode && PlacesUtils.nodeIsContainer(aNode)) { >>+ if (aWhere != "current") { >>+ this.openContainerNodeInTabs(aNode, aWhere); >>+ } else if (this.checkURLSecurity(aNode, win)) { >>+ this.markPageAsTyped(aNode.uri); >>+ win.openUILinkIn(aNode.uri, aWhere); >>+ } >>+ } else if (aNode && PlacesUtils.nodeIsURI(aNode) && >>+ this.checkURLSecurity(aNode, win)) { > This looks overly complicated. Can't you do something like > if (aNode && PlacesUtils.nodeIsContainer(aNode) && aWhere != "current") > this.openContainerNodeInTabs(aNode, aWhere); > else if (aNode && PlacesUtils.nodeIsURI(aNode) && this.checkURLSecurity(aNode, win)) ... OK. Simplified. >>+XPCOMUtils.defineLazyServiceGetter(PlacesUIUtils, "clipboard", >>+ "@mozilla.org/widget/clipboard;1", >>+ "nsIClipboard"); > Where is this used? In controller.js I've moved it there. >>diff --git a/suite/locales/en-US/chrome/common/history/places.properties b/suite/locales/en-US/chrome/common/history/places.properties >>deleted file mode 100644 > [Which source change makes this possible?] This one: > diff --git a/suite/common/history/utils.js b/suite/common/history/utils.js > deleted file mode 100644 > --- a/suite/common/history/utils.js > +++ /dev/null ................................. > <menuitem id="addBookmarkContextItem" > label="&bookmarkLinkCmd.label;" > accesskey="&bookmarkLinkCmd.accesskey;" > selectiontype="single" > selection="link" > oncommand="historyAddBookmarks();"/> > - <menuitem id="addBookmarkContextItem" > + <menuitem id="addMultipleBookmarksContextItem" This is one of Neils "improvements" to history not found in Firefox. Probably from our pre-places history code. The other menu items are all like "placesContext_foobar". How about: "placesContext_addBookmarks" "placesContext_add:bookmarks" "placesContext_addSelectionToBookmarks" "placesContext_add:selectionToBookmarks" > openURINodesInTabs: function PUIU_openURINodesInTabs(aNodes, aEvent) { > + let where = this._getCurrentActiveWin() > + .whereToOpenLink(aEvent, false, true); > + this.openSelectionIn(aNodes, where); > + }, > + > + openSelectionIn: function PUIU_openSelectionIn(aNodes, aWhere) { openSelectionIn() is one of Neils "improvements" to history that mysteriously disappeared when history/utils.js was copied to places/PlacesUIUtils.jsm.

Comment on attachment 8539716 [details] [diff] [review] Patch v2.0 address review issues. >+ this.clipboard.setData(xferable, null, >+ this.clipboard.kGlobalClipboard); (Is this the only use? I don't think it's worth a lazy getter just for this. I'd either get the clipboard here or use Services.clipboard if it's already in scope.) >-XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() { >- Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); >- return PlacesUtils; >-}); >+// PlacesUtils exposes multiple symbols, so we can't use defineLazyModuleGetter. >+Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); (Still don't have to change this if we're not using the multiple symbols.) >+ let where = this._getCurrentActiveWin() >+ .whereToOpenLink(aEvent, false, true); _getCurrentActiveWin returns null if it can't find one. Might be easier to tweak openTabset to accept an optional aWhere parameter.

(In reply to Philip Chee from comment #9) > > [These fixes belong in a separate patch.] > I disagree. Confusion arose because bug says consolidate utils.js and PlacesUIUtils.jsm; no mention of PlacesUtils.

(In reply to neil@parkwaycc.co.uk from comment #11) > (In reply to Philip Chee from comment #9) >>> [These fixes belong in a separate patch.] > > I disagree. > Confusion arose because bug says consolidate utils.js and PlacesUIUtils.jsm; > no mention of PlacesUtils. When Firefox changed history/utils.js to PlacesUIUtils.jsm, some globals were moved to toolkit/PlacesUtils.jsm

>>+ this.clipboard.setData(xferable, null, >>+ this.clipboard.kGlobalClipboard); > (Is this the only use? I don't think it's worth a lazy getter just for > this. I'd either get the clipboard here or use Services.clipboard if it's > already in scope.) Now using Services.clipboard. >>-XPCOMUtils.defineLazyGetter(this, "PlacesUtils", function() { >>- Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); >>- return PlacesUtils; >>-}); >>+// PlacesUtils exposes multiple symbols, so we can't use defineLazyModuleGetter. >>+Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > (Still don't have to change this if we're not using the multiple symbols.) Reverted. >>+ let where = this._getCurrentActiveWin() >>+ .whereToOpenLink(aEvent, false, true); > _getCurrentActiveWin returns null if it can't find one. Might be easier > to tweak openTabset to accept an optional aWhere parameter. I've made _openTabset accept an optional aWhere parameter. > <menuseparator id="placesContext_openSeparator"/> > <menuitem id="addBookmarkContextItem" > label="&bookmarkLinkCmd.label;" > accesskey="&bookmarkLinkCmd.accesskey;" > selectiontype="single" > selection="link" > oncommand="historyAddBookmarks();"/> > - <menuitem id="addBookmarkContextItem" > + <menuitem id="addBookmarkContextItem" I've changed the second "addBookmarkContextItem" to "addBookmarkContextItem" because |selectiontype="multiple"| Also should I put these two items in controller.js?

Comment on attachment 8543166 [details] [diff] [review] Patch v3.0 more fixes. >- for (var i = 0; i < aItemsToOpen.length; i++) { >- var item = aItemsToOpen[i]; >+ for (let item of aItemsToOpen) { [Bah.] >+ let where = aWhere || browserWindow.whereToOpenLink(aEvent, false, true); [Humbug.] >+ var win = this._getCurrentActiveWin(); [Phew!]

(In reply to Philip Chee from comment #13) > Also should I put these two items in controller.js? I think that's only needed for items that might be disabled.

Pushed to comm-central http://hg.mozilla.org/comm-central/rev/c72f18c8c604