Closed Bug 1442302 Opened 3 years ago Closed 3 years ago

Remove placesOverlay.xul

Categories

(Firefox :: Bookmarks & History, enhancement, P2)

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(2 files)

before.svg
20.18 KB, image/svg+xml
Details
Bug 1442302 - Remove placesOverlay.xul.
59 bytes, text/x-review-board-request
Gijs
: review+
standard8
: review+
Details
Attached image before.svg — 
Similar to baseMenuOverlay and editMenuOverlay this overlay could be removed by splitting up placesOvleray and inlining or using includes.

Some things that stand out:
- several files use the overlay but no elements, e.g. they probably just using the scripts loaded (webext-panels.xul, selectBookmark.xul, bookmarkProperties.xul, web-panels.xul)
- it doesn't look like this will reduce as nicely as baseMenuOverlay since browser-sets.inc, bookmarksPanel.xul, and history-panel.xul use placesCommands.

I've attached a flow of the elements in the overlay.
- red = MacOS only
- green - NON-MacOS
- blue - overlay is used in
Blocks: 759260
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review230786

::: browser/components/places/content/placesCommands.inc.xul:1
(Diff revision 1)
> +<commandset id="placesCommands"

I was hoping to collapse this into the mainCommandSet like we did for the baseMenuCommands, but there is a dependency on the placesCommands in bookmarksPanel.xul and history-panel.xul.
Brendan, this needs the ESLint configuration updating. ESLint has special handling for places-overlay and browser-window, so we need that to land and update at the same time, otherwise no-undef is likely to allow too many globals.

https://searchfox.org/mozilla-central/search?q=places-overlay&case=false&regexp=false&path=

Having taken only a quick look at your patch, I think this comes down to:

- Remove the places-overlay environment
- Update the mappings in browser-window.js for the updated global-scripts.inc
- Where files are imported via <script> tags, we'll probably need to update some other scripts to use /* import-globals-from ... */, e.g. selectBookmark.js will likely need these defining.
Flags: needinfo?(bdahl)
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review230928

I think the approach here is generally fine, and the XUL bits look OK to me. The only thing I'm not sure about is the creation of the placesModules.js file here and how we deal with scripts. If a lot of the tests need this, I think we should have a test-only file. Otherwise, it seems like you're currently adding this to:

- browser.xul and the hidden window (via global-scripts.inc). Does the hidden window really need places tree controller code? I'd kind of expect not...
- web-panels.xul (ie the sidebar document). Because the actual sidebar loads in a `<browser>` inside it, I'm not convinced this needs all the places-related code.
- webext-panels.xul (sidebar document for webextension panels). Similar issue.
- bookmarkProperties.xul (bookmark properties edit dialog). While I'm sure this needs some places code, I'd be surprised if it needed everything in placesModules.js, and/or if the lazy getters are worth it for this usecase
- bookmarksPanel.xul and history-panel.xul (bookmark and history sidebar documents). I guess this probably needs all of the scripts.
- places.xul (library window). I guess this probably needs all of these as well, but you're adding them via global-scripts.inc and macBrowserOverlay.xul already on macOS!
- selectBookmark.xul (bookmark picker for the prefs). I expect this needs at least some of these scripts, though I wonder if there's really a point to loading them lazily.

I have 2 concerns about this pattern. 1 is that, given the list I think we're probably including more stuff than we need to. The other is that the contents of placesModules.js create 2 lazy script loaders, so effectively we've now swapped a lazily-inserted script for a non-lazily-inserted script. And while I'm sure that at least some of the cost of loading the script is proportional to its size, I'd expect various bits of overhead to mean that this isn't always a great trade-off.

Of course, any kind of perf/bloat concern I have here may be overthinking things. After all, we've always loaded more or less all this stuff, and abstractions that ensure that we have everything we need in all these files are good (and duplicating the code around isn't necessarily very good). But still, I'd like Marco to take a look at this. He knows places a lot better than me, and maybe he can offer more concrete suggestions about how to deal with the places-related scripts that used to be 'inline' in placesOverlay.xul...

::: browser/base/content/webext-panels.xul:27
(Diff revision 1)
>    <script type="application/javascript" src="chrome://browser/content/browser.js"/>
>    <script type="application/javascript" src="chrome://browser/content/browser-places.js"/>
>    <script type="application/javascript" src="chrome://browser/content/webext-panels.js"/>
> +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> +  <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>
> +  <script type="application/javascript" src="chrome://browser/content/places/placesModules.js"/>

FWIW, I would be a bit surprised if the webext-panels case needed `placesModules.js`.
Attachment #8955681 - Flags: review?(gijskruitbosch+bugs)
Attachment #8955681 - Flags: review?(mak77)
The dependencies situation is complex, see bug 759260 comment 4

