Closed Bug 1445764 Opened 6 years ago Closed 6 years ago

Remove places related ChromeUtils.import from XUL

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: bdahl, Assigned: bgrins)

References

Details

Attachments

(3 files)

Follow up for bug 1442302. 

(In reply to Mark Banner (: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.
Priority: -- → P3
Brendan, is bug 1443971 the last of the places related overlays now? If so, I'll take a stab at cleaning this up.
Flags: needinfo?(bdahl)
Yeah (last of all the non-test overlays actually).
Flags: needinfo?(bdahl)
I've started looking into this. Will probably not land any patches before 62 development starts though.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
I bumped into this when looking into Bug 1471734. Is this sort of what you were thinking?
See Also: → 1471734
Just realized this misses some more Places XUL files: https://searchfox.org/mozilla-central/search?q=placesutils&path=xul. I don't see a shared script to put the imports in at first glance, let me know if you have any ideas.
That's roughly what I had been thinking about - put the inclusions into browser.js or global-scripts.inc (worst case), and that would make things a bit cleaner.

Unfortunately as you've found there's the other dialogs, for which I was thinking about putting the required scripts into editBookmark.js or historySidebar.js (i.e. the main script associated with the xul).

I had been looking at seeing if we could work out the exact dependencies of each one, e.g. historySidebar.js requires XPCOMUtils, Services etc, and only include those, however, Places' tree.xml then comes along and throws a curve ball by ending up requiring the Places' world (aside: when we can move to modules and only have modules included per script, it'd making sorting out globals & inclusions sooo much easier).

So it may just be easiest to move the inclusions into the associated js files for now until we're a bit further down the module line.
Comment on attachment 8988346 [details]
Bug 1445764 - Move Places imports from XUL to JS for the Places documents;

https://reviewboard.mozilla.org/r/253584/#review260380
Attachment #8988346 - Flags: review?(standard8)
Pushed up some patches that do:

1) Add a new js file that includes the imports and make sure that's loaded in any page in places/ that currently use the boilerplate includes (then add eslint import globals comments as needed).
2) Handle browser window (and non-browser windows on mac) through separate imports in browser.js
3) Clean up lint configurations

A couple notes:
1) It's possible we'd want to fold together 1 and 2 by including the placesImports.js file from global-scripts.inc. In that case I we'd still need a bit of special casing for eslint to know that placesImports.js is loaded everywhere. I guess it depends on how often we imagine the list of dependencies changing (I expect not too much but I'm not sure).
2) I kept placesImports.js pretty thin, but I also did consider loading globalOverlay.js and utilityOverly.js directly from that file with subscript loader so we don't need to assume that the pages also include those as script tags. In that case we'd likely get a duplicate load of those two files in places.xul on mac (once from the script tag and once from macWindow.inc.xul)
(In reply to Brian Grinstead [:bgrins] from comment #12)
> Pushed up some patches that do:
> 
> 1) Add a new js file that includes the imports and make sure that's loaded
> in any page in places/ that currently use the boilerplate includes (then add

I had done this previously, but it was decided just inlining it everywhere then reducing the uses was better https://bugzilla.mozilla.org/show_bug.cgi?id=1442302#c11
(In reply to Brendan Dahl [:bdahl] from comment #13)
> (In reply to Brian Grinstead [:bgrins] from comment #12)
> > Pushed up some patches that do:
> > 
> > 1) Add a new js file that includes the imports and make sure that's loaded
> > in any page in places/ that currently use the boilerplate includes (then add
> 
> I had done this previously, but it was decided just inlining it everywhere
> then reducing the uses was better
> https://bugzilla.mozilla.org/show_bug.cgi?id=1442302#c11

OK, I could copy the boilerplate out into individual places JS files instead of introducing a new script.
Comment on attachment 8988346 [details]
Bug 1445764 - Move Places imports from XUL to JS for the Places documents;

https://reviewboard.mozilla.org/r/253584/#review261388

::: browser/components/places/content/places.xul
(Diff revision 4)
>    <script type="application/javascript"
>            src="chrome://global/content/globalOverlay.js"/>
>    <script type="application/javascript"
>            src="chrome://browser/content/utilityOverlay.js"/>
> -  <!-- On Mac, these are included via macWindow.inc.xul -->
> -  <script type="application/javascript"><![CDATA[

Unfortunately removing all of these from selectBookmark causes it to break - because it uses the places tree widget, it needs most of these to work.

I had a quick test, and it looks like we need:

XPCOMUtils.js
PlacesUtils.js
PlacesUIUtils.js
treeView.js
controller.js
Attachment #8988346 - Flags: review?(standard8)
Comment on attachment 8989245 [details]
Bug 1445764 - Remove redundant Places imports from pages that load browser.js;

https://reviewboard.mozilla.org/r/254298/#review261402
Attachment #8989245 - Flags: review?(standard8) → review+
Comment on attachment 8989246 [details]
Bug 1445764 - Remove some Places special casing in eslint configurations;

https://reviewboard.mozilla.org/r/254300/#review261406

Yes much nicer. Once you've updated the first patch, you might want to do a full mochitest run on try just to make sure we've not missed anything. It wouldn't catch the selectBookmark issue as we don't have a test for that (though I'm just about to go write one if I can).
Attachment #8989246 - Flags: review?(standard8) → review+
I've got a good try push with the current set of patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e0da39ea5921633a3d1e17f9919fbdac3da3235. But I'll do another one after updating the first patch.
Since we needed most of them anyway, I copied the whole "Shared Places Import" boilerplate into selectBookmarks.js so it can be easier kept in sync with any changes to the Places documents.
Assignee: standard8 → bgrinstead
Comment on attachment 8988346 [details]
Bug 1445764 - Move Places imports from XUL to JS for the Places documents;

https://reviewboard.mozilla.org/r/253584/#review261850

Great, thank you.
Attachment #8988346 - Flags: review?(standard8) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8c1351d138a
Move Places imports from XUL to JS for the Places documents;r=standard8
https://hg.mozilla.org/integration/autoland/rev/f4774e2d5eba
Remove redundant Places imports from pages that load browser.js;r=standard8
https://hg.mozilla.org/integration/autoland/rev/9d075027105b
Remove some Places special casing in eslint configurations;r=standard8
https://hg.mozilla.org/mozilla-central/rev/c8c1351d138a
https://hg.mozilla.org/mozilla-central/rev/f4774e2d5eba
https://hg.mozilla.org/mozilla-central/rev/9d075027105b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: