172491 - RFE: Add support for ISO-8859-6-16 encoded fonts

Status: RESOLVED WONTFIX

(Core :: Layout: Text and Fonts, enhancement)

Description

[Roland Mainz]

RFE: Add support for ISO-8859-6-16 encoded fonts (see
http://www.langbox.com/arabic/fontara16.html).

Comment 1

[Katsuhiko Momoi]

I am curious. Is this charset officially registered in IANA charset 
registry?
http://www.iana.org/assignments/character-sets

Comment 2

[Simon Montagu :smontagu]

Thanks for pointing that out. This encoding doesn't seem to be registered in
IANA, so we should use a name like x-iso-8859-6-16.

As far as I know, it's never used as a content encoding, and the intention is
only to add a decoder to be used by the GTK and Xlib rendering layers.

Comment 3

[Roland Mainz]

Simon Montagu wrote:
> Thanks for pointing that out. This encoding doesn't seem to be registered in
> IANA, so we should use a name like x-iso-8859-6-16.

AFAIK ISO8859-6.8x isn't registered by IANA either... this is just another
encoding for arabic X11 fonts...

Comment 4

[Roland Mainz]

Attachment: Arabic *-iso8859-6.16 encoded bitmap fonts for testing as tar.gz

The tar.gz archive attached here contains two BDF sample fonts in the
"*-iso8859-6.16"-encoding (donation from langbox.com :)
-lbi-naskhi16-medium-r-normal--16-160-75-75-p-22-iso8859-6.16
-lbi-arabic16-medium-r-normal--15-130-75-75-c-100-iso8859-6.16

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch v.0

There are still some issues to be resolved here. Arabic text looks fine, but
English text on Arabic pages isn't always shown correctly.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 105522 [details] [diff] [review]
Patch v.0

The symmetric characters '(' ')' '[' and ']' also need correction. The
8859-6-16 encoding reverses them!

Comment 7

[Roland Mainz]

Comment on attachment 105522 [details] [diff] [review]
Patch v.0

Nice patch... can you fix the nits listed in your prevous comment and update
the patch to match "trunk" (I'd like to see it as part of Mozilla 1.4a if
possible... :) , please ?

Comment 8

[Simon Montagu :smontagu]

Looking again at the font mapping at
http://www.langbox.com/arabic/fontara16.html, the encoding maps Arabic glyphs to
codepoints in the US-ASCII range, which makes it impractical for most web
content. This is the cause of the problem I mention in comment 5, and it will
occur on any site that does not rigidly mark up every change of language with
<span lang="ar"> <span lang="en"> etc.

Comment 9

[Simon Montagu :smontagu]

I was hasty WONTFIXing this, the problems are all soluble

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch v.1

This also moves both the Langbox Arabic encoding into the 
#ifdef MOZ_EXTRA_X11CONVERTERS, since we don't need them elsewhere

Comment 11

[kill this account]

I'm competent to review the changes to nsFontMetricsGTK.cpp (and probably
nsFontMetricsXlib.cpp) and those look fine to me, so r=[e-mail]
on those parts.

The rest needs a qualified reviewer like ftang to approve them.

Comment 12

[Roland Mainz]

The patch is OK in general (it's a simple |nsBasicEncoder|, it's unlikely to
make much mistakes in this code... :) ...
... jumping into superstrict nitpicking review mode I see some minor issues:

-- SON (start-of-nitpicking) -- 
> Index: intl/uconv/src/nsUConvModule.cpp
> ===================================================================
> RCS file: /cvsroot/mozilla/intl/uconv/src/nsUConvModule.cpp,v
> retrieving revision 1.42
> diff -u -r1.42 nsUConvModule.cpp
> --- intl/uconv/src/nsUConvModule.cpp    15 Mar 2003 01:02:56 -0000      1.42
> +++ intl/uconv/src/nsUConvModule.cpp    21 Mar 2003 07:02:50 -0000
> @@ -197,7 +197,10 @@
>  #include "nsUnicodeToMacGujarati.h"
>  #include "nsUnicodeToMacGurmukhi.h"
>  #include "nsUnicodeToMacHebrew.h"
> +#ifdef MOZ_EXTRA_X11CONVERTERS
>  #include "nsUnicodeToLangBoxArabic8.h"
> +#include "nsUnicodeToLangBoxArabic16.h"
> +#endif

#endif /* MOZ_EXTRA_X11CONVERTERS */ (assuming that this #if...#endif may grow)
...