I only see 2 possibilities:
1. hardcode the scripts/getters in every place, keeping only the strictly necessary ones
   The advantage is loading only the strictly needed stuff
   The disadvantage is we may forget things causing (easy to fix) regressions and the code is duplicated everywhere
2. keep things as in the patch, with an abstraction that lazy defines everything where Places may be used
   The advantage is that it likely won't cause regressions and the code is well contained
   The disadvantage is that there's a new script to load and a fee lazy getters we may not use

The cost we care more is about opening browser windows, the visible ones need pretty much need all of placesModules.js
The hidden window is indeed a problem, and abuse of global-scripts.inc in general is a problem (it's too easy and tempting to just add things there).
So maybe we could start by avoiding adding things to global-scripts.inc?
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review231112

::: browser/base/content/webext-panels.xul:27
(Diff revision 1)
>    <script type="application/javascript" src="chrome://browser/content/browser.js"/>
>    <script type="application/javascript" src="chrome://browser/content/browser-places.js"/>
>    <script type="application/javascript" src="chrome://browser/content/webext-panels.js"/>
> +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> +  <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>
> +  <script type="application/javascript" src="chrome://browser/content/places/placesModules.js"/>

This includes browser-context.inc which has a bookmark menu, which calls into PlacesUtils.
(In reply to :Gijs (under the weather; responses will be slow) from comment #4)
> The only thing I'm not sure about is the creation of the placesModules.js
> file here and how we deal with scripts.

I'm also not a big fan of this approach. I started to play with two other approaches 1) inline only the needed scripts in every file and alternatively 2) use an include file to define all the places modules, but I ran into issues with both...

1) I've been finding it hard to tease apart what scripts are actually needed in each XUL file. I started removing some scripts and tests were passing, but later discovered there were code paths not being tested for bookmarking. Maybe if I fix up the eslint environment I could be more confident removing scripts. 

2) A number of tests used the placesOverlay and it's not easy to use the preprocessor in tests. I suppose we could do a hybrid approach where when we can use the prepocessor we use the include file and in test files we use placesModule.js.

I'm kind of leaning towards #2 and also trying to not include everything in global scripts (as Marco mentioned). I'm open to suggestions though.
Flags: needinfo?(bdahl)
PlacesUtils and PlacesUIUtils are likely to be needed in most windows, apart from the hidden window.
browserPlacesViews and treeview.js are needed where there is a Places menu/toolbar, or a tree respectively (bookmark dialogs and the star panel include a tree too).
controller.js is necessary where there's any of the above Places view or a Places context menu.
I don't know about the preprocessor, my understanding was that we were trying to reduce its usage (for eslint, code line references and other build reasons) but I'm not up-to-date with our decision regarding that.

Off-hand I'm not much worried by us forgetting to include one script in one xul file, if we land in 61 we have all the time to land oneline followup fixes, that kind of breakage is not critical in Nightly.
Unfortunately, it looks like the hidden window needs more than we thought. The hidden window on Mac is responsible for the top menu bar (which has bookmarks) when all windows are closed. It appears to need everything from places modules except for PlacesTreeView.
You're right, the native menubar bookmarks menu is one of our views.
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review231328

Clearing for now, we seem to have an agreement about not adding Places stuff to global-scripts.inc

I'm not sure about the preprocessor, for the usual issues (line numbers, slower builds)... How much code would we be adding just hardcoding the needed getters in the xul files? I honestly don't plan to enlarge the number of Places objects in the global scope in the future (maybe the opposite), thus I don't care to have a central script to add further stuff.
Attachment #8955681 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #11)

> Clearing for now, we seem to have an agreement about not adding Places stuff
> to global-scripts.inc

