407481 - Unicode ellipsis to be used in code as well

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Hendrik Maryns]

Attachment: introduces … in nsContextMenu.js

Following bug 373623, also in the places where ellipses are introduced programmatically, to shorten long strings, the unicode symbol should be used.

Comment 1

[Reed Loden [:reed]]

So, you should probably use the new "intl.ellipsis" pref here instead of hardcoding the ellipsis.

Comment 2

[Robert Kaiser]

Yes, this sounds like a case for intl.ellipsis - esp. as there's actually something shortended there. And all such characters should go through some place in localization anyways, as in some languages (like Japanese), the unicode ellipsis is not rendered in the way we want it and needs to be replaced with dots (which, of course, can nicely be done through intl.ellipsis).

Comment 3

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

Comment on attachment 292213 [details] [diff] [review]
introduces … in nsContextMenu.js

per previous comments

Comment 5

[Masahiro YAMADA]

Attachment: Patch using intl.ellipsis rv1.0

Comment 6

[Hendrik Maryns]

(In reply to comment #5)
> Created an attachment (id=298444) [details]
> Patch using intl.ellipsis rv1.0

Looks good to me, in as far as I can tell with my limited knowledge of JS.  You’ll want to ask someone to review this, Gavin, I suppose.

I am sorry not to have tackled this myself.  I had a try, but it didn’t work, and then christmas holidays and computer trouble hindered me from getting to it.

Comment 7

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

Comment on attachment 298444 [details] [diff] [review]
Patch using intl.ellipsis rv1.0

>Index: browser/base/content/nsContextMenu.js

I'd just use: 
>+    this.ellipsis = "\u2026"; // default value
>+    try {
>+      this.ellipsis = gPrefService.getComplexValue("intl.ellipsis",
>+                                            Ci.nsIPrefLocalizedString).data;
>+    } catch (e) { }

and put it directly in the nsContextMenu constructor.

The rest looks fine. I'll r+ a new patch with that change made.

Comment 8

[Masahiro YAMADA]

Attachment: Patch using intl.ellipsis rv2.0

Comment 9

[Reed Loden [:reed]]

Comment on attachment 298659 [details] [diff] [review]
Patch using intl.ellipsis rv2.0

Use the correct ellipsis in code, too.

Comment 10

[Masahiro YAMADA]

(In reply to comment #9)
> (From update of attachment 298659 [details] [diff] [review])
> Use the correct ellipsis in code, too.
> 
Is it needed to replace "\u2026" as "…" ?

Comment 11

[Reed Loden [:reed]]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 298659 [details] [diff] [review] [details])
> > Use the correct ellipsis in code, too.
> Is it needed to replace "\u2026" as "…" ?

Ah, that comment was addressed to drivers, not to you. Don't worry about that comment. :)

Comment 12

[Masahiro YAMADA]

Sorry my patch contains CR-LF, please replace it as LF.

Comment 13

[Hendrik Maryns]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 298659 [details] [diff] [review] [details])
> > Use the correct ellipsis in code, too.
> > 
> Is it needed to replace "\u2026" as "…" ?

Maybe not, but I don’t see why not?

Comment 14

[Reed Loden [:reed]]

Gavin, is that change ok?

Comment 15

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

The current patch is fine.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 298659 [details] [diff] [review]
Patch using intl.ellipsis rv2.0

a=beltzner for 1.9 ... :)

Comment 17

[Reed Loden [:reed]]

Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.32; previous revision: 1.31
done

Comment 18

[Masahiro YAMADA]

Verified at Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012404 Minefield/3.0b3pre