Closed
Bug 1445764
Opened 7 years ago
Closed 6 years ago
Remove places related ChromeUtils.import from XUL
Categories
(Firefox :: Bookmarks & History, enhancement, P3)
Firefox
Bookmarks & History
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.
Updated•7 years ago
|
Priority: -- → P3
Comment 1•7 years ago
|
||
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)
Reporter | ||
Comment 2•7 years ago
|
||
Yeah (last of all the non-test overlays actually).
Flags: needinfo?(bdahl)
Comment 3•7 years ago
|
||
I've started looking into this. Will probably not land any patches before 62 development starts though.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
I bumped into this when looking into Bug 1471734. Is this sort of what you were thinking?
Assignee | ||
Comment 6•6 years ago
|
||
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.
Comment 7•6 years ago
|
||
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 8•6 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
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)
Reporter | ||
Comment 13•6 years ago
|
||
(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
Assignee | ||
Comment 14•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
Comment 22•6 years ago
|
||
mozreview-review |
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 23•6 years ago
|
||
mozreview-review |
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 24•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 25•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: standard8 → bgrinstead
Assignee | ||
Comment 30•6 years ago
|
||
Comment 31•6 years ago
|
||
mozreview-review |
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+
Comment 32•6 years ago
|
||
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
Comment 33•6 years ago
|
||
bugherder |
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
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in
before you can comment on or make changes to this bug.
Description
•