128530 - Use |nsISaveAsCharset::attr_EntityAfterCharsetConv| instead of |nsISaveAsCharset::attr_EntityBeforeCharsetConv| in GTK+/Xlib toolkits

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Roland Mainz]

[rbs wrote in bug 33498 comment #9]
> In the process, I noted that other platforms are using 
> nsISaveAsCharset::attr_EntityBeforeCharsetConv. I think what people want is
> nsISaveAsCharset::attr_EntityAfterCharsetConv. 
>
> The former will convert to entities _before_ doing the transliteration, 
> thereby possibly converting some glyphs that could be transliterated. The 
> latter will transliterate, and then the glyphs that couldn't be transliterated 
> will be converted to their entity representation if possible or to question 
> marks if not possible. Finally, just in case, I also made sure that the Win32 
> routines that expect the converted result are 0-length safe. Not sure about 
> other platforms here.

RFE: Do it!

Comment 1

[Roland Mainz]

Attachment: Patch for 2002-02-28-08-trunk

Comment 2

[Roland Mainz]

Requesting r=/sr=/a= ...

Comment 3

[rbs]

Comment on attachment 72147 [details] [diff] [review]
Patch for 2002-02-28-08-trunk

r=rbs

Comment 4

[Roland Mainz]

rbs:
Do we have any testcase to see the "before patch <--> after patch"-effect ?

Comment 5

[kill this account]

I would like to (strongly) recommend we have Frank Tang, the converter expert,
look at this.

Comment 6

[rbs]

>rbs:
>Do we have any testcase to see the "before patch <--> after patch"-effect ?

No. But you can easily make one that will exercise the code as per my above 
description. See transliterate.properties for a sample of characters that can be
transliterated. To test if the code 0-length safe, you could try some of the
empty characters, eg., <span>&zwsp;&InvisibleComma;...etc</span>  (while making
sure that you have no fonts with glyphs for them...).

Comment 7

[Frank Tang]

I think rbs rbs is right, but I don't really think this change will have any
effect since probably no characters which can be covered by "ISO-8859-1" will
ever hit the code here unless there are no ISO-8859-1 font installed on the
system, which is very very very unlikely.

I think we should not check in this patch now since:
1. there are no obvious gain (no functionality gain or loss with or without this
patch)
and 2. there are risk (tiny but not zero) if we take this patch. 

I think there are no reason to take a zero gain tiny risk patch in this stage.
why don'w we save it till post m1.0?

Comment 8

[rbs]

Because it frees your mind, reduce bug counts, and allows concentrating fully on
other important things.

Comment 9

[Roland Mainz]

Just wondering - why are we using "ISO-8859-1" and not $LANG or $LC_CTYPE (which
ever one is the correct one) - and fall back to "ISO-8859-1" on demand ?

Comment 10

[rbs]

waterson asked similarly in bug 33498 comment #18, reply: bug 33498 comment #18

Comment 11

[Roland Mainz]

ftang:
Windows works with this since eternity... I think Windows and X11(=GTK,Xlib)
should do the same.

Unless you see major problems I would like to get sr= and a= for trunk, please
...

Comment 12

[Roland Mainz]

Requesting sr= ...

Comment 13

[kill this account]

Roland: could you point out some specific cases for this?

Without a strong case for doing this I would recommend we avoid the risk
till after m1.0.

Comment 14

[Roland Mainz]

Brian Stell wrote:
> Roland: could you point out some specific cases for this?

Mhhh... not really... 

... rbs - do you have one ?

Comment 15

[rbs]

I just happen to see a checkin comment on a related issue:
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1018051860&maxdate=1018052880&who=nhotta%25netscape.com

(The trivial patch for this bug has been sitting here with a r= all along, it is
not really the best use of my time to drag over such intelligible matters on and
on.)

Comment 16

[Marc Attinasi]

Comment on attachment 72147 [details] [diff] [review]
Patch for 2002-02-28-08-trunk

rs=attinasi

Comment 17

[Roland Mainz]

Attachment: Patch for 2002-04-09-08-trunk (merge to tip only)

Comment 18

[Roland Mainz]

Checked into "trunk" by timeless
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1018746780&maxdate=1018747800&who=timeless%25mac.com)
...

----

rbs:
Should I keep this bug open and wait a while and then request a= for the
1.0-branch again (assuming noone files a bug again this... :) ?

Comment 19

[rbs]

Why bother that much for something that minor? Just close the bug and move on,
and the patch will eventually make its way to another Mozilla release.

Comment 20

[Roland Mainz]

OK, marking bug as FIXED...