Closed Bug 189870 Opened 22 years ago Closed 6 years ago

should update or invalidate fastload file when locale is switched

Categories

(Core :: Internationalization, defect, P3)

x86
Linux
defect

Tracking

()

RESOLVED WONTFIX
mozilla1.4final

People

(Reporter: masaki.katakai, Assigned: jrgmorrison)

Details

(Whiteboard: [a=sspitzer, a=asa])

Attachments

(2 files, 2 obsolete files)

Currently -UILocale option ja-JP does not work. Changing UI language and contentLocale on preference dialog works fine. It seems that XUL cache for fast loading will not be updated when we specify -UILocale option in nsProfile.cpp, which has been already done in pref-contentpacks.xul for preference dialog. It is OK when we create new profile, but it doesn't work when we try to switch language of the existing profile because XUL.mfasl will not be updated. We need to do the same thing in nsProfile.cpp. 119 // If we changed locale, we need to destroy the fastload file so that it 120 // will load the language strings from the new locale jars. No one should 121 // have the fastload file open at this moment, so the remove should succeed. 122 // (XXX actually there is a small window where this is possible, in which 123 // case we're screwed). 124 // XXX This should really be done in the chrome registry itself, not be in 125 // front-end code, but this patch is only to get this mostly working for 1.1b 126 // The code below must die before 1.1final!! 127 if (shouldRemoveFaslFile) { 128 try { 129 const XUL_FASTLOAD_FILE_BASENAME = "XUL"; 130 var faslService = Components.classes['@mozilla.org/fast-load-service;1'] 131 .getService(Components.interfaces.nsIFastLoadService); 132 var faslFile = faslService.newFastLoadFile(XUL_FASTLOAD_FILE_BASENAME); 133 faslFile.remove(false); 134 } catch(e) {} 135 }
Attached patch 1st patch for nsProfile.cpp (obsolete) — Splinter Review
Attached proposed patch: - check the existing locale by using chromeRegistry->IsLocaleSelected() if the same locale is already selected, do nothing - when new locale is selected, remove "XUL.mfasl" to update the file, which is the same logic of pref-contentpacks.xul
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
reopened
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #112132 - Flags: review?(ccarlen)
Status: REOPENED → ASSIGNED
Comment on attachment 112132 [details] [diff] [review] 1st patch for nsProfile.cpp > 126 // The code below must die before 1.1final!! This comment is true. Before adding another instance of this code (that which removes the XUL fastload file) where it doesn't belong, the chrome registry or the fastload service itself should handle this.
Attachment #112132 - Flags: review?(ccarlen) → review-
Agreed. I'll try to put these changes into chrome registry. Thank you for comments.
Changed Summary from regression: -UILocale does not work to nsChromeRegistry should update fastload file when locale is switched
Summary: regression: -UILocale does not work → nsChromeRegistry should update fastload file when locale is switched
Attached patch patch for nsChromeRegistry.cpp (obsolete) — Splinter Review
codes moved to nsChromeRegistry.cpp
Attachment #112132 - Attachment is obsolete: true
should remove codes from pref-contentpacks.xul if this problem is fixed.
dbaron, bzbarsky, Could you review the patch? Actually the patch works fine for me but I'm not sure that way is correct or not.
Attachment #112265 - Flags: review?(bzbarsky)
Comment on attachment 112265 [details] [diff] [review] patch for nsChromeRegistry.cpp I am on vacation and not accepting new review requests. If this still needs review when I get back, please ask.
Attachment #112265 - Flags: review?(bzbarsky)
Comment on attachment 112265 [details] [diff] [review] patch for nsChromeRegistry.cpp I'll try to look at this, although at first glance I think it's the wrong approach, and the correct fix probably lies somewhere in the serialization/deserialization methods (perhaps that for chrome URLs).
Attachment #112265 - Flags: review?(dbaron)
(Although if the set of URLs from which the prototype was created isn't kept attached to the prototype, a solution like this might be reasonable.)
Wheee!! /be
> (From update of attachment 112132 [details] [diff] [review]) > > 126 // The code below must die before 1.1final!! Hmm, that was me for bug 142623. Guess I left the full fix dangling. Sorry. I had come to the conclusion at that time that this should be in the chrome registry (although the fact that there is a fastload file and that it needs to be deleted should probably be hidden behind an "Invalidate" method on the fastload service). Unfortunately I'm not particularly familiar with the chrome baffling, so I can't really comment on where the chrome registry should actually make the decision. [Obviously, at some point where it has changed the locale, but is there only one place where that occurs?].
A couple of things... I noticed that we seem to hold the fastload file open more often than we used to do (or at least more than I remember when I put in the hack in content-packs.xul). So, I think that "fix" is working as reliably as it did back in 1.1. But removing that file was only a convenient way to work around the problem. And after thinking about this, maybe this should all be handled inside of the fastload service, where it serializes the locale id into the fastload file, and asks for the selectedLocale before using the current fastload file. By the way, I also noticed that starting with the command line '-UILocale de-AT', we enter SetProvider twice for "locale", and this patch would try to delete the fastload file twice. Why is that being entered twice?
I understand that The 1st is called from SelectLocale() from nsAppRunner.cpp for setting locale for global. The 2nd is called from SelectLocaleForProfile() of nsProfile::StartupWithArgs() for user profile. The file under user profile will be deleted in only 2nd time.
dbaron is away until April 22. Can anyone work on this?
I agree with jrgm that this should be fixed for 1.4final -- also, I agree it ought to be fixed in the XUL fastload code, not in the chrome registry code. /be
Flags: blocking1.4?
Flags: blocking1.4? → blocking1.4+
Taking. /be
Assignee: katakai → brendan
Status: ASSIGNED → NEW
Priority: -- → P1
Target Milestone: --- → mozilla1.4final
Attached patch proposed fixSplinter Review
This patch exposes (see the XXXbe comment in nsXULPrototypeCache.cpp) the odd assumption that all FastLoaded URIs have the same chrome package. There should probably be a sequel bug on fixing that assumption. Can anyone think of a XUL app that would violate it? /be
Comment on attachment 123939 [details] [diff] [review] proposed fix jrgm points out that we do have multiple packages in chrome URIs that may be fastloaded -- but they all must have the same locale. Is that a fair assumption? It's the one I meant to point out with that XXXbe comment. /be
Attachment #123939 - Flags: superreview?(ben)
Attachment #123939 - Flags: review?(jrgm)
Comment on attachment 123939 [details] [diff] [review] proposed fix this is ben. sr=ben@netscape.com
Attachment #123939 - Flags: superreview?(ben) → superreview+
Attachment #112265 - Attachment is obsolete: true
Comment on attachment 123939 [details] [diff] [review] proposed fix r=jrgm. Thanks for doing this. (and setting approval1.4? flag)
Attachment #123939 - Flags: review?(jrgm)
Attachment #123939 - Flags: review+
Attachment #123939 - Flags: approval1.4?
Whiteboard: [a=sspitzer, let's wait for another driver to second this?]
Comment on attachment 123939 [details] [diff] [review] proposed fix a=asa (on behalf of drivers) for checkin to 1.4.
Attachment #123939 - Flags: approval1.4? → approval1.4+
Fixed. I checked in a version with slightly better comments and a few whitespace and newline tweaks. /be
Status: NEW → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Fixing summary so it doesn't prescribe a proposed implementation detail that turns out to be not what was checked in. /be
Summary: nsChromeRegistry should update fastload file when locale is switched → should update or invalidate fastload file when locale is switched
updating summary, asa seconded it.
Whiteboard: [a=sspitzer, let's wait for another driver to second this?] → [a=sspitzer, a=asa]
Does this fix bug 142623 as well?
Yes, this fixes bug 142623 as well. I should probably remove the workaround, since now, after switching locales, the first attempt to open the fastload file will result in the fastload file being blown away. (But any UI already in the in-memory XUL cache will not be updated to the new locale will not be changed until the next start of the browser; maybe I should blow away the xul cache at that point in the front end. I'll update my build and have a look at what happens).
Ugh. I think we may need to also serialize in the "contentLocale" (e.g., "US") in addition to the "uiLocale" (e.g., "en-US"), since they control different sets of packages that can get serialized into the fastload file. I don't really get all this, but the profile code makes a distinction between these two by calling reg->GetSelectedLocale("global", uiLocale); versus reg->GetSelectedLocale("global-region", contentLocale); http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1662 Although, these same values are also stored in the prefs system as "general.useragent.locale" and "general.useragent.contentlocale". Don't know if those are guaranteed to always be in synch with the chrome reg values.
jrgm: Right, you should take care of both the locale and the contentLocale (though I'd vote for the removal of seperate content packs altogether if someone would plan it - I hope it will be done in the new UI architecture). Chrome registry and pref should be in sync normally - but there's no guarantee. I'd go for taking the chrome registry value in this case...
So, maybe something like this, although I'm not real confident that checking for the "global" and "global-region" packages' locale values is absolutely guaranteed to give the correct answer in all cases. (In particular, it's probably safe to assume that those package names will exist in Buffy, or in Firebird, but I don't think that there is any requirement that those must exist in all possible XUL applications.)
removing blocker+ and reopening until we sort out what needs doing for "content locale". kairo: do you have an updated de-AT xpi (i.e., fairly close to the trunk). I tried the one that is currently available, but I had a few undefined entities that were breaking parts of the UI. (I'm not complaining, since I really appreciate how valuable it is that you do keep those as close to the tip as you do :-).
Status: RESOLVED → REOPENED
Flags: blocking1.4+
Resolution: FIXED → ---
Under what conditions might global and global-region differ? What is the relation, if any, between them? And why must we wire names such as "global" into the XUL-specific FastLoad code, if those names are used only by convention by some (but not all) apps? /be
Um, good questions. And I don't really know the answers... As far as I can tell, the "-region" variants of "global", "navigator", etc., contain information related to the regional content, not the language per se. For example, to use the 'en-US' language pack for the UI (and live with the misspelling of "color"), but supplement it with the "CA" content pack and get Canadian homepages, etc. But the distinction seems blurry, since sometimes the content packs also contain language changes that are relevant to the presumed language of the region (i.e., if I use the "FR" regional content pack with the "en-CA" UI, then the keyword dropdown in NS7.02 has French text, while the rest of the UI is in English). Yes, hard-wiring in the "global" is wrong. So, if someone who has worked on all this uiLocale versus contentLocale stuff can say that we can be certain to get the right answer from the prefs system, then we could use that I suppose.
Hmm, I believe we should _only_ believe chrome registry, and that one holds a selectedLocale entry for every single chrome package (i.e. global, communicator, navigator, global-region, communicator-region, etc.) If we save those selectedLocale settings in the FastLoad file, we can simply blow away FastLoad for only the single package when we see its selectedLocale setting is out of sync. This would fit our chrome registry logic much better than checking only for global, and would also allow us to change the locale for one one package, as well as splitting the need of blowing away and rebuilding, so that it doesn't hit us that big at Mozilla startup, but do some of the work e.g. at mailnews or chatzilla startup. I'm not sure how good this is to realize but it surely fits the logic better, and we'd get rid of having to mention global and global-region, as we would just check for the needed packages as soon as we need them to load.
jrgm has kindly agreed to take this bug. I'm not clear why Mozilla (as opposed to Netscape) cares about content packs. What UI uses the wrong language if I startup from a FastLoad file that was written using the locale I last chose? If I choose a new locale at startup, the checked in code will correctly invalidate, and I'll get the new localizations that I want. Can someone list *exactly* what UI (Mozilla or Netscape commercial) depends on the content pack, rather than the language pack, notion of locale? /be
Assignee: brendan → jrgm
Status: REOPENED → NEW
brendan: Almost all URLs that appear anywhere in the UI are dependent on content packs. Anyway, with the solution I wrote about in Comment #36, I believe we wouldn't have to care about that, not even about hardcoding a name like "global" in that code.
I don't think tracking the (langpack|contentpack) per serialized document (or per package) in the fastload file is in the cards for 1.4; we don't have a mechanism for selectively updating/invalidating portions of the serialization. Longer term, it would be better to perhaps dump this contentpacks scheme, or at least making the distinction between "language" and "content" less blurry. As for the current state of affairs, as far as it seems to me (corrections welcome): 1) if the user uses the pref panel UI to change _language_, upon restart, that choice will be reflected in the UI. 2) if the user uses the pref panel UI to change _language_ and _content_, upon restart, that choice will be reflected in the UI. 3) if the user uses the pref panel UI to change only _content_, upon restart, that choice will (most likely) be reflected in the UI. 4) if the user uses the command line to change _language_, upon restart, that choice will be reflected in the UI. 5) if the user uses the command line UI to change _language_ and _content_, upon restart, that choice will be reflected in the UI. 6) if the user uses the command line UI to change only _content_, upon restart, that choice will _not_ be reflected in the UI. So, we fail in the final case. But, is it really the case that end users wish to regularly toggle only the region and not the language. Is that really what people do?
jrgm: If it works as you tell, then it's good enough for 1.4, I believe. At least it looks better than the currrent case with the workaround. In the longer term, it would be good to have a chance of blowing away only the serialization package-wise (I didn't know that's not possible yet) and do it the way I described, IMHO. I'd vote for dumping the content-pack thingy as well, but I belive that's orthogonal to the case here. (And e.g. the new application concept might make per-package localizations more likely, I believe...)
QA Contact: amyy → i18n
Decreasing the priority as no update for the last 2 years on this bug. See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage about the priority meaning.
Priority: P1 → P5
Obsoleted by bug 654489.
Status: NEW → RESOLVED
Closed: 22 years ago6 years ago
Priority: P5 → P3
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: