Closed
Bug 1358790
Opened 8 years ago
Closed 8 years ago
compiling city.cpp into browsercomps makes toolkit/xre/ depend on browser/
Categories
(Toolkit :: Application Update, defect)
Toolkit
Application Update
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: myk, Assigned: myk)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mhowell
:
review+
|
Details | Diff | Splinter Review |
Bug 1324617 added /other-licenses/nsis/Contrib/CityHash/cityhash/city.cpp to the sources for browser/components/shell/, which eventually gets linked into xul via browsercomps. Bug 1348609 then made toolkit/xre/nsXREDirProvider.cpp use the same file, which made toolkit depend on browser. That busts other XUL apps, which then have to individually satisfy that dependency. This patch reverses the direction of the dependency by linking city.cpp into xul directly as part of toolkit/xre/. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8c6b2690ca6e00e8666b9b27b1f94f324a34f45 Matt, you did one of the original reviews in that latter bug. Is there a particular reason to compile city.cpp into browsercomps rather than directly into xul?
Attachment #8860668 -
Flags: review?(mhowell)
Comment 1•8 years ago
|
||
Comment on attachment 8860668 [details] [diff] [review] compile city.cpp in toolkit/xre/ so it doesn't depend on browser/ Review of attachment 8860668 [details] [diff] [review]: ----------------------------------------------------------------- No, there's no specific reason for linking it that way. Moving it is fine. Thanks for taking care of this. I would like to see a comment next to the new moz.build line to indicate why it's there, since we no longer build city.cpp anywhere near the source file that uses it.
Attachment #8860668 -
Flags: review?(mhowell) → review+
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Matt Howell [:mhowell] from comment #1) > I would like to see a comment next to the new moz.build line to indicate why > it's there, since we no longer build city.cpp anywhere near the source file > that uses it. We no longer build it near browser/components/shell/nsWindowsShellService.cpp, which uses it. But we now build it near toolkit/xre/nsXREDirProvider.cpp, which also uses it. I could add a comment to toolkit/xre/moz.build like: # city.cpp is used here, by nsXREDirProvider.cpp, and also in files # whose objects eventually link into xul (by way of browsercomps), like # browser/components/shell/nsWindowsShellService.cpp. However, a comment here in toolkit/ that references such a file in browser/ seems like it'll eventually become stale, as browser files are reorganized. Wouldn't it be reasonable to assume that application-specific libraries like browsercomps, which link into xul, may use xul's symbols, without having to specifically reference the symbols being used in xul's build configuration?
Flags: needinfo?(mhowell)
Comment 3•8 years ago
|
||
If you don't feel such a comment would be helpful, then feel free to leave it out. I had forgotten that a usage had been added to nsXREDirProvider.cpp; that's a more recent addition that I didn't write and it just slipped my mind, but you're correct that there are now usages in both places.
Flags: needinfo?(mhowell)
Pushed by myk@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ad48353fa6 compile city.cpp in toolkit/xre/ so it doesn't depend on browser/; r=mhowell
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Matt Howell [:mhowell] from comment #3) > If you don't feel such a comment would be helpful, then feel free to leave > it out. I had forgotten that a usage had been added to nsXREDirProvider.cpp; > that's a more recent addition that I didn't write and it just slipped my > mind, but you're correct that there are now usages in both places. Ok, thanks! I've gone ahead and pushed it without the comment, but I'm happy to add it later if this change seems to be confusing.
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5ad48353fa6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•