Closed
Bug 96525
Opened 23 years ago
Closed 23 years ago
delay loading of strres dll in nsProfile.cpp
Categories
(Core Graveyard :: Profile: BackEnd, defect)
Core Graveyard
Profile: BackEnd
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.9
People
(Reporter: ftang, Assigned: ccarlen)
References
Details
Attachments
(1 file)
(deleted),
patch
|
tao
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
I find this out when I try to address 75041. Somehow the first time we load the
strres.dll is in nsProfile while we try to flush out the stringbundle cache.
This is funny, we load it to ask it to clean up. I think we should not load the
dll while we don't need to.
reproduce procedure in window.
1. download the http://bugzilla.mozilla.org/showattachment.cgi?attach_id=31201
save it as c:/temp/minimal.xul
2. in VC, open file mozilla/intl/strres/src/nsStringBundle.cpp
set a breakpoint at the last line , the line of NS_IMPL_NSGETMODULE
3. in VC, open "Project:Setting, switch from "General" tab to "Debug" tab, while
Category combobox is set to "General" put
"-chrome file:/c:/temp/minimal.xul" in Program Argument
4. change the Category Combobox to "Addiontional DLLs", dobule click under the
"Local Name" it will show a new row with a checkbox check in the left hand side,
in the right hand side of the row, you will see a "..." button, click that
button and locate dist/win*/bin/components/strres.dll
5. run the mozilla
it will break at the previous break point, and you will find out the reason it
load the strres.dll into the memory is because of the following line:
1067 if (isSwitch)
1068 {
1069 // Phase 1: See if anybody objects to the profile being changed.
1070 mProfileChangeVetoed = PR_FALSE;
1071 observerService->Notify(subject,
NS_LITERAL_STRING("profile-approve-change").get(), context.get());
1072 if (mProfileChangeVetoed)
1073 return NS_OK;
1074 1075 // Phase 2: Send the "teardown" notification
1076 observerService->Notify(subject,
NS_LITERAL_STRING("profile-change-teardown").get(), context.get());
1077 1078 // Phase 3: Notify observers of a profile change
1079 observerService->Notify(subject,
NS_LITERAL_STRING("profile-before-change").get(), context.get());
1080 }
1081 1082 // Flush the stringbundle cache
1083 nsCOMPtr<nsIStringBundleService> bundleService =
1084 do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
1085 if (NS_SUCCEEDED(rv)) {
1086 rv = bundleService->FlushBundles();
1087 NS_ASSERTION(NS_SUCCEEDED(rv), "failed to flush bundle cache");
1088 }
I talk to dp, and he make a good point, should line 1081-1088 inside the if
(isSwitch) block?
We should not load the nsIStringBundleService and call FlushBundles unless it
have ever been created.
Assignee | ||
Comment 1•23 years ago
|
||
Yes. There should be no ref to string bundle as there is here in profile mgr.
The reason this is done (it has been for a long time) is that, on choosing a new
profile, possibly the locale has changed. This has been around for longer then
the profile changeobserver mechanism. See:
http://lxr.mozilla.org/mozilla/source/profile/public/nsIProfileChangeStatus.idl
The string bundle service should be a profile change observer (which it can be
by implementing nsIObserver andregistering for a notification listed in
nsIProfileChangeStatus). Probably either "profile-before-change" or
"profile-after-change"
Assignee | ||
Comment 2•23 years ago
|
||
Accepting.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.5
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
This will fix it. Since the string bundle service already implemented
nsIObserver and nsISupportsWeakReference, it was pretty simple :-)
CC'ing Alec for sr. Frank - can you r=?
Comment on attachment 56688 [details] [diff] [review]
remove ref to strres from profile
looks good. r=tao providing
you verified this patch
does not cause the bundle
flushing needed in switching
from global locale to user
profile locale.
TO verify this, you can
1. download a region pack,
regfr.xpi from http://home.netscape.com/computing/download/languagepacks_62.html
and
2. select the France region pack and restart.
3. you default home page
should point to our French home page.
Attachment #56688 -
Flags: review+
Comment 7•23 years ago
|
||
Comment on attachment 56688 [details] [diff] [review]
remove ref to strres from profile
nice! sr=alecf
Attachment #56688 -
Flags: superreview+
Assignee | ||
Comment 8•23 years ago
|
||
tao - I tried your test with NS 6.2 so I could observe the right behavior first
before trying it with my patch. With 6.2, I did this:
1. installed the french langpack and restarted (I couldn't set language/locale
to french until I did)
2. set language and content to french and homepage pref to
http://home.netscape.com/bookmark/6_2/homebutton.html (the default)
3. restarted.
The homepage was the same as U.S. homepage. What could be wrong? Did this work
right in 6.2? Is there another way to test this?
>2. set language and content to french and
Do this.
> homepage pref to http://home.netscape.com/bookmark/6_2/homebutton.html (the
default)
Don't change this. If you manually re-define this, it will be saved into your
profile.
Try this:
1. launch the browser with profile manager
2. create a new profile and set the region to France (French Region Pack).
3. start N6 with this new profile.
4. the start up page and subsequential home page (clicking on "N" logo) should
all point to French sites.
Ping me if this still does not work for you.
Assignee | ||
Comment 13•23 years ago
|
||
I was finally able to run Tao's test. I had to make an alternate content region
by hand in order to do so. I changed "browser.startup.homepage" in my new region
to be cnn.com.
1) I made a new profile in the new region and it came up with the right home page.
2) To make sure it was OK with a string which had actually been used before the
region changed, I stuck in some code in the top and bottom of
nsProfile::SetCurrentProfile to get the value of the "browser.startup.homepage"
pref and dump it to the console. It gave me:
before switch: http://www.mozilla.org/start
### nsCacheProfilePrefObserver::Observe [topic=profile-after-change data=startup]
after switch: http://www.cnn.com
With that, I can say it works. I'll check it in.
Comment 14•23 years ago
|
||
thank you :-)
Assignee | ||
Comment 15•23 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•