1358790 - compiling city.cpp into browsercomps makes toolkit/xre/ depend on browser/

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: compile city.cpp in toolkit/xre/ so it doesn't depend on browser/

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?

Comment 1

[Molly Howell (she/her) (no longer active)]

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.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(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?

Comment 3

[Molly Howell (she/her) (no longer active)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ad48353fa6
compile city.cpp in toolkit/xre/ so it doesn't depend on browser/; r=mhowell

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(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 6

[Molly Howell (she/her) (no longer active)]

Sounds good; thanks!

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5ad48353fa6