Closed Bug 56680 Opened 24 years ago Closed 11 years ago

use a xul <stringbundle/> instead of including the strres.js code

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jag+mozbugs, Assigned: sgautherie)

References

(Blocks 1 open bug, )

Details

(Keywords: meta)

Attachments

(6 files, 18 obsolete files)

6.69 KB, patch
Details | Diff | Splinter Review
127.12 KB, patch
Details | Diff | Splinter Review
1.39 KB, patch
Details | Diff | Splinter Review
21.53 KB, patch
Details | Diff | Splinter Review
35.47 KB, patch
Details | Diff | Splinter Review
10.10 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
<BenGoodger>
Ok, basically every time you include a js file you get that js file again since
they aren't cached so all over the product we have 65,423 people including
strres.js over and over. The binding is cached ;) Plus, strres also has a bunch
of other shit that no one ever uses in it.
</BenGoodger>

Currently there are 83 .xul files "including" strres.js which could benefit from
Ben's <stringbundle/>. I suggest we move to <stringbundle/>.

The migration is simple:
In the xul file:
- add a <stringbundle id="foo" src="chrome://path/to/file.properties"/>
- remove the strres.js reference
In the js file:
- replace the stringbundle fetching code with a document.getElementById('foo');
- replace .GetStringFromName(property) with .getString(property)

I'll attach a sample patch for the xp filepicker.
In that case I'll spend some time tracking module owners and creating a bunch of
patches :-)
cc'ing myself. this is a good idea, thanks for taking it on.
If we did cache and share JS scripts loaded via <script src="..."/> in XUL, then
I wonder how big a win this <stringbundle> tag would be over the XPConnect way. 
I like it for other reasons (language-neutrality, XUL+DOM sufficiency), don't
get me wrong.  Jag, any idea of the win from this one change, in terms of code
and data footprint?

/be
I've no clue on the win. How'd I find out?

Btw, I believe we're still doing things the XPConnect way (behind the scenes),
take a look at stringbundleBindings.xml.
Use a reliable process monitor, one that tells the truth about data segment size
moment by moment.  Do A-B tests starting mozilla on the test page, measure size.

/be
Suggestions for a reliable process monitor on linux?
Btw, we could lazily initialize _bundle.
r=timeless for 24026
you sexy bitch. 

my personal fetish is for globals to be prefixed with 'g' so that its obvious
that the var is global when it's used throughout the file. If you agree with me,
you can change it, otherwise, feel free to ignore me. 