>  // ucvibm
>  #include "nsUCvIBMCID.h"
> @@ -364,7 +367,10 @@
>  NS_UCONV_REG_UNREG_ENCODER("UTF-16" , NS_UNICODETOUTF16_CID)
>  NS_UCONV_REG_UNREG_ENCODER("Adobe-Symbol-Encoding" , NS_UNICODETOSYMBOL_CID)
>  NS_UCONV_REG_UNREG_ENCODER("x-zapf-dingbats" , NS_UNICODETOZAPFDINGBATS_CID)
> +#ifdef MOZ_EXTRA_X11CONVERTERS
>  NS_UCONV_REG_UNREG_ENCODER("x-iso-8859-6-8-x" ,
NS_UNICODETOLANGBOXARABIC_CID)
> +NS_UCONV_REG_UNREG_ENCODER("x-iso-8859-6-16" ,
NS_UNICODETOLANGBOXARABIC16_CID)
> +#endif

...

>    // ucvibm
>  NS_UCONV_REG_UNREG("IBM850", NS_CP850TOUNICODE_CID, NS_UNICODETOCP850_CID)
> @@ -462,7 +468,10 @@
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsUnicodeToUTF16);
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsUnicodeToUTF32BE);
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsUnicodeToUTF32LE);
> +#ifdef MOZ_EXTRA_X11CONVERTERS
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsUnicodeToLangBoxArabic8);
> +NS_GENERIC_FACTORY_CONSTRUCTOR(nsUnicodeToLangBoxArabic16);
> +#endif

see above...

>  // ucvibm
>  
> @@ -1343,11 +1352,18 @@
>      NS_UNICODEENCODER_CONTRACTID_BASE "x-mac-hebrew",
>      nsUnicodeToMacHebrewConstructor, 
>    },
> +#ifdef MOZ_EXTRA_X11CONVERTERS
>    { 
>      ENCODER_NAME_BASE "x-iso-8859-6-8-x" , NS_UNICODETOLANGBOXARABIC_CID, 
>      NS_UNICODEENCODER_CONTRACTID_BASE "x-iso-8859-6-8-x",
>      nsUnicodeToLangBoxArabic8Constructor, 
>    },
> +  { 
> +    ENCODER_NAME_BASE "x-iso-8859-6-16" , NS_UNICODETOLANGBOXARABIC16_CID, 
> +    NS_UNICODEENCODER_CONTRACTID_BASE "x-iso-8859-6-16",
> +    nsUnicodeToLangBoxArabic16Constructor, 
> +  },
> +#endif

ditto.

