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

RESOLVED FIXED in mozilla26

Status

()

Core
XUL
RESOLVED FIXED
18 years ago
5 years ago

People

(Reporter: Peter ``jag'' Annema, Assigned: sgautherie)

Tracking

(Blocks: 1 bug, {meta})

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 18 obsolete attachments)

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
Ian Neal
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
<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.
(Reporter)

Comment 1

18 years ago
Created attachment 17153 [details] [diff] [review]
[sample-patch] use <stringbundle/> in xp filepicker
[Superseded by bug 27612]
I like it.  r=brendan@mozilla.org.

/be
(Reporter)

Comment 3

18 years ago
In that case I'll spend some time tracking module owners and creating a bunch of
patches :-)

Comment 4

18 years ago
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
(Reporter)

Comment 6

18 years ago
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
(Reporter)

Comment 8

18 years ago
Suggestions for a reliable process monitor on linux?
(Reporter)

Comment 9

17 years ago
Btw, we could lazily initialize _bundle.
(Reporter)

Comment 10

17 years ago
Created attachment 24026 [details] [diff] [review]
[patch] Move navigator over to <stringbundle/>
[Checkin: Comment 70]

Comment 11

17 years ago
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
(Reporter)

Comment 13

17 years ago
Created attachment 24935 [details] [diff] [review]
[patch] missed something in navigatorDD.js
[Moved to bug 72137]

Comment 14

17 years ago
r=blake

Comment 15

17 years ago
Created attachment 25006 [details] [diff] [review]
patch - downloadheaders.* (includes detabbification and agreement with modeline changes) (mailnews/news only)

Comment 16

17 years ago
+    if (window.arguments && window.arguments[0]) {
"arguments" in window, no?

Comment 17

17 years ago
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

Comment 18

17 years ago
Created attachment 25048 [details] [diff] [review]
patch - mailnews/* (most anyway)

Comment 19

17 years ago
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

Comment 20

17 years ago
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
(Reporter)

Comment 21

17 years ago
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 ;-)
(Reporter)

Comment 22

17 years ago
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");
+    }

Comment 23

17 years ago
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.
(Reporter)

Comment 24

17 years ago
> 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.

Comment 25

17 years ago
Created attachment 25326 [details] [diff] [review]
patch - latest thinking on the matter (mailnews)

Comment 26

17 years ago
Created attachment 25330 [details] [diff] [review]
update (tip shifted a bit today - no functional changes) (mailnews)
(Reporter)

Comment 27

17 years ago
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

?

Comment 28

17 years ago
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

Comment 29

17 years ago
Created attachment 25423 [details] [diff] [review]
patch - latest mailnews stuff
(Reporter)

Comment 30

17 years ago
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.

Comment 31

17 years ago
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).

Comment 32

17 years ago
Created attachment 25870 [details] [diff] [review]
patch - resync w/ tip after bug 68505 checkin (mailnews)
[Checkin: Comment 34]

Comment 33

17 years ago
sr=bienvenu for the mailnews stuff.

Comment 34

17 years ago
blake checked in the mailnews stuff today.  More to come in other areas.

Comment 35

17 years ago
Created attachment 26606 [details] [diff] [review]
patch - remove strres.js from embedding
[Checkin: Comment 38]

Comment 36

17 years ago
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.

Comment 37

17 years ago
r=blake

Comment 38

17 years ago
Embedding portion checked in (sr=blizzard on irc)

Profile manager diff coming up.

Comment 39

17 years ago
Created attachment 26765 [details] [diff] [review]
patch - profile manager changes

Comment 40

17 years ago
 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

Comment 42

17 years ago
brendan's right. onload handlers should return a boolean.

Comment 43

17 years ago
Created attachment 26852 [details] [diff] [review]
patch - profile manager v2
[Checkin: Comment 47]

Comment 44

17 years ago
html:iframe => iframe
the .replace() stuff is more important. jag want to offer some help?

Comment 45

17 years ago
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.

Comment 46

17 years ago
r=timeless w/ leway to fix stuff i've mentioned. fwiw the replace stuff is now 
covered by another bug

Comment 47

17 years ago
profile manager stuff checked in - a=ben via irc.

Comment 48

17 years ago
Created attachment 27439 [details] [diff] [review]
patch - prefs
(Reporter)

Comment 49

17 years ago
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...

Comment 50

17 years ago
Created attachment 28222 [details] [diff] [review]
patch - prefs v2
[Checkin: Comment 53]
(Reporter)

Comment 51

17 years ago
r=jag

Comment 52

17 years ago
nice! sr=alecf on the prefs stuff

Comment 53

17 years ago
prefs changes checked in...  we're getting there slowly but surely.

Comment 54

17 years ago
Created attachment 28689 [details] [diff] [review]
patch - convert prefutilities.js
[Superseded by bug 73355]
(Reporter)

Comment 55

17 years ago
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.
(Reporter)

Comment 56

17 years ago
Or, you just kill prefutilities.js :-) (See bug 73355)

Updated

14 years ago
Assignee: maolson → jag
Status: ASSIGNED → NEW

Comment 57

13 years ago
Created attachment 173192 [details] [diff] [review]
Patch for prefs / languages

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)

Comment 58

13 years ago
Created attachment 173193 [details] [diff] [review]
Same patch, diff -w

Comment 59

13 years ago
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 60

13 years ago
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.

Comment 61

13 years ago
(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.

Comment 62

13 years ago
Created attachment 173249 [details] [diff] [review]
New pref / languages patch

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)

Comment 63

13 years ago
Created attachment 173250 [details] [diff] [review]
Same patch, diff -w -b -6

Updated

13 years ago
Attachment #173192 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 64

13 years ago
Created attachment 173254 [details] [diff] [review]
pref-languages.xul (only)

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.

Comment 65

13 years ago
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

Comment 66

13 years ago
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...

Comment 67

13 years ago
You've loaded your patch into an editor which strips trailing spaces...

Comment 68

13 years ago
Created attachment 173633 [details] [diff] [review]
pref-languages v4
Attachment #173249 - Attachment is obsolete: true
Attachment #173250 - Attachment is obsolete: true
Attachment #173254 - Attachment is obsolete: true

Updated

13 years ago
Attachment #173249 - Flags: review?(neil.parkwaycc.co.uk)

Comment 69

12 years ago
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?
(Assignee)

Updated

10 years ago
Attachment #27439 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #28222 - Attachment description: patch - prefs v2 → patch - prefs v2 [Checkin: Comment 53]
(Assignee)

Updated

10 years ago
Attachment #26606 - Attachment description: patch - remove strres.js from embedding → patch - remove strres.js from embedding [Checkin: Comment 38]
(Assignee)

Updated

10 years ago
Attachment #25423 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #25870 - Attachment description: patch - resync w/ tip after bug 68505 checkin → patch - resync w/ tip after bug 68505 checkin (mailnews) [Checkin: Comment 34]
(Assignee)

Updated

10 years ago
Attachment #25048 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #26765 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #26852 - Attachment description: patch - profile manager v2 → patch - profile manager v2 [Checkin: Comment 47]
(Assignee)

Updated

10 years ago
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
(Assignee)

Updated

10 years ago
Attachment #25326 - Attachment description: patch - latest thinking on the matter → patch - latest thinking on the matter (mailnews)
Attachment #25326 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Depends on: 61801, 61802, 68505
OS: Linux → All
QA Contact: jrgmorrison → xptoolkit.widgets
Hardware: PC → All
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 70

10 years ago
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]
(Assignee)

Updated

10 years ago
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
(Assignee)

Updated

10 years ago
Attachment #28689 - Attachment description: patch - convert prefutilities.js → patch - convert prefutilities.js [Superseded by bug 73355]
Attachment #28689 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Depends on: 73355
(Assignee)

Updated

10 years ago
Attachment #173254 - Attachment description: pref-languages.xul → pref-languages.xul (only)
(Assignee)

Updated

10 years ago
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
(Assignee)

Updated

10 years ago
Depends on: 27612
(Assignee)

Updated

10 years ago
Blocks: 444411
(Assignee)

Comment 72

10 years ago
Created attachment 328807 [details] [diff] [review]
(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.
Assignee: jag → sgautherie.bz
Attachment #173633 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #328807 - Flags: superreview?(neil)
Attachment #328807 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #328807 - Flags: review? → review?(iann_bugzilla)

Comment 73

10 years ago
(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
(Assignee)

Comment 74

10 years ago
Created attachment 328977 [details] [diff] [review]
(Iv4b) <pref-languages*.*>

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

10 years ago
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+
(Assignee)

Updated

10 years ago
Attachment #328807 - Attachment is obsolete: true
Attachment #328807 - Flags: review?(iann_bugzilla)

Comment 76

10 years ago
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+
(Assignee)

Comment 77

10 years ago
Created attachment 329561 [details] [diff] [review]
(Iv4c) <pref-languages*.*>
[Checkin: Comment 79]

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

Updated

10 years ago
Attachment #329561 - Flags: review?(iann_bugzilla) → review+

Comment 78

10 years ago
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+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Priority: P3 → --
Whiteboard: [c-n: Iv4c // Leave opened]
(Assignee)

Updated

10 years ago
Depends on: 445371
(Assignee)

Updated

10 years ago
Depends on: 445374
(Assignee)

Updated

10 years ago
Depends on: 445376
(Assignee)

Updated

10 years ago
Depends on: 445383
(Assignee)

Updated

10 years ago
Depends on: 445387

Comment 79

10 years ago
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)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Iv4c // Leave opened]

Updated

10 years ago
No longer blocks: 444411
(Assignee)

Updated

10 years ago
Attachment #329561 - Attachment description: (Iv4c) <pref-languages*.*> (Checked in) → (Iv4c) <pref-languages*.*> [Checkin: Comment 79]
(Assignee)

Comment 80

10 years ago
Thanks Erik for all the good work he had done on this "language" patch !
Keywords: meta
Whiteboard: [Waiting for blocking bugs, for now]
(Assignee)

Updated

10 years ago
Blocks: 446123
(Assignee)

Comment 81

10 years ago
(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.
(Assignee)

Updated

10 years ago
Depends on: 228102
(Assignee)

Updated

9 years ago
Depends on: 508277
(Assignee)

Updated

9 years ago
Depends on: 508280

Comment 82

5 years ago
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)
(Assignee)

Updated

5 years ago
Depends on: 909107
(Assignee)

Comment 83

5 years ago
(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)
(Assignee)

Updated

5 years ago
Blocks: 916235
(Assignee)

Comment 84

5 years ago
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
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [Waiting for blocking bugs, for now]
Target Milestone: --- → mozilla26
(Assignee)

Comment 85

5 years ago
> 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.