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.

code issue? QA to yokoyama@netscape.com for now.

Adding topembed+

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.

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.

oops, lets try that again. here's a patch that builds

above, 1), should have read "for bug 160165"

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.

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

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

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 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

Ok, this fixes a bug in the enumeration code. It should work now.

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.

here's the updated version that reflects tao's comments.

Comment on attachment 102359 [details] [diff] [review] provide override mechanism, addresses tao's comments r=tao. thx!

Comment on attachment 102359 [details] [diff] [review] provide override mechanism, addresses tao's comments sr=sfraser

approval for 1.2 checkin has been requested.

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)

Fix has been checked in. Thanks, Alec

yep! Tomorrows build should have this.

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?

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.

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.

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

I checked that fix in to hopefully fix the os2 bustage, r=sicking