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)
Tracking
()
RESOLVED
WONTFIX
mozilla1.4final
People
(Reporter: masaki.katakai, Assigned: jrgmorrison)
Details
(Whiteboard: [a=sspitzer, a=asa])
Attachments
(2 files, 2 obsolete files)
5.83 KB,
patch
|
jrgmorrison
:
review+
brendan
:
superreview+
asa
:
approval1.4+
|
Details | Diff | Splinter Review |
3.42 KB,
patch
|
Details | Diff | Splinter Review |
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 }
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Comment 2•22 years ago
|
||
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
Reporter | ||
Updated•22 years ago
|
Attachment #112132 -
Flags: review?(ccarlen)
Reporter | ||
Updated•22 years ago
|
Status: REOPENED → ASSIGNED
Comment 4•22 years ago
|
||
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-
Reporter | ||
Comment 5•22 years ago
|
||
Agreed. I'll try to put these changes into chrome registry.
Thank you for comments.
Reporter | ||
Comment 6•22 years ago
|
||
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
Reporter | ||
Comment 7•22 years ago
|
||
codes moved to nsChromeRegistry.cpp
Attachment #112132 -
Attachment is obsolete: true
Reporter | ||
Comment 8•22 years ago
|
||
should remove codes from pref-contentpacks.xul
if this problem is fixed.
Reporter | ||
Comment 9•22 years ago
|
||
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.
Reporter | ||
Updated•22 years ago
|
Attachment #112265 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•22 years ago
|
||
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.)
Comment 13•22 years ago
|
||
Wheee!!
/be
Assignee | ||
Comment 14•22 years ago
|
||
> (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?].
Assignee | ||
Comment 15•22 years ago
|
||
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?
Reporter | ||
Comment 16•22 years ago
|
||
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.
Reporter | ||
Comment 17•22 years ago
|
||
dbaron is away until April 22.
Can anyone work on this?
Comment 18•22 years ago
|
||
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?
Updated•22 years ago
|
Flags: blocking1.4? → blocking1.4+
Comment 19•22 years ago
|
||
Taking.
/be
Assignee: katakai → brendan
Status: ASSIGNED → NEW
Priority: -- → P1
Target Milestone: --- → mozilla1.4final
Attachment #112265 -
Flags: review?(dbaron)
Comment 20•22 years ago
|
||
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 21•22 years ago
|
||
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 22•22 years ago
|
||
Comment on attachment 123939 [details] [diff] [review]
proposed fix
this is ben. sr=ben@netscape.com
Attachment #123939 -
Flags: superreview?(ben) → superreview+
Updated•22 years ago
|
Attachment #112265 -
Attachment is obsolete: true
Assignee | ||
Comment 23•22 years ago
|
||
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?
Updated•22 years ago
|
Whiteboard: [a=sspitzer, let's wait for another driver to second this?]
Comment 24•22 years ago
|
||
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+
Comment 25•22 years ago
|
||
Fixed. I checked in a version with slightly better comments and a few
whitespace and newline tweaks.
/be
Status: NEW → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 26•22 years ago
|
||
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
Comment 27•22 years ago
|
||
updating summary, asa seconded it.
Whiteboard: [a=sspitzer, let's wait for another driver to second this?] → [a=sspitzer, a=asa]
![]() |
||
Comment 28•22 years ago
|
||
Does this fix bug 142623 as well?
Assignee | ||
Comment 29•22 years ago
|
||
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).
Assignee | ||
Comment 30•22 years ago
|
||
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.
![]() |
||
Comment 31•22 years ago
|
||
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...
Assignee | ||
Comment 32•22 years ago
|
||
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.)
Assignee | ||
Comment 33•22 years ago
|
||
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 → ---
Comment 34•22 years ago
|
||
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
Assignee | ||
Comment 35•22 years ago
|
||
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.
![]() |
||
Comment 36•22 years ago
|
||
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.
Comment 37•22 years ago
|
||
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
![]() |
||
Comment 38•22 years ago
|
||
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.
Assignee | ||
Comment 39•22 years ago
|
||
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?
![]() |
||
Comment 40•22 years ago
|
||
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...)
Updated•15 years ago
|
QA Contact: amyy → i18n
Comment 41•6 years ago
|
||
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 ago → 6 years ago
Priority: P5 → P3
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•