524201 - Move browserconfig.properties to a jar

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(dormant account)]

This file is slowing down our startup and isn't doing anything important.

Comment 1

[Saint Wesonga]

Attachment: Patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

You should set the pref in the branding pref file, to maintain the different home pages depending on branding.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 408234 [details] [diff] [review]
Patch

Also, Phil correctly points out that patch will break all current uses of the home page, since they use GetComplexValue(nsIPrefLocalizedString).

Comment 4

[Phil Ringnalda (:philor)]

Though I pointed it out slightly wrongly, because nsIPrefLocalizedString always confuses me.

You can use an http URI in something that's fetched that way, after all that's what happens when you change away from the default for your home page, but what you can't do is stick in something with %LOCALE% and things like that, since those expect to be processed by nsIURLFormatter.formatURLPref (and when they aren't, they make GetComplexValue angry).

So you would need to stick with branding/locales/ to not break the current system of having it per-locale and per-branding, but preprocess the pref out of there into yet another prefs file (you know, like the ones Taras keeps trying to remove because there's too many).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> You can use an http URI in something that's fetched that way, after all that's
> what happens when you change away from the default for your home page

Well, not quite - when there's a user pref, GetComplexValue takes a different code path that tries GetCharPref first.

Comment 6

[Phil Ringnalda (:philor)]

See, I _told_ you I was confused!

Okay, so this bug probably has just two choices? A total compatibility break, since browser.startup.homepage has been a localized string pref since forever, and things outside Fx, like nsGlobalWindow, depend on it being one, or find a way to speed up GetComplexValue(nsIPrefLocalizedString) in the non-user-pref case, without breaking any (probably not exhaustively tested) other uses, which might or might not involve recognizing that a string starting with http isn't pointing toward a .properties file.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I don't think we want to move away from it being a localizable pref - too much compat cost, and pain if we need to switch back later. What we could do for the moment is specify the pref in a "data: URI "bundle", e.g.: 

pref("browser.startup.homepge", "data:text/plain,browser.startup.homepage=http://%LOCALE%.www.mozilla.com/%LOCALE%/%APP%/%VERSION%/whatsnew/");
 
I'd like to know more about how much this file is "slowing down our startup" before doing that, though.

Comment 8

[(dormant account)]

(In reply to comment #7)

> I'd like to know more about how much this file is "slowing down our startup"
> before doing that, though.

It costs around 30ms of cold startup on harddrives.

Comment 9

[(dormant account)]

Would it be simpler to move this file into a jar?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Yes, that would be pretty easy. Just package it up and use a chrome:// URI.

Comment 11

[Saint Wesonga]

Attachment: Patch

Comment 12

[:Gavin Sharp [email: [e-mail]]]

You'll have to also add it to removed-files.in. We may also want to move this to branding's content/ given that that's where we're packaging it. And we may even be able to add a resource:/ alias for it to avoid having to update the existing users - I'll test that.

I think this doesn't have any l10n impact (since we're not packaging it from the locale, and weren't before), but CCing Axel as a heads up in case I'm missing something.

Comment 13

[Axel Hecht [:Pike]]

browserconfig.properties contains locale dependent information, and should not be in browser.jar. I'm pretty sure it shouldn't be in @AB_CD@.jar, either, because I don't want to see language packs setting the homepage.

Not sure what BYOB does.

FWIW, I don't agree with the initial comment in this bug, setting the homepage *is* important. It's a revenue-generating asset for us and for everybody that can spoof it, so making that hard to do is good.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> browserconfig.properties contains locale dependent information

What information is that? It contains a single pref that doesn't vary based on locale, and it only lives in m-c (there aren't any copies in the l10n repos)...

We're just changing how the properties file is packaged, we're not actually impacting the ability to change the home page in any way (be it from a locale pack, distribution, etc.).

Comment 15

[Axel Hecht [:Pike]]

http://mxr.mozilla.org/mozilla-central/source/other-licenses/branding/firefox/locales/browserconfig.properties has the locale code of the distributed locale in it. We're also contemplating changing the url completely for some of our smaller locale, in particular for Kurdish google doesn't seem to be a good choice as it prompts users of our latin-based kurdish with arabic script kurdish.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Oh, right. Just means we need to package it in the locale jar, then. It's already possible for langpacks to change the home page by setting the pref, right?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

We could also just change callers to use the URL formatter rather than preprocessing the properties file itself...

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 413953 [details] [diff] [review]
Patch

from the subsequent comments, looks like a new patch is needed.

Comment 19

[Dietrich Ayala (:dietrich)]

Wesonga, are you able to put a new patch up for this?

Comment 20

[Saint Wesonga]

Yeah, I should have a new patch for this in the next week.

Comment 21

[Saint Wesonga]

Attachment: Patch

Comment 22

[(dormant account)]

Gavin, any updates on this?

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 429651 [details] [diff] [review]
Patch

This will break some addons that do silly things (e.g. using getCharPref for the home page pref and comparing against the actual resource: URI), and also people who are used to manually editing browserconfig.properties in customized builds to change the homepage, but I think I am OK with that.

>diff --git a/browser/branding/nightly/locales/Makefile.in b/browser/branding/nightly/locales/Makefile.in

> libs::
>-	@$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) \
>-	  $(srcdir)/browserconfig.properties > $(FINAL_TARGET)/browserconfig.properties
> 
> install::
>-	@$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) \
>-	  $(srcdir)/browserconfig.properties > $(DESTDIR)$(mozappdir)/browserconfig.properties

Can't you just remove these blocks entirely (applies to other files as well)?

The only thing I'm unsure about is the effect this will have on l10n repacks. Axel should sign off on this before it lands.

Comment 24

[Saint Wesonga]

Attachment: Patch

> Can't you just remove these blocks entirely (applies to other files as well)?

Done

> The only thing I'm unsure about is the effect this will have on l10n repacks. Axel should sign off on this before it lands.

Requesting Review...

Comment 25

[Axel Hecht [:Pike]]

Comment on attachment 442004 [details] [diff] [review]
Patch

Requesting feedback from Kev.

I'm concerned about the various ways that folks could override our home page after this patch.

I.e., setting an overlay in an extension's chrome.manifest would overwrite the homepage.

And all language packs out there would probably need to get their values reviewed, according to some policy to be determined? (For language packs right now, we discourage using prefs at all, so it's not that much of a problem).

Comment 26

[Axel Hecht [:Pike]]

Comment on attachment 442004 [details] [diff] [review]
Patch

Denoting that l10n isn't blocking here, r=me.

My comment 25 on the greater ecosystem impact stands, though.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm not sure how this patch makes that work; add-ons can already (and have been known to) change a user's start page.

Comment 28

[Kev Needham [:kev]]

Comment on attachment 442004 [details] [diff] [review]
Patch

Apologies for the delay in replying. I'm not sure how this would make it easier to override user prefs. My understanding is that the trump order is (ideally) defaults->distribution->extensions->user with the user prefs being the ultimate trump. if an extension overwrites a pref (or writes it to prefs.js), any default or distribution default will be over-ridden, and I don't think the change outlined here will affect the ease of which that can be done.

I also admit I may be missing something, but by design extension prefs and user prefs take priority (always) over defaults. It does mean we'll have to make sure start page changes aren't made, as our policy has been (to date) that we don't permit changes to browser.startup.homepage* from the locales (we make the changes in DNS to point at different start pages), so there is a l10n impact there, I believe.

again, unless I'm missing something (totally possible), I don't think this has any adverse impacts.

Comment 29

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/da9e50cb4091