Last Comment Bug 172004 - Provide override for individual strings in string bundles
: Provide override for individual strings in string bundles
Status: RESOLVED FIXED
fix in hand, waiting on approval
: intl, topembed+
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.2beta
Assigned To: Alec Flett
: Roy Yokoyama
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks: 160165
  Show dependency treegraph
 
Reported: 2002-10-01 15:03 PDT by Alec Flett
Modified: 2002-10-14 22:29 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first cut - missing enumeration capability (36.19 KB, patch)
2002-10-07 15:35 PDT, Alec Flett
no flags Details | Diff | Splinter Review
first cut - missing enumeration capability (36.41 KB, patch)
2002-10-07 15:39 PDT, Alec Flett
no flags Details | Diff | Splinter Review
second cut - including enumeration capability (39.09 KB, patch)
2002-10-08 11:20 PDT, Alec Flett
no flags Details | Diff | Splinter Review
updated patch (40.26 KB, patch)
2002-10-08 13:07 PDT, Alec Flett
no flags Details | Diff | Splinter Review
updated patch, fixed enumeration (40.40 KB, patch)
2002-10-09 13:14 PDT, Alec Flett
no flags Details | Diff | Splinter Review
provide override mechanism, addresses tao's comments (41.41 KB, patch)
2002-10-09 13:27 PDT, Alec Flett
tao: review+
sfraser_bugs: superreview+
asa: approval+
Details | Diff | Splinter Review

Description Alec Flett 2002-10-01 15:03:04 PDT
Embeddors need a way to override specific string values in various property
files across the product. the string bundle module should provide a mechanism to
override the existing values and use the overrides instead.

In addition, the mechanism probably needs to make sure to support enumeration of
string properties that don't already exist in the string bundle.

This allows the embeddor to distribute the GRE with minimal customizations to
the actual product, and instead just distribute this override file.
Comment 1 Rui Xu 2002-10-01 16:06:44 PDT
code issue? QA to yokoyama@netscape.com for now.
Comment 2 Chak Nanga 2002-10-01 17:09:03 PDT
Adding topembed+
Comment 3 Alec Flett 2002-10-07 12:45:35 PDT
ok, I've made some really good progress here. Basically what I did was provide a
mechanism for anyone to override a given {stringbundle, key} combination, and
then provided an implementation which overrides it from custom-strings.txt

I'm about 90% done with the implementation, and the only question up in the air
is if each string bundle will maintain a pointer to this string-override object,
or if they will all defer to the string bundle service first. I've also
implemented a mechanism to enumerate override keys for a given bundle, so it
should not be hard to allow the override object to add new keys to an existing
bundle.

I'll have a patch ready hopefully by late afternoon.
Comment 4 Alec Flett 2002-10-07 15:35:15 PDT
Created attachment 102062 [details] [diff] [review]
first cut - missing enumeration capability

ok, here's my first cut at this feature. 

This will look for custom-strings.txt in your chrome directory, and use that to
override any strings.

custom-strings.txt should be in the form:
chromeurl:key=value

such as:

chrome://navigator/locale/navigator.properties:font.language.group=en-GB
chrome://communicator/locale/openLocation.properties:existingNavigatorWindow=Existing
browser window

The two things that this patch that are still an open issue are:
1) right now there is only one override implementation, and we probably want to
be more generic for bug 
2) the enumeration system isn't quite working - I've implemented the
per-chrome-url string enumeration in the override object, but haven't yet
hooked it up to nsStringBundle::GetSimpleEnumeration() - all this basically
means is that you can't yet enumerate the union of all keys in both the
override and the string bundle in question.

I'll try to hook up the enumeration stuff today/tomorrow.
Comment 5 Alec Flett 2002-10-07 15:39:13 PDT
Created attachment 102064 [details] [diff] [review]
first cut - missing enumeration capability

oops, lets try that again. here's a patch that builds
Comment 6 Alec Flett 2002-10-07 16:21:20 PDT
above, 1), should have read "for bug 160165"
Comment 7 Alec Flett 2002-10-08 11:20:48 PDT
Created attachment 102190 [details] [diff] [review]
second cut - including enumeration capability