In the current patch I've not included the places stuff in global-scripts.inc, but it doesn't really buy us anything since everything is still needed by browser.xul and macBrowserOverlay.xul (it's effectively global).

Also, for this next version, I've hardcoded all the places uses in non-test files. There are probably places we could reduce which scripts are included, but I'm leaning towards doing that in a follow up bug as my goal here is really to just remove overlay (and not make things worse).
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review231554

Having a follow-up for stripping back some of the script file usage wfm, assuming Marco's happy with that too.

::: browser/components/places/content/placesCommands.inc.xul:1
(Diff revision 2)
> +<commandset id="placesCommands"

For this and the context menu, if you use `hg cp --after` you may be able to preserve some blame on these lines, in the 'ignore whitespace' mode (and even where it doesn't, it will at least help future code archaeology to figure out where this all came from).
Attachment #8955681 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

I tried to do this in mozreview but I think it's convinced Marco reviewed this after the last changes, which is odd...
Attachment #8955681 - Flags: review?(mak77)
Priority: -- → P2
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review232018

It mostly looks ok, or at least there is nothing looking like a show-stopper.

I'm adding Mark to check eslint, mostly I'd like to understand if there may be a smarter way than "import-globals-from module.jsm" everywhere

::: browser/base/content/browser.xul:68
(Diff revision 2)
>  # so that they can be shared by macBrowserOverlay.xul.
>  #include global-scripts.inc
>  
>  <script type="application/javascript">
>    Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", this);
> +  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

move this just before its first use

::: browser/base/content/browser.xul:76
(Diff revision 2)
> +  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");

I'm not totally convinced that ForgetAboutSite should be in the global, since it's used only once...
Imo we have 2 options:
1. for now just import it in "placesCmd_deleteDataHost"
2. we add a lazy getter for it in PlacesUIUtils and replace the only call with PlacesUIUtils.forgetAboutsite.removeData...

For now I'd probably go with 1 considered it's used rarely and when it's used we will already pay a much larger I/O cost.

::: browser/base/content/browser.xul:82
(Diff revision 2)
> +
> +  XPCOMUtils.defineLazyScriptGetter(window, "PlacesTreeView",
> +    "chrome://browser/content/places/treeView.js");
> +  XPCOMUtils.defineLazyScriptGetter(window,
> +    ["PlacesInsertionPoint", "PlacesController", "PlacesControllerDragHelper"],
> +    "chrome://browser/content/places/controller.js");

nit: the indentation is off everywhere for this part, array contents should align.

::: browser/base/content/browser.xul:488
(Diff revision 2)
>                  label="&pageAction.manageExtension.label;"
>                  oncommand="BrowserPageActions.openAboutAddonsForContextAction();"/>
>      </menupopup>
>  
>      <!-- Bookmarks and history tooltip -->
> -    <tooltip id="bhTooltip"/>
> +#include ../../components/places/content/bhTooltip.inc.xul

in multiple places we have this comment
<!-- Bookmarks and history tooltip -->
that in practice is only necessary because the old bhTooltip name was un-understandable.
What about we remove this comment from everywhere and rename the inc file to
bookmarksHistoryTooltip.inc.xul

::: browser/base/content/global-scripts.inc:29
(Diff revision 2)
>    "chrome://browser/content/browser-sidebar.js",
>    "chrome://browser/content/browser-tabsintitlebar.js",
>    "chrome://browser/content/browser-trackingprotection.js",
> +
> +  "chrome://global/content/globalOverlay.js",
> +  "chrome://browser/content/utilityOverlay.js",

I know it doesn't change a thing here, but in general I'm worried about all the stuff added by utilityOverlay.js. Apart from a bunch of ungrouped helpers, it also loads or lazy loads various modules. Some of this stuff may only be relevant for certain windows, and modules import should maybe be elsewhere, not hidden in a subscript. I don't expect us to solve that clearly, just wanted to add a brief note for brainstorming.

For now, the only thing I'd like to see changed here, is converting ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); to a defineModuleGetter in utilityOverlay.js itself. Opening an issue just for this.

::: browser/base/content/macBrowserOverlay.xul:50
(Diff revision 2)
> +  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");

yeah, for now let's just remove ForgetAboutSite from everywhere, import only where/when it's used.

::: browser/components/places/tests/chrome/head.js:6
(Diff revision 2)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Services.scriptloader.loadSubScript("chrome://global/content/globalOverlay.js", this);
> +Services.scriptloader.loadSubScript("chrome://browser/content/utilityOverlay.js", this);

Are these necessary? looks like we are importing them into the global, but later instead we import the modules into the test sub window.
Could you check what's the strict necessary just to run these tests? it shouldn't take much time.
Attachment #8955681 - Flags: review?(mak77) → review+
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review232018

It mostly looks ok, or at least there is nothing looking like a show-stopper.

I'm adding Mark to check eslint, mostly I'd like to understand if there may be a smarter way than "import-globals-from module.jsm" everywhere

::: browser/base/content/browser.xul:68
(Diff revision 2)
>  # so that they can be shared by macBrowserOverlay.xul.
>  #include global-scripts.inc
>  
>  <script type="application/javascript">
>    Services.scriptloader.loadSubScript("chrome://global/content/contentAreaUtils.js", this);
> +  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

move this just before its first use

::: browser/base/content/browser.xul:76
(Diff revision 2)
> +  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");

I'm not totally convinced that ForgetAboutSite should be in the global, since it's used only once...
Imo we have 2 options:
1. for now just import it in "placesCmd_deleteDataHost"
2. we add a lazy getter for it in PlacesUIUtils and replace the only call with PlacesUIUtils.forgetAboutsite.removeData...

For now I'd probably go with 1 considered it's used rarely and when it's used we will already pay a much larger I/O cost.

::: browser/base/content/browser.xul:82
(Diff revision 2)
> +
> +  XPCOMUtils.defineLazyScriptGetter(window, "PlacesTreeView",
> +    "chrome://browser/content/places/treeView.js");
> +  XPCOMUtils.defineLazyScriptGetter(window,
> +    ["PlacesInsertionPoint", "PlacesController", "PlacesControllerDragHelper"],
> +    "chrome://browser/content/places/controller.js");

nit: the indentation is off everywhere for this part, array contents should align.

::: browser/base/content/browser.xul:488
(Diff revision 2)
>                  label="&pageAction.manageExtension.label;"
>                  oncommand="BrowserPageActions.openAboutAddonsForContextAction();"/>
>      </menupopup>
>  
>      <!-- Bookmarks and history tooltip -->
> -    <tooltip id="bhTooltip"/>
> +#include ../../components/places/content/bhTooltip.inc.xul

in multiple places we have this comment
<!-- Bookmarks and history tooltip -->
that in practice is only necessary because the old bhTooltip name was un-understandable.
What about we remove this comment from everywhere and rename the inc file to
bookmarksHistoryTooltip.inc.xul

::: browser/base/content/global-scripts.inc:29
(Diff revision 2)
>    "chrome://browser/content/browser-sidebar.js",
>    "chrome://browser/content/browser-tabsintitlebar.js",
>    "chrome://browser/content/browser-trackingprotection.js",
> +
> +  "chrome://global/content/globalOverlay.js",
> +  "chrome://browser/content/utilityOverlay.js",

I know it doesn't change a thing here, but in general I'm worried about all the stuff added by utilityOverlay.js. Apart from a bunch of ungrouped helpers, it also loads or lazy loads various modules. Some of this stuff may only be relevant for certain windows, and modules import should maybe be elsewhere, not hidden in a subscript. I don't expect us to solve that clearly, just wanted to add a brief note for brainstorming.

For now, the only thing I'd like to see changed here, is converting ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); to a defineModuleGetter in utilityOverlay.js itself. Opening an issue just for this.

::: browser/base/content/macBrowserOverlay.xul:50
(Diff revision 2)
> +  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");

yeah, for now let's just remove ForgetAboutSite from everywhere, import only where/when it's used.

::: browser/components/places/tests/chrome/head.js:6
(Diff revision 2)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Services.scriptloader.loadSubScript("chrome://global/content/globalOverlay.js", this);
> +Services.scriptloader.loadSubScript("chrome://browser/content/utilityOverlay.js", this);

Are these necessary? looks like we are importing them into the global, but later instead we import the modules into the test sub window.
Could you check what's the strict necessary just to run these tests? it shouldn't take much time.
Attachment #8955681 - Flags: review?(standard8)
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review231554

> For this and the context menu, if you use `hg cp --after` you may be able to preserve some blame on these lines, in the 'ignore whitespace' mode (and even where it doesn't, it will at least help future code archaeology to figure out where this all came from).

Will do with my hg repo before my push (using git currently).
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review232018

> I know it doesn't change a thing here, but in general I'm worried about all the stuff added by utilityOverlay.js. Apart from a bunch of ungrouped helpers, it also loads or lazy loads various modules. Some of this stuff may only be relevant for certain windows, and modules import should maybe be elsewhere, not hidden in a subscript. I don't expect us to solve that clearly, just wanted to add a brief note for brainstorming.
> 
> For now, the only thing I'd like to see changed here, is converting ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); to a defineModuleGetter in utilityOverlay.js itself. Opening an issue just for this.

bgrins and I have also been discussing the future of utilityOverlay.js and globalOverlay.js. With the overlay removal the situation should be a little more clear. I'll file a bug to discuss more.

> Are these necessary? looks like we are importing them into the global, but later instead we import the modules into the test sub window.
> Could you check what's the strict necessary just to run these tests? it shouldn't take much time.

Yes, globalOverlay.js and utilityOverlay.js are in global-scripts.inc. global-script.inc doesn't apply to all xul files, it's only imported into browser.xul and macBrowserOverlay.xul.
Attachment #8955681 - Flags: review?(standard8)
Comment on attachment 8955681 [details]
Bug 1442302 - Remove placesOverlay.xul.

https://reviewboard.mozilla.org/r/224770/#review233476

I think we should make a general move towards not having ChromeUtils.import and similar definitions in xul files - it makes life harder for ESLint to know what's in scope, and to follow the scopes through. It also makes it more difficult for developers.

Additionally, it would avoid the need for import-globals-from (which causes more eslint processing).

However, I've agreed with Brendan that we'll tidy this up once we've dropped all the overlays from places.xul as it should be clearer as to what is required/used where.

Therefore r=Standard8 with a follow-up bug filed to clean these up that depends on the relevant overlay removal bugs.
Attachment #8955681 - Flags: review?(standard8) → review+
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e4b09a24ae7
Remove placesOverlay.xul. r=Gijs,mak,standard8
https://hg.mozilla.org/mozilla-central/rev/4e4b09a24ae7
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.