Closed Bug 759260 Opened 12 years ago Closed 6 years ago

Don't load placesOverlay.xul during browser window open


(Firefox :: Bookmarks & History, defect, P3)




Firefox 61


(Reporter: asaf, Unassigned)



(Whiteboard: [fxsearch])

The places overlay provides the browser window (and other windows, but that's unimportant for the purpose of this bug) with the places commands and the places context menu. Given that the the places context menu is the only way to activate these commands, this means that most of the time there's absolutely no point for loading the places-overlay during browser window open.

At the very least its loading could be delayed to the the point where a places view is used (bookmarks or history menu is opened, or bookmarks toolbar is visible). Even better would be to only load it once the context menu is opened. That might be tricky, though, given that load-dynamic-overlay loads overlays asynchronously.
Summary: Figure out if we can avoid loading placesOverlay.xul during browser window open → Don't load placesOverlay.xul during browser window open
Depends on: 406371
Priority: -- → P2
Whiteboard: [fxsearch]
Depends on: 1439313
Depends on: 1439315
Priority: P2 → P3
Bdahl, I heard you are working on placesOverlay removal, so you may be interested in the work I'm doing in the dependencies here for treeview.js and controller.js.
I read some of your comments, and I agree the dependencies situation is not great, placesOverlay ended up defining things that other components immediately started using, since its pretty much included in every window that may contain a Places view (most windows indeed), even if it wasn't for them (Cc,Ci,Cu,NetUtil are some of the examples).
Some of that cruft has been cleaned up finally, by the dependencies here.

I'd love if you could link this bug to the ones you are working on for its removal, or just feel free to reuse this one, we don't have plans for further work in this area short term.
Flags: needinfo?(bdahl)
I just started removing places overlay yesterday and filed bug 1442302. In my initial patch[1] I've basically split placesOverlay into four include files:,,, Also, see the bug for a graph of all the dependencies.

If we cleaned up the script dependencies could probably be removed and we could just directly include one script.

Flags: needinfo?(bdahl)
I just landed bug 1439315, so at the next merge placesOverlay.xul scripts part will look like this:

  <script type="application/javascript"
  <script type="application/javascript"
  <script type="application/javascript"><![CDATA[
      "PlacesUtils", "resource://gre/modules/PlacesUtils.jsm");
      "PlacesUIUtils", "resource:///modules/PlacesUIUtils.jsm");
      "PlacesTransactions", "resource://gre/modules/PlacesTransactions.jsm");
      "ForgetAboutSite", "resource://gre/modules/ForgetAboutSite.jsm");

    XPCOMUtils.defineLazyScriptGetter(window, "PlacesTreeView",
      ["PlacesInsertionPoint", "PlacesController", "PlacesControllerDragHelper"],
Depends on: 1442302
there are a lot of windows in need for these places defines:
The browser window for sure needs everything in placesOverlay
The library window likely won't use browserPlacesViews.js and the tooltip, but will use all the rest
the native Mac menubar has the legacy bookmarks menu, thus it won't use treeview.js, but needs the rest
the sidebar window has Places treeviews, it won't use browserPlacesViews.js and the tooltip but needs the rest
some dialog windows like the Properties dialog (right click on bookmark and properties) have a tree places view, won't use browserPlacesViews.js and the tooltip, it *may* try to use commands, probably not the context menu
well, this is no more useful since placesOverlay is gone \o/
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.