Ok, here's my first candidate. I'm going to do some testing this afternoon and
hopefully get it finished & checked in today or tomorrow. Wish me luck.
Comment 8 Alec Flett 2002-10-08 13:07:30 PDT
Created attachment 102209 [details] [diff] [review]
updated patch

here's an updated patch, where I've worked out a few issues. The biggest issue
is that .properties files actually use ':' as a delimiter (and since we're
trying to adhere to the Java standard, I won't change that)

so instead, I'm escaping the : in the URL with %3A, and using "#" as a
delimiter to indicate the string key. This means that the entries in
custom-strings.txt need to look like:

chrome%3A//navigator/locale/navigator.properties#nv_done=Done loading the page.


I've tested to make sure this works, and it does! feel free to try the patch.
it will also spit out an WARNING on startup (debug builds) if it finds
custom-strings.txt
Comment 9 Alec Flett 2002-10-08 14:38:52 PDT
There are a few other things going on in this patch. For the sake over reviewers:
- moved nsStringBundleService definition out to nsStringBundleService.h, to make
the code easier to understand
- switched the extensible bundle stuff from nsISupportsArray to
nsCOMArray<nsIStringBundle>
- removed ASYNC_LOADING code which had bit-rotted and made the existing code
hard to read
- fixed some spacing issues where someone was using (gasp!) 1-space indenting
Comment 10 Chak Nanga 2002-10-09 08:42:28 PDT
I applied Alec's patch to my latest trunk build and did a quick test to make
sure that it works in a GRE based env - and it does!

Here's what i did:

1. Created a custom-strings.txt file (with the contents below) in my embedding
client's "chrome" dir(change status msgs. to uppercase):

chrome%3A//necko/locale/necko.properties#ResolvingHost=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#ConnectedTo=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#SendingTo=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#ReceivingFrom=TRANSFERRING DATA FROM
%1$S...
chrome%3A//necko/locale/necko.properties#ConnectingTo=CONNECTING TO %1$S...

2. Ran MfcEmbed and noticed that the status msgs. now appear in upper case in
MfcEmbed's status bar.

Hi Ashish : Can you or someone else in the QA team verify this...just to make
sure we have another set of eyes take a look...thanks
Comment 11 tao 2002-10-09 13:13:12 PDT
Comment on attachment 102209 [details] [diff] [review]
updated patch

Lots of work here...A few comments:
1. since you are taking out Async loading code, why not
taking out CreateAsyncBundle() 
2. How many times would the bundleoverride created if the stringBundleService
is QI-ed multiple times? Will each QI cause a re-check of the ".txt"?
3. What's the performance overhead of this feature?
What's the impact to start up time?

THX
Comment 12 Alec Flett 2002-10-09 13:14:52 PDT
Created attachment 102357 [details] [diff] [review]
updated patch, fixed enumeration

Ok, this fixes a bug in the enumeration code. It should work now.
Comment 13 Alec Flett 2002-10-09 13:26:34 PDT
adding my reviewers so I can answer their questions.

tao:
1. I will do that - and in fact I've now done it in my tree. new patch coming up.
2. QI has nothing to do with the creation of the override object - it is
connected to the string bundle service creation, which only happens once. Thus,
the override object is only created once.
3. Performance impact is negligable in the case where the override does not
exist. during the application's lifetime, the impact is a simple null check
every time a string is created. Startup impact is minimal, and is just a check
for custom-strings.txt, which is a single stat() call to one file.
Comment 14 Alec Flett 2002-10-09 13:27:26 PDT
Created attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

here's the updated version that reflects tao's comments.
Comment 15 tao 2002-10-09 13:35:10 PDT
Comment on attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

r=tao. thx!
Comment 16 Simon Fraser 2002-10-09 14:31:14 PDT
Comment on attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

sr=sfraser
Comment 17 Alec Flett 2002-10-09 15:24:08 PDT
approval for 1.2 checkin has been requested.
Comment 18 Asa Dotzler [:asa] 2002-10-09 16:29:13 PDT
Comment on attachment 102359 [details] [diff] [review]
provide override mechanism, addresses tao's comments

a=asa for checkin to 1.2beta (on behalf of drivers)
Comment 19 Hansoo Kim 2002-10-14 13:36:29 PDT
Fix has been checked in.

Thanks, Alec
Comment 20 Alec Flett 2002-10-14 13:38:23 PDT
yep! Tomorrows build should have this.
Comment 21 Chak Nanga 2002-10-14 13:54:04 PDT
I updated and built my trunk but am unable to verify this fix i.e. the strings
are not being overridden.

1. I created a file named "custom-strings.txt" with the following contents in
the dist\bin\chrome dir:

chrome%3A//necko/locale/necko.properties#3=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#4=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#5=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#6=TRANSFERRING DATA FROM %1$S...
chrome%3A//necko/locale/necko.properties#7=CONNECTING TO %1$S...

2. When i run MfcEmbed or Mozilla from dist\bin i do not see the status msgs. in
all upper case (it showed status msgs. in upper case when i applied the patch a
few days ago and tested)

However, it seems to be finding the custom-strings.txt file as indicated by the
following console msg:

WARNING: Using custom-strings.txt to override string bundles., file d:/mozilla_s
rc/mozilla/intl/strres/src/nsStringBundleTextOverride.cpp, line 107

Any ideas?
Comment 22 Chak Nanga 2002-10-14 14:22:30 PDT
An update...

I built the MfcEmbed base embedding distribution and a GRE distribution and
tested the string override mechanism - it works fine in both the GRE based and
in the regular embedding scenarios.

I'm not sure why Mozilla did not pick up the string overrides. Alec and i
suspect it may be due to the fact that Mozilla FE might be using different
string ids than what's in the sample custom-strings.txt test file.

Anyway, it seems to be working fine now.
Comment 23 Chak Nanga 2002-10-14 14:43:08 PDT
Upon further investigation....there's a rational explanation for all of this
madness.

Please note that when i tested (in Comment #21 above) i used the following
custom-strings.txt:

chrome%3A//necko/locale/necko.properties#3=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#4=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#5=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#6=TRANSFERRING DATA FROM %1$S...
chrome%3A//necko/locale/necko.properties#7=CONNECTING TO %1$S...

Notice the usage of string id's 3,4,5 etc. instead of ResolvingHost,ConnectedTo
etc. That's the reason it did not work from dist\bin.

When i tested in the embedding scenarios, i had created custom-strings.txt with
the following contents - which worked:

chrome%3A//necko/locale/necko.properties#ResolvingHost=RESOLVING HOST %1$S...
chrome%3A//necko/locale/necko.properties#ConnectedTo=CONNECTED TO %1$S...
chrome%3A//necko/locale/necko.properties#SendingTo=SENDING REQUEST TO %1$S...
chrome%3A//necko/locale/necko.properties#ReceivingFrom=TRANSFERRING DATA FROM
%1$S...
chrome%3A//necko/locale/necko.properties#ConnectingTo=CONNECTING TO %1$S...

So, basically we do not have any issues here - everything works just fine!

Except, i'm not sure why we have string id's 3,4,5 etc. and ResolvingHost,
ConnectedTo etc. referring to the same strings in
http://lxr.mozilla.org/seamonkey/source/netwerk/resources/locale/en-US/necko.properties
- guess that's a different issue.
Comment 24 Bradley Baetz (:bbaetz) 2002-10-14 22:08:59 PDT
os2 doesn't seem to like this: ..\..\..\dist\include\xpcom\nsCOMArray.h(212:15)
: error EDC3090: Syntax error - expected "<" and found "&".

Actually, its probably right - you need nsCOMArray<T> for both the param and the
return type, don't you? I don't know why other compilers accept this, but it may
have something to do with the instantiation thing os2 breaks on - see bug 107291
Comment 25 Bradley Baetz (:bbaetz) 2002-10-14 22:29:32 PDT
I checked that fix in to hopefully fix the os2 bustage, r=sicking

Note You need to log in before you can comment on or make changes to this bug.