>    // ucvibm
>    { 
>      DECODER_NAME_BASE "IBM850" , NS_CP850TOUNICODE_CID, 
> Index: intl/uconv/ucvlatin/Makefile.in
> ===================================================================
> RCS file: /cvsroot/mozilla/intl/uconv/ucvlatin/Makefile.in,v
> retrieving revision 1.49
> diff -u -r1.49 Makefile.in
> --- intl/uconv/ucvlatin/Makefile.in     31 Jan 2003 23:26:15 -0000      1.49
> +++ intl/uconv/ucvlatin/Makefile.in     21 Mar 2003 07:02:50 -0000
> @@ -129,6 +129,7 @@
>                 nsUnicodeToKOI8R.cpp \
>                 nsUnicodeToKOI8U.cpp \
>                 nsUnicodeToLangBoxArabic8.cpp \
> +               nsUnicodeToLangBoxArabic16.cpp \
>                 nsUnicodeToMacCE.cpp \
>                 nsUnicodeToMacGreek.cpp \
>                 nsUnicodeToMacTurkish.cpp \
> Index: intl/uconv/ucvlatin/nsUCvLatinCID.h
> ===================================================================
> RCS file: /cvsroot/mozilla/intl/uconv/ucvlatin/nsUCvLatinCID.h,v
> retrieving revision 1.36
> diff -u -r1.36 nsUCvLatinCID.h
> --- intl/uconv/ucvlatin/nsUCvLatinCID.h 8 Oct 2002 23:58:43 -0000       1.36
> +++ intl/uconv/ucvlatin/nsUCvLatinCID.h 21 Mar 2003 07:02:50 -0000
> @@ -601,9 +601,11 @@
>  #define NS_UNICODETOUESCAPE_CID \
>    { 0x319ff9c3, 0x51d2, 0x11d3, {0xb3, 0xc3, 0x0, 0x80, 0x5f, 0x8a, 0x66,
0x70}}
>  
> +#ifdef MOZ_EXTRA_X11CONVERTERS
>  // {49B38F11-6193-11d3-B3C5-00805F8A6670}
>  #define NS_UNICODETOLANGBOXARABIC8_CID \
>    { 0x49b38f11, 0x6193, 0x11d3, {0xb3, 0xc5, 0x0, 0x80, 0x5f, 0x8a, 0x66,
0x70}}
> +#endif

...

>  // {49B38F12-6193-11d3-B3C5-00805F8A6670}
>  #define NS_UNICODETOUTF16_CID \
> @@ -661,8 +663,14 @@
>  #define NS_UNICODETOMACGURMUKHI_CID \
>    { 0x6803cacf, 0x1e3b, 0x11d5, { 0xa1, 0x45, 0x0, 0x50, 0x4, 0x83, 0x21,
0x42 } }
>  
> +#ifdef MOZ_EXTRA_X11CONVERTERS
>  // {4DBBD94F-0153-44cb-966A-7F39B9CB477D}
>  #define NS_UNICODETOLANGBOXARABIC_CID \
>    { 0x4dbbd94f, 0x153, 0x44cb, { 0x96, 0x6a, 0x7f, 0x39, 0xb9, 0xcb, 0x47,
0x7d } }
> +
> +// {8E269A63-94B4-46e5-A31F-11F0EDE6065B}
> +#define NS_UNICODETOLANGBOXARABIC16_CID \
> +  { 0x8e269a63, 0x94b4, 0x46e5, { 0xa3, 0x1f, 0x11, 0xf0, 0xed, 0xe6, 0x6,
0x5b } }
> +#endif

...

>  #endif /* nsUCvLatinCID_h___ */
> Index: intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.cpp
> ===================================================================
> RCS file: intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.cpp
> diff -N intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.cpp
> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.cpp  21 Mar 2003 07:02:50
-0000
> @@ -0,0 +1,327 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
*/
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: NPL 1.1/GPL 2.0/LGPL 2.1
> + *
> + * The contents of this file are subject to the Netscape Public License
> + * Version 1.1 (the "License"); you may not use this file except in
> + * compliance with the License. You may obtain a copy of the License at
> + * http://www.mozilla.org/NPL/
> + *
> + * Software distributed under the License is distributed on an "AS IS" basis,
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is Mozilla Communicator client code.
> + *
> + * The Initial Developer of the Original Code is
> + * Netscape Communications Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2002
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s): Simon Montagu

email?
+ * Contributor(s):
+ *   Simon Montagu <[e-mail]>

[snip]
> +  0x79, // U+FEFB ARABIC LIGATURE LAM ALEF ISOLATED FORM
> +  0xFD, // U+FEFC ARABIC LIGATURE LAM ALEF FINAL FORM
        ^^^

One comma too much...

> + };
> +
> +NS_IMETHODIMP nsUnicodeToLangBoxArabic16::Convert(
> +      const PRUnichar * aSrc, PRInt32 * aSrcLength,
> +      char * aDest, PRInt32 * aDestLength)
[snip]
> +NS_IMETHODIMP nsUnicodeToLangBoxArabic16::FillInfo(PRUint32* aInfo)
> +{
> +   PRUnichar i;
> +
> +   // ASCII range
> +   for(i=0x0000; i <= 0x007F; i++)
> +     CLEAR_REPRESENTABLE(aInfo, i);

What about referencing http://bugzilla.mozilla.org/show_bug.cgi?id=172491#c8 ?
:)

> +   for(i=0x0020; i <= 0x002B; i++)
> +     SET_REPRESENTABLE(aInfo, i);
> +   for(i=0x002D; i <= 0x002F; i++)
> +     SET_REPRESENTABLE(aInfo, i);
> +   SET_REPRESENTABLE(aInfo, 0x003A);
> +   for(i=0x003C; i <= 0x003E; i++)
> +     SET_REPRESENTABLE(aInfo, i);
> +   SET_REPRESENTABLE(aInfo, 0x0040);
> +   for(i=0x005B; i <= 0x005F; i++)
[snip]
> Index: intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.h
> ===================================================================
> RCS file: intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.h
> diff -N intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.h
> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ intl/uconv/ucvlatin/nsUnicodeToLangBoxArabic16.h    21 Mar 2003 07:02:50
-0000
> @@ -0,0 +1,75 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
*/
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: NPL 1.1/GPL 2.0/LGPL 2.1
> + *
> + * The contents of this file are subject to the Netscape Public License
> + * Version 1.1 (the "License"); you may not use this file except in
> + * compliance with the License. You may obtain a copy of the License at
> + * http://www.mozilla.org/NPL/
> + *
> + * Software distributed under the License is distributed on an "AS IS" basis,
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is Mozilla Communicator client code.
> + *
> + * The Initial Developer of the Original Code is 
> + * Netscape Communications Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2002
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s): Simon Montagu