a=ben@netscape.com
r=blake
+    if (window.arguments && window.arguments[0]) {
"arguments" in window, no?
Ok, ignoring that last patch for now, as I'm intending to do mailnews/* at once,
due to interdependencies between overlays, etc.  Also, jag and I were discussing
the conventions to be used, and going forward it will be:

.xul -> id="bundle_<identifier>"
.js -> g<Identifier>Bundle

where <identifier> is the first part of the .properties file referenced e.g. for
search.properties we would have id="bundle_search" in xul and gSearchBundle in js
Attached patch patch - mailnews/* (most anyway) (obsolete) — Splinter Review
Ok, the patch just attached requires jag's patch to 68449 to even stand a
chance.  Also, the attached patch includes, as an unintended bonus, the patches
to 61801 and 61802 which will likely be checked in well ahead of this and thus
leave in a later revisions of this patch.

That said, there are also instances where I couldn't yet avoid strres.js,
especially where it is used by xbl controls...  Also, there are a few places
that look like code was moved around arbitrarily, but it must be noted that the
getElementById() call will fail when the .js file is first loaded, so some
things got moved to accommodate that.

I tested as much of mailnews as I could, but I didn't cover IMAP and might have
missed some other things, so it might be nice to get a mailnews qa, or someone
equally familiar with this area, to apply this patch and run through their
testsuite before this gets near the tree.  enjoy
Index: mozilla/mailnews/addrbook/resources/content/abCardViewOverlay.js
wouldn't a global object containing the z* properties be more appropriate? what 
are the issues [performance ?] between globals and objects?

<q class="ignore" comment="if someone else convinces you to do a rewrite, ...">
at some point we need to fix <script language="javascript" src=...> and <script 
src=...> to <script type="text/javascript" src=...>
It looks like you're continuing tabs, esp AccountManager.js,v @@ -201,7 +201,7 
@@
if (!a || a == "") => if (!a) ibid.
return(a); => return a;

... if (!(accountCount > 0)) {
I know, not your fault.  maybe i need a bug against me to do all this.
</q>
+  <!-- XXX: only mailWidgets.xml requires strres.js (<script> isn't valid in 
XUL yet - see hyatt)-->
?@#?

 function onNewFilter()
2spaces or 4?

there are 2 //dump()s that your patch mentions that you didn't remove. Do we 
think they're more important?

the bundle conventions sound good.
Assignee: disttsc → maolson
Regarding your re-ordering trick, I think in the end you'll be safer off by
always initializing var gFooBundle from the onload function instead of depending
on load order (which is fragile, meaning you need comments indicating to keep
that stuff in that specific order, blah).

If for whatever reason you can't edit the onload function (in the case of an
overlay where the overlay doesn't provide the onload function for example) you
can do something like this in the overlay's js:

var gFooBundle
window.addListener("load", overlayInit, false);
function overlayInit()
{
  gFooBundle = document.getElementById(...);
}

In the cases you're touching it looks like you won't need that though.

I see you've put some <stringbundle/>s in windows which are used by the overlay.
What you could do instead is use <stringbundleset id="stringbundleset"/>.

Also, the other way around won't work. If you're placing a <stringbundle/> in an
overlay, you'll need to have a <stringbundle/> with the same id in the document
you're overlaying on. In that case, again it's easier to use <stringbundleset
id="stringbundleset"/> and wrap the <stringbundle/> in the overlay in it.

For example, put <stringbundleset id="stringbundleset"/> in :
/mailnews/addrbook/resources/content/abEditCardDialog.xul
/mailnews/addrbook/resources/content/abNewCardDialog.xul

Then put this:
<stringbundleset id="stringbundleset">
  <stringbundle id="bundle_addressBook
src="chrome://messenger/locale/addressbook/addressBook.properties"/>
</stringbundleset>

in /mailnews/addrbook/resources/content/abCardOverlay.xul

That way you keep the stringbundle and the code closer together.

Same for abCardViewOverlay.* and addressbook.xul.

These two files already have abMailListDialog.js
/mailnews/addrbook/resources/content/abMailListDialog.xul
/mailnews/addrbook/resources/content/abEditListDialog.xul

because they're having this file overlayed:

/mailnews/addrbook/resources/content/abListOverlay.xul

I think you can safely remove the script tags from the first two, or
alternatively (and better, I think) you can split the JS up into two files.

Anyway, here too you can use a stringbundle set, and it looks like you didn't
add the code to init gAddressBookBundle. 

Also, this change is not needed if you're always calling awClickEmptySpace with
a DOM element as |targ|:

-       if (targ.localName != 'treechildren')
+       if ("localName" in targ && targ.localName != 'treechildren')

/mailnews/addrbook/resources/content/abSelectAddressesDialog.xul and .js

It looks like the <stringbundle/> needed by MsgComposeCommands.js is only ever
used in messengercompose.xul. For now I think it's safe to not put the
composeMsgs <stringbundle/> in abSelectAddressesDialog.xul, though ultimately
the code shared should be factored out (less bloat).

---- more to come, but I feel a crash coming up ;-)
About WizardManager, you could move these four scripts into wizardOverlay.xul:
<script src="chrome://global/content/wizardOverlay.js"/>
<script src="chrome://global/content/wizardHandlerSet.js"/>
<script src="chrome://global/content/wizardManager.js"/>
<script src="chrome://global/content/widgetStateManager.js"/>

And then you can do the <stringbundleset/> thing again :-)

Same deal about moving <stringbundle/> from newFolderDialog.xul and
renameFolderDialog.xul to msgFolderPickerOverlay.xul

Etc. etc... If you want specifics I'll look further and write them in an e-mail
:-) You've done great work here though, don't let all these comments make you
think otherwise.

Since you're touching these lines, you could use |foo.disabled = !enable;|

+    if (enable) {
+        checkbox.removeAttribute("disabled");
+        numberFld.removeAttribute("disabled");
+    }
+    else {
+        checkbox.setAttribute("disabled", "true");
+        numberFld.setAttribute("disabled", "true");
+    }
Nice work. A few comments:

1. StringBundles are cached insde strres module. Once a bundle is created,
   it is cached inside nsIStringBundleService. Furhter request of stringBundle
   creation of the same URI gets the cached bundle. 
2. StringBundle creation and loading is a synchronous operation which blocks
   the UI thread. Attempting to create several bundles at once during startup
   might be a performance drag.
3. Q: What's the life span of these XUL binding cached bundles and how are they
   cached? Be careful not to reference a bogus bundle. This probably won't
   happen if the xul binding cache is a in-memory cache.
> 1. StringBundles are cached insde strres module. Once a bundle is created,
>    it is cached inside nsIStringBundleService. Furhter request of stringBundle
>    creation of the same URI gets the cached bundle.

I guess what's cached here is the xbl prototype (xbl binding DOM + compiled
script), which is a win over having to compile strres.js every time it's used.
But you should really ask Ben or hyatt...

> 2. StringBundle creation and loading is a synchronous operation which blocks
>    the UI thread. Attempting to create several bundles at once during startup
>    might be a performance drag.

Yeah, that's why I recently changed <stringbundle/> to lazily create the bundle,
i.e. the first time someone does |getString| or |getFormattedString|, instead of
at xbl binding time.

> 3. Q: What's the life span of these XUL binding cached bundles and how are
>    they cached? Be careful not to reference a bogus bundle. This probably
>    won't happen if the xul binding cache is a in-memory cache.

I'd expect the per instance data to live as long as the instance, the DOM
element. In these cases, that's as long as the window is open.
For some of the getFormattedString calls you could make the second param (on the
next line) line up with the first.

abCardViewOverlay.js:
You have a var gAddressBookBundle in addressbook.js which will clash with the
one in here. One way around this is to use function local vars. Another is to
just expect gAddressBookBundle to already be set. A third is to do something
like:

At the global level:
if (!("gAddressBookBundle" in window))
  var gAddressBookBundle;

And then in onload:
if (!gAddressBookBundle)
  gAddressBookBundle = document.getElementById("...");

In this case I'd just expect gAddressBookBundle to be set, perhaps add a note
that it's set in addressbook.js' onload function. Normally I'd pick the function
local var option, though

abMailListDialog.js:

Perhaps we should replace the <script> tags in abEditListDialog.xul and
abMailListDialog.xul with comments referring to abListOverlay.xul for
abMailListDialog.js.

 function awClickEmptySpace(targ, setFocus)
 {
-       if (targ.localName != 'treechildren')
+       if ("localName" in targ && targ.localName != 'treechildren')
                return;

I don't think that change is necessary.

msgAccountCentral.js:

It looks like the string bundle vars don't need to be global there.

In subscribe.js:
+var gSubscribeBundle = document.getElementById("bundle_subscribe");

In SearchDialog.js:
-       if (gCurrentFolder && (searchSubfolders || gCurrentFolder.isServer))
-       {
+  if (gCurrentFolder && (searchSubfolders || gCurrentFolder.isServer))
+  {
                AddSubFolders(gCurrentFolder);
        }

Fix those other two lines too then :-)

In SearchDialog.xul:
-  <script src="chrome://global/content/globalOverlay.js"/>
   <script src="chrome://messenger/content/SearchDialog.js"/>
+  <script src="chrome://global/content/globalOverlay.js"/>

Tsk! :-)

downloadheaders.js:

+    if (window.arguments && window.arguments[0]) {
...

-       numberElement = document.getElementById("number");
-       numberElement.value = nntpServer.maxArticles;
+  numberElement = document.getElementById("number");
+  numberElement.value = nntpServer.maxArticles;

Indentation looks wrong there (switching from 4 to 2).

In importDialog.js:
-  meterText = GetFormattedBundleString('MailProgressMeterText', name);
+  meterText = gImportMsgsBundle.getFormattedString('MailProgressMeterText',
name);

You forgot to add the [ ] which were added inside GetFormattedBundleString.
What do you think, this change, or just changing the implementation of
Get(Formatted)BundleString?

importDialog.xul: is this a dead file?

In MsgComposeCommands.js:
+var gComposeMsgsBundle = document.getElementById("bundle_composeMsgs");

Forgot one ;-)

In msgPrintEngine.xul:
are you sure you can remove that? utilityOverlay.js seems to need it, and since
utilityOverlay.* hasn't been converted yet, I'm not sure that's safe. Then
again, it looks like utilityOverlay.xul isn't used there at all. *sigh*

In messageWindow.xul:
You'll need to remove the <stringbundleset id="stringbundleset"/> a bit lower in
that file :-)

In mailWindowOverlay.xul:
I'm not sure if you can remove the stringbundleset there, I'm using it has hook
for the overlays on that overlay. If mWO.xul is first overlayed, and then its
overlays are overlayed, your change is fine. If the overlays are first overlayed
on mWO.xul, before it is overlayed itself, then there's a problem. I didn't look
this up and just kept on the safe side, but this is an interesting question IMO.

In am-copies.xul:
+  <script type="text/avascript" src="chrome://messenger/content/am-copies.js"/>
                 ^^^^^^^^^^^^^^
Oops...

The rest of the code looks pretty good.

What do you think about adding comments like:

// gMessengerBundle needs to be defined and set to use this overlay

and

// gMessengerBundle can be found in foo.js / foo.xul

?
Sweet!  I can always count on thorough comments from jag, that's for sure...

If I don't mention it, it's done.  Othewise:

-       if (targ.localName != 'treechildren')
+       if ("localName" in targ && targ.localName != 'treechildren')
Remnant of a similar strict warning fix - may or may not be necessary, but
should be harmless either way.

I screwed up the calls to getFormattedString, that doesn't mean we should break
the function to fix the programmer.  I think last time we talked about this, it
was decided to leave getFormattedString alone.  Now I think it might be a good
idea to assert or at least dump() loudly if we get a string instead of an array.

importDialog.xul - I'd like to stay (somewhat) focused on the bundle changes,
since an sr is going to be hard enough here...

Should we see what brendan can do about an avascript engine for that newly
created type there?  :(

Do I sense some unease with the status quo here?  Perhaps some refactoring here
jag?  There seem to be a couple areas that could benefit from a careful look at
the js.

I think that's all the comments here.  I think we have a few side bugs that
might spring out from what we've seen here.  Using globals in overlayed js is
just asking for conflicts, so maybe we need to grab bundles locally to each
function that uses them, although the globals may provide more benefit than
trouble...

New patch momentarily, after some testing.
Status: NEW → ASSIGNED
Attached patch patch - latest mailnews stuff (obsolete) — Splinter Review
About getFormattedString:

> What do you think, this change, or just changing the implementation of
> Get(Formatted)BundleString?

With that I meant changing the implementation of those functions in
importDialog.js:

function GetFormattedBundleString(strId, formatStr)
{
  return gImportMsgsBundle.getFormattedString(strId, [formatStr]);
}

Or something like that...

I think calling nsIStringBundle::formatStringFromName with an incorrect argument
type (string instead of array) will cause xpconnect to throw some exception of
that kind, so I don't think adding special code in |getFormattedString| is
necessary.

r=jag.
Ok, since the mailnews stuff has jag's highly sought after r=, cc'ing all three
of the mailnews sr folks since any or all might be interested. (Or all three
might think the others are covering this - let's hope that doesn't happen).
sr=bienvenu for the mailnews stuff.
blake checked in the mailnews stuff today.  More to come in other areas.
Latest attachment removes strres.js include from mini-nav.xul (it wasn't
actually used there) and the embedding jar.mn.  stringBundleBindings.xml is
already in jar.mn, so embedding can use that if stringbundles are necessary.

Removing mailnews folks I cc'd earlier to save them the spam.
r=blake
Embedding portion checked in (sr=blizzard on irc)

Profile manager diff coming up.
Attached patch patch - profile manager changes (obsolete) — Splinter Review
 function onLoad()
strict error. function doesn't always return a value.
suggest: return;

<script language="javascript" ...></script>
should be
<script type="application/x-javascript" .../> although until we do the mass 
change, i'll accept text/javascript.

+    lString = lString.replace(/%brandShortName%/gi,
+                              gBrandBundle.getString("brandShortName"));
breaks the %s convention and using the bundle formatting functions rules.
timeless: shouldn't you suggest 'return true;' at the end of onLoad, instead of
just 'return;'?

/be
brendan's right. onload handlers should return a boolean.
html:iframe => iframe
the .replace() stuff is more important. jag want to offer some help?
timeless, what patch are you reviewing?  Surely it isn't one attached to a bug
entitled "use a xul <stringbundle/> instead of including the strres.js code",
because none of your comments have addressed anything material to the issue at
hand.  The only reason I touched the <html:iframe> line was to remove a tab. 
Whatever issues you have with replace(), please take up in another bug as this
one is quite large enough of its own accord.
r=timeless w/ leway to fix stuff i've mentioned. fwiw the replace stuff is now 
covered by another bug
profile manager stuff checked in - a=ben via irc.
Attached patch patch - prefs (obsolete) — Splinter Review
mozilla/xpfe/components/prefwindow/resources/content/pref-proxies.js

+  prefutilitiesBundle = document.getElementById("bundle_prefutilities");

no var?



mozilla/xpfe/components/prefwindow/resources/content/pref-proxies.xul

@@ -102,7 +110,7 @@
               <textfield id="networkProxySOCKS_Port" pref="true" preftype="int"
prefstring="network.proxy.socks_port"
                        prefattribute="value" size="5"/>
             </box>
-         </row>
+    </row>
           <row> 

Is that re-indent correct?



mozilla/xpfe/components/prefwindow/resources/content/pref-smart_browsing.xul:
+    var regionBundle = document.getElementById("bundle_region");
+    if (regionBundle)
+      var smartBrowsingURL = regionBundle.getString("smartBrowsingURL");

There is no need for that if...
r=jag
nice! sr=alecf on the prefs stuff
prefs changes checked in...  we're getting there slowly but surely.
Hmmm... I say we create a prefutilities.xul and dump some stuff in there :-)

Then you can add a <stringbundleset> to the files that need it and put the
actual <stringbundle> in there.
Or, you just kill prefutilities.js :-) (See bug 73355)
Assignee: maolson → jag
Status: ASSIGNED → NEW
Attached patch Patch for prefs / languages (obsolete) — Splinter Review
Also includes a small optimisation for the Add Languages dialog (it doesn't
build its own copy of the available languages array any more).

Timeless, could you review this or pass it on if you don't want it?
Attachment #173192 - Flags: review?(timeless)
Attached patch Same patch, diff -w (obsolete) — Splinter Review
Comment on attachment 173192 [details] [diff] [review]
Patch for prefs / languages

seems ok, but i'd rather pass :)
Attachment #173192 - Flags: review?(timeless) → superreview?(neil.parkwaycc.co.uk)
Comment on attachment 173192 [details] [diff] [review]
Patch for prefs / languages

Just by reading it two things spring to mind; the first is that the popup
dialog only uses one of the string bundles. The second is that we normally list
stringbundles near the top after the scripts. However more importantly I don't
like the way the Init method is overloaded. What should happen is this:
1. The panel's onload should be the call to parent.initPanel.
2. There should be a Startup() function for the panel. Conveniently
parent.initPanel calls this automatically.
3. The popup dialog's startup function can stay in Init.
Another point is that we generally store the stringbundle element in the
variable and call the XBL functions rather than directly calling the C++
interface; for instance this saves a parameter when calling getFormattedString,
although this doesn't help in the case of acceptBundle.
(In reply to comment #60)

Thanks for the input, Neil, really appreciated!

> Just by reading it two things spring to mind; the first is that the popup
> dialog only uses one of the string bundles. The second is that we normally 
list
> stringbundles near the top after the scripts.

Right, no problem.

> However more importantly I don't
> like the way the Init method is overloaded. What should happen is this:
> 1. The panel's onload should be the call to parent.initPanel.
> 2. There should be a Startup() function for the panel. Conveniently
> parent.initPanel calls this automatically.
> 3. The popup dialog's startup function can stay in Init.

I didn't knew what initPanel actually does. Now I do, will fix.

> Another point is that we generally store the stringbundle element in the
> variable and call the XBL functions rather than directly calling the C++
> interface; 

I wanted to keep the patch small, but again, no problem.

Since bug 280693 complains about createElement for XUL I'll also convert that to 
creatElementNS. New patch coming later today.
Attached patch New pref / languages patch (obsolete) — Splinter Review
As described above. LXR says I'm the only one using
parent.initPanel(this.baseURI). I hope that's ok, I hate repeating long string
constants that are available elsewhere.

I also fixed a bug caused by the broken array logic in AddAvailableLanguages
which could lead to 'undefined' appearing in the listbox.
To reproduce:
1. Add Afrikaans
2. Open the Add... dialog again, select Afrikaans and Albanian.
3. Click ok.

Result: Afrikaans, undefined, Albanian.
Attachment #173192 - Attachment is obsolete: true
Attachment #173193 - Attachment is obsolete: true
Attachment #173249 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Same patch, diff -w -b -6 (obsolete) — Splinter Review
Attachment #173192 - Flags: superreview?(neil.parkwaycc.co.uk)
Attached patch pref-languages.xul (only) (obsolete) — Splinter Review
Argh, I believed |this| was a reference to the page element but instead it
points to the window, just like in HTML. document.documentURI is better.
I did wonder about this.baseURI ;-)

I can't get your patches to apply:
patching pref-languages-add.xul
2 out of 2 hunks FAILED
patching pref-languages.js
7 out of 11 hunks FAILED
patching pref-languages.xul
2 out of 3 hunks FAILED
Neil, strange, the diff is against the latest version on the trunk. You don't
have my patch from bug 168728 in your tree, do you? That won't work...
You've loaded your patch into an editor which strips trailing spaces...
Attached patch pref-languages v4 (obsolete) — Splinter Review
Attachment #173249 - Attachment is obsolete: true
Attachment #173250 - Attachment is obsolete: true
Attachment #173254 - Attachment is obsolete: true
Attachment #173249 - Flags: review?(neil.parkwaycc.co.uk)
I guess I'm too late to weigh in much on this one, but here goes anyway:

I prefer strres.js.  It means I can directly access the string bundle service from JS code instead of using a "hopefully-unique" ID for yet another XUL element.  You have a problem with Moz not cacheing JS files, why not fix that instead of creating this 'stringbundle' element?  I find it much cleaner to access the stringbundle service purely from within JS code than to have this arbitrary mixing of XUL and JS, with messy grabbing of an element via Yet Another Unique ID.

Someone care to explain how this is superior?
Attachment #27439 - Attachment is obsolete: true
Attachment #28222 - Attachment description: patch - prefs v2 → patch - prefs v2 [Checkin: Comment 53]
Attachment #26606 - Attachment description: patch - remove strres.js from embedding → patch - remove strres.js from embedding [Checkin: Comment 38]
Attachment #25423 - Attachment is obsolete: true
Attachment #25870 - Attachment description: patch - resync w/ tip after bug 68505 checkin → patch - resync w/ tip after bug 68505 checkin (mailnews) [Checkin: Comment 34]
Attachment #25048 - Attachment is obsolete: true
Attachment #26765 - Attachment is obsolete: true
Attachment #26852 - Attachment description: patch - profile manager v2 → patch - profile manager v2 [Checkin: Comment 47]
Attachment #25330 - Attachment description: update (tip shifted a bit today - no functional changes) → update (tip shifted a bit today - no functional changes) (mailnews)
Attachment #25330 - Attachment is obsolete: true
Attachment #25326 - Attachment description: patch - latest thinking on the matter → patch - latest thinking on the matter (mailnews)
Attachment #25326 - Attachment is obsolete: true
Depends on: 61801, 61802, 68505
OS: Linux → All
QA Contact: jrgmorrison → xptoolkit.widgets
Hardware: PC → All
Attachment #25006 - Attachment description: patch - downloadheaders.* (includes detabbification and agreement with modeline changes) → patch - downloadheaders.* (includes detabbification and agreement with modeline changes) (mailnews/news only)
Attachment #25006 - Attachment is obsolete: true
Comment on attachment 24026 [details] [diff] [review]
[patch] Move navigator over to <stringbundle/>
[Checkin: Comment 70]

{{
2001-02-01 01:53	disttsc%bart.nl 	...  	Move over, strres.js, the new, sexy <stringbundle/> is in Browser Town. bug=56680, r=timeless, a=ben
}}
with comment 12 suggestion(s).
Attachment #24026 - Attachment description: [patch] Move navigator over to <stringbundle/> → [patch] Move navigator over to <stringbundle/> [Checkin: Comment 70]
Attachment #24935 - Attachment description: [patch] missed something in navigatorDD.js → [patch] missed something in navigatorDD.js [Moved to bug 72137]
Attachment #24935 - Attachment is obsolete: true
Attachment #28689 - Attachment description: patch - convert prefutilities.js → patch - convert prefutilities.js [Superseded by bug 73355]
Attachment #28689 - Attachment is obsolete: true
Depends on: 73355
Attachment #173254 - Attachment description: pref-languages.xul → pref-languages.xul (only)
Attachment #17153 - Attachment description: [sample-patch] use <stringbundle/> in xp filepicker → [sample-patch] use <stringbundle/> in xp filepicker [Superseded by bug 27612]
Attachment #17153 - Attachment is obsolete: true
Depends on: 27612
Blocks: 444411
Attached patch (Iv4a) <pref-languages*.*> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008070902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

"pref-languages v4", ported to </suite/>,
plus:
*a few "nits".
*removal of the obsoleted |broadcaster| element.
Assignee: jag → sgautherie.bz
Attachment #173633 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #328807 - Flags: superreview?(neil)
Attachment #328807 - Flags: review?
Attachment #328807 - Flags: review? → review?(iann_bugzilla)
(In reply to comment #72)
> Created an attachment (id=328807) [details]
> (Iv4a) <pref-languages*.*>
> 
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre)
> Gecko/2008070902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
> 
> "pref-languages v4", ported to </suite/>,
> plus:
> *a few "nits".
> *removal of the obsoleted |broadcaster| element.
> 
I would forget about making the whitespace changes to xul as this will all be changing with your patch on bug 444411
Attached patch (Iv4b) <pref-languages*.*> (obsolete) — Splinter Review
(In reply to comment #73)
> I would forget about making the whitespace changes to xul as this will all be

Iv4a, with comment 73 suggestion(s).

> changing with your patch on bug 444411

Note that I am not working on that bug.
Attachment #328977 - Flags: review?(iann_bugzilla)
Comment on attachment 328807 [details] [diff] [review]
(Iv4a) <pref-languages*.*>

The stringbundle-specific changes look OK but I didn't look at any of the other miscellaneous changes as those probably belong in bug 444411.
Attachment #328807 - Flags: superreview?(neil) → superreview+
Attachment #328807 - Attachment is obsolete: true
Attachment #328807 - Flags: review?(iann_bugzilla)
Comment on attachment 328977 [details] [diff] [review]
(Iv4b) <pref-languages*.*>

As Neil said, only the strres.js related changes should be in here r=me for just those changes.
Attachment #328977 - Flags: review?(iann_bugzilla) → review+
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Iv4b, with comment 75 & 76 suggestion(s).

Like this ?
Or even without the "onload" change ?
Attachment #328977 - Attachment is obsolete: true
Attachment #329561 - Flags: superreview?(neil)
Attachment #329561 - Flags: review?(iann_bugzilla)
Attachment #329561 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 329561 [details] [diff] [review]
(Iv4c) <pref-languages*.*>
[Checkin: Comment 79]

The new pane will need Startup() so I don't think that's a problem.
Attachment #329561 - Flags: superreview?(neil) → superreview+
Keywords: checkin-needed
Priority: P3 → --
Whiteboard: [c-n: Iv4c // Leave opened]
Depends on: 445371
Depends on: 445374
Depends on: 445376
Depends on: 445383
Depends on: 445387
Comment on attachment 329561 [details] [diff] [review]
(Iv4c) <pref-languages*.*>
[Checkin: Comment 79]

Checking in pref-languages-add.xul;
/cvsroot/mozilla/suite/common/pref/pref-languages-add.xul,v  <--  pref-languages-add.xul
new revision: 1.24; previous revision: 1.23
done
Checking in pref-languages.js;
/cvsroot/mozilla/suite/common/pref/pref-languages.js,v  <--  pref-languages.js
new revision: 1.36; previous revision: 1.35
done
Checking in pref-languages.xul;
/cvsroot/mozilla/suite/common/pref/pref-languages.xul,v  <--  pref-languages.xul
new revision: 1.52; previous revision: 1.51
done
Attachment #329561 - Attachment description: (Iv4c) <pref-languages*.*> → (Iv4c) <pref-languages*.*> (Checked in)
Keywords: checkin-needed
Whiteboard: [c-n: Iv4c // Leave opened]
No longer blocks: 444411
Attachment #329561 - Attachment description: (Iv4c) <pref-languages*.*> (Checked in) → (Iv4c) <pref-languages*.*> [Checkin: Comment 79]
Thanks Erik for all the good work he had done on this "language" patch !
Keywords: meta
Whiteboard: [Waiting for blocking bugs, for now]
Blocks: 446123
(In reply to comment #75)
> (From update of attachment 328807 [details] [diff] [review])
> The stringbundle-specific changes look OK but I didn't look at any of the other
> miscellaneous changes as those probably belong in bug 444411.

I filed bug 446123 about the additional fixes.
Depends on: 228102
Depends on: 508277
Depends on: 508280
Can this bug be closed now that Bug 445371 has been fixed? The only remaining use of strres.js that I can find is test_bug292789.html
Flags: needinfo?(sgautherie.bz)
Depends on: 909107
(In reply to Cykesiopka from comment #82)

> Can this bug be closed now that Bug 445371 has been fixed?

I prefer to leave this bug open until strres.js itself is removed.

> The only remaining use of strres.js that I can find is test_bug292789.html

Right. I filed (blocking) bug 909107.
Flags: needinfo?(sgautherie.bz)
Blocks: 916235
Bug 909107 is fixed, then strres.js is no longer used :-)

I just filed
Bug 916235 - Remove obsolete (and now unused) strres.js from the tree
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [Waiting for blocking bugs, for now]
Target Milestone: --- → mozilla26
> Target Milestone: --- → mozilla26

Ftr, this bug patches themselves were checked in years before that release.
Blocks: 917024
No longer blocks: 916235
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: