445753 - "Rewrite" <nsSidebar.js> |function srGetStrBundle()|

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Serge Gautherie (:sgautherie)]

This case can not be replaced by a |<stringbundle/>|,
nor even use </toolkit/obsolete/content/strres.js>;
but it can still be improved a little.

{{
/browser/components/sidebar/src/nsSidebar.js
/suite/common/sidebar/nsSidebar.js
}}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-SM) <nsSidebar.js>

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-SM) <nsSidebar.js>

Av1-SM still had an occurrence of the old global variable name.

Comment 3

[[e-mail]]

Comment on attachment 330037 [details] [diff] [review]
(Av1a-SM) <nsSidebar.js>

>+  const strBundle = gStrBundleService.createBundle(path);
var please.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b-SM) <nsSidebar.js>

Av1a-SM, with comment 3 suggestion(s).

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-FF) <nsSidebar.js>

Sync SM -> FF:
*Copy/Port patch Av1b-SM.
*"Rewrite" |nsSidebar.prototype.QueryInterface()|.
*Do the white-space cleanups.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 334908 [details] [diff] [review]
(Bv1-FF) <nsSidebar.js>

>diff --git a/browser/components/sidebar/src/nsSidebar.js b/browser/components/sidebar/src/nsSidebar.js

>+  var strBundle = gStrBundleService.createBundle(path);
>+  if (!strBundle)

createBundle will either return a bundle, or throw as far as I can tell, so this check is useless.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> createBundle will either return a bundle, or throw as far as I can tell, so
> this check is useless.

Right:
createBundle() -> getStringBundle() -> |if (!bundle) return NS_ERROR_OUT_OF_MEMORY;|

Then, should I simply remove the test and let the exception spring, as it actually does,
or add another try+catch and return null here too, as it seems it was initially wanted ?
Currently, I seems odd to have the two behaviors in the same function :-<

Comment 8

[:Gavin Sharp [email: [e-mail]]]

OOM is exceptional, just let it throw in that case.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I would remove the other try/catch as well, that getService failing should also be pretty exceptional and it looks like no one null checks the return value of this function anyways.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> OOM is exceptional, just let it throw in that case.

(In reply to comment #9)
> I would remove the other try/catch as well, that getService failing should also
> be pretty exceptional and it looks like no one null checks the return value of
> this function anyways.

Fully agreed :-)

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av2-SM) <nsSidebar.js> [Checkin: Comment 18]

Av1a-SM, with comment 9 suggestion(s).

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2-FF) <nsSidebar.js> [Checkin: Comment 20]

Bv1-FF, with comment 9 suggestion(s).

Comment 13

[[e-mail]]

Comment on attachment 334984 [details] [diff] [review]
(Av2-SM) <nsSidebar.js>
[Checkin: Comment 18]

>Bug 445753 � "Rewrite" <nsSidebar.js> |function srGetStrBundle()|
Does TortoiseHg not handle Unicode correctly? ;-)

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> Does TortoiseHg not handle Unicode correctly? ;-)

Ah ! It must be caused by Windows (2000) Notepad, which I use to input the commit comment.
This character (in the patch file) is actually 0x96 (in Windows-1252).

I don't know why SeaMonkey tries to display it as UTF-8 :-/
It displays fine as ISO-8859-1...

Is this causing other issues ?

Comment 15

[[e-mail]]

iirc hg is supposed to use UTF-8

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 334988 [details] [diff] [review]
(Bv2-FF) <nsSidebar.js>
[Checkin: Comment 20]

at this point it seems silly to have a helper at all, but... let's not get carried away.

Comment 17

[Tony Mechelynck [:tonymec]]

(In reply to comment #14)
> (In reply to comment #13)
> > Does TortoiseHg not handle Unicode correctly? ;-)
> 
> Ah ! It must be caused by Windows (2000) Notepad, which I use to input the
> commit comment.
> This character (in the patch file) is actually 0x96 (in Windows-1252).
> 
> I don't know why SeaMonkey tries to display it as UTF-8 :-/
> It displays fine as ISO-8859-1...
> 
> Is this causing other issues ?
> 

Windows-1252 0x96 corresponds IIUC to Unicode U+2013 i.e. –
in Latin1 (ISO-8859-1) 0x96 it is actually a control character, "START OF GUARDED AREA". On some systems (esp. some Windows systems) it will be displayed as if it were Windows-1252, on others you'll get the "unknown character" glyph.

Bugzilla pages are in UTF-8 nowadays; not sure about Mercurial but I'd bet on UTF-8 for anything recent which was originally developed on Unix.

Comment 18

[Frank Wein [:mcsmurf]]

Comment on attachment 334984 [details] [diff] [review]
(Av2-SM) <nsSidebar.js>
[Checkin: Comment 18]

Checked in, changeset 076411b96f5c

Comment 19

[Philip Chee]

+  if (!gStrBundleService)
+    gStrBundleService =
+      Components.classes["@mozilla.org/intl/stringbundle;1"]
+                .getService(Components.interfaces.nsIStringBundleService);
+
+  return gStrBundleService.createBundle(path);

Hmm. FYI. Firefox is moving towards using smart GETTERs instead.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 334988 [details] [diff] [review]
(Bv2-FF) <nsSidebar.js>
[Checkin: Comment 20]

http://hg.mozilla.org/mozilla-central/rev/e3649faaaf18

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> at this point it seems silly to have a helper at all, but... let's not get
> carried away.

(In reply to comment #19)
> Hmm. FYI. Firefox is moving towards using smart GETTERs instead.

I filed bug 453980 as a followup.