Closed Bug 524201 Opened 15 years ago Closed 14 years ago

Move browserconfig.properties to a jar

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla3.7a5

People

(Reporter: taras.mozilla, Assigned: wesongathedeveloper)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [ts])

Attachments

(1 file, 3 obsolete files)

This file is slowing down our startup and isn't doing anything important.
Whiteboard: [ts]
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #408234 - Flags: review?(mconnor)
You should set the pref in the branding pref file, to maintain the different home pages depending on branding.
Attachment #408234 - Flags: review?(mconnor) → review-
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).
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).
(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.
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.
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.
(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.
Would it be simpler to move this file into a jar?
Yes, that would be pretty easy. Just package it up and use a chrome:// URI.
Summary: get rid of browserconfig.properties → Move browserconfig.properties to a jar
Attached patch Patch (obsolete) — Splinter Review
Attachment #408234 - Attachment is obsolete: true
Attachment #413953 - Flags: review?(gavin.sharp)
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.
OS: Linux → All
Hardware: x86 → All
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.
(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.).
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.
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?
We could also just change callers to use the URL formatter rather than preprocessing the properties file itself...
Attachment #413953 - Flags: review?(gavin.sharp)
Comment on attachment 413953 [details] [diff] [review]
Patch

from the subsequent comments, looks like a new patch is needed.
Wesonga, are you able to put a new patch up for this?
Yeah, I should have a new patch for this in the next week.
Attached patch Patch (obsolete) — Splinter Review
Attachment #413953 - Attachment is obsolete: true
Attachment #429651 - Flags: review?(gavin.sharp)
Blocks: 447581
Gavin, any updates on this?
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.
Attachment #429651 - Flags: review?(gavin.sharp) → review+
Attached patch PatchSplinter Review
> 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...
Attachment #429651 - Attachment is obsolete: true
Attachment #442004 - Flags: review?(l10n)
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).
Attachment #442004 - Flags: review?(kev)
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.
Attachment #442004 - Flags: review?(l10n) → review+
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 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.
Attachment #442004 - Flags: review?(kev) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/da9e50cb4091
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-neededperf
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
Component: Build Config → General
Product: Firefox → Firefox Build System
Keywords: perf
Target Milestone: Firefox 3.7a5 → mozilla3.7a5
You need to log in before you can comment on or make changes to this bug.