email?

> + *
> + *
> + * Alternatively, the contents of this file may be used under the terms of
> + * either the GNU General Public License Version 2 or later (the "GPL"), or
> + * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> + * in which case the provisions of the GPL or the LGPL are applicable instead
> + * of those above. If you wish to allow use of your version of this file only
> + * under the terms of either the GPL or the LGPL, and not to allow others to
> + * use your version of this file under the terms of the NPL, indicate your
> + * decision by deleting the provisions above and replace them with the notice
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the NPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +#ifndef nsUnicodeToLangBoxArabic16_h__
> +#define nsUnicodeToLangBoxArabic16_h__
> +#include "nsUCSupport.h"

Actually a |#ifndef nsUCSupport_h___ ... #endif| would be nice - but
nsUCSupport.h uses |nsUCvJaSupport_h___| instead (why?!) ...

> +
> +//----------------------------------------------------------------------
> +// Class nsUnicodeToLangBoxArabic16 [declaration]
> +class nsUnicodeToLangBoxArabic16 : public nsBasicEncoder
> +{
[snip]
> +};
> +
> +#endif // nsUnicodeToLangBoxArabic16_h__

#endif // !nsUnicodeToLangBoxArabic16_h__
-- EON (end-of-nitpicking) --

Compiles, browses and prints arabic (using Xprint) and the patch is OK in
general ...

... r=[e-mail] (no need to file a new patch
before checkin unless the superreviewer wants to see other issues fixed, too)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   if(*aSrc<=0) {
+      *aDestLength = 0;
+      return NS_OK;
+   }

Isn't this supposed to be "*aSrcLength"?
If it is supposed to be "*aSrcLength", you can get rid of the whole 'if'
statement because the rest of the code will Do The Right Thing.

Comment 14

[Roland Mainz]

Robert O'Callahan wrote:
> +   if(*aSrc<=0) {
> +      *aDestLength = 0;
> +      return NS_OK;
> +   }
>
> Isn't this supposed to be "*aSrcLength"?

Yes, this should be AFAIK "*aSrcLength"... OK, I am blind... ;-/
... and |nsUnicodeToLangBoxArabic8| has the same bug...

> If it is supposed to be "*aSrcLength", you can get rid of the whole 'if'
> statement because the rest of the code will Do The Right Thing.

Well, this would be a shortcut and to safeguard against later changes below in
the code...

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should just get the while loop right and not write unnecessary code. Please
take the "if" out in both files where it appears.

Comment 16

[Simon Montagu :smontagu]

Attachment: Patch v.2

Addressed all but the most super-pedantic nits.

Comment 17

[Roland Mainz]

Comment on attachment 118384 [details] [diff] [review]
Patch v.2

nice work :)
r=[e-mail]

Comment 18

[Simon Montagu :smontagu]

Patch checked in. Reassigning to gisburn to deal with release notes.

Comment 19

[Roland Mainz]

Filed http://bugs.xfree86.org//cgi-bin/bugzilla/show_bug.cgi?id=420 ("RFE: Add
encodings files for Arabic LangBox encodings iso8859-6.8, iso8859-6.8x and
iso8859-6.16") to get support for these encodings in Xfree86...

Comment 20

[Prognathous]

The patch was checked in over a year ago, is there any reason why this bug is
still open?

Prog.

Comment 21

[Uri Bernstein]

(In reply to comment #20)
> The patch was checked in over a year ago, is there any reason why this bug is
> still open?
> 
> Prog.

No reply for the last 3 and a half years, so I'm going to go ahead and assume "no".

Comment 22

[Simon Montagu :smontagu]

Oh historical irony. Since this was left unresolved until now (waiting for the release notes mentioned in comment 18), and the code checked in in 2003 was removed just last week in bug 413613, it should really be WONTFIX.