759260 - Don't load placesOverlay.xul during browser window open

Status: RESOLVED FIXED

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

Description

[Asaf Romano (gone)]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Brendan Dahl [: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: bhTooltip.inc.xul, placesCommands.inc.xul, placesContextMenu.inc.xul, placesScripts.inc.xul. Also, see the bug for a graph of all the dependencies.

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


[1] https://hg.mozilla.org/try/rev/940f35c855cadeb1554baa6ecb7555372a57a3ac

Comment 3

[Marco Bonardo [:mak]]

I just landed bug 1439315, so at the next merge placesOverlay.xul scripts part will look like this:

  <script type="application/javascript"
          src="chrome://global/content/globalOverlay.js"/>
  <script type="application/javascript"
          src="chrome://browser/content/utilityOverlay.js"/>
  <script type="application/javascript"><![CDATA[
    ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
    ChromeUtils.defineModuleGetter(window,
      "PlacesUtils", "resource://gre/modules/PlacesUtils.jsm");
    ChromeUtils.defineModuleGetter(window,
      "PlacesUIUtils", "resource:///modules/PlacesUIUtils.jsm");
    ChromeUtils.defineModuleGetter(window,
      "PlacesTransactions", "resource://gre/modules/PlacesTransactions.jsm");
    ChromeUtils.defineModuleGetter(window,
      "ForgetAboutSite", "resource://gre/modules/ForgetAboutSite.jsm");

    XPCOMUtils.defineLazyScriptGetter(window, "PlacesTreeView",
      "chrome://browser/content/places/treeView.js");
    XPCOMUtils.defineLazyScriptGetter(window,
      ["PlacesInsertionPoint", "PlacesController", "PlacesControllerDragHelper"],
      "chrome://browser/content/places/controller.js");
  ]]></script>

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Marco Bonardo [:mak]]

well, this is no more useful since placesOverlay is gone \o/