126920 - [FIX]Japanese printing does not work at all

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Masaki Katakai]

Japanese printing does not work at all on nightly.
All japanese characters are drawn as rectangle.

Comment 1

[Masaki Katakai]

At nsPostScriptObj.cpp 1.65 changing, pref name has been
changed but we need to change the length for Cut().

Found a problem, preferences have been changed, but argument
has not been changed.

   if (strstr(aName, "print.postscript.nativefont.")) {
-    lang.Cut(0, 19);
+    lang.Cut(0, 28);
   } else if (strstr(aName, "print.postscript.unicodefont.")) {
-    lang.Cut(0, 20);
+    lang.Cut(0, 29);
   }

Is it better to use strlen() to determine the length?

Comment 2

[Masaki Katakai]

Attachment: proposed path

Comment 3

[Masaki Katakai]

Rods, can you take this?

Comment 4

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 5

[Marc Attinasi]

Comment on attachment 70678 [details] [diff] [review]
proposed path

This is lame.  We already see what happens when you use hard-coded numbers -
things break.  Please at least add an assertion that the magic number used
equals the strlen of the string used.  With that check for correctness, the
next change probably won't cause printing to break.  sr=attinasi assuming that
change is made.

Comment 6

[timeless]

I think NS_NAMED_LITERAL_CSTRING would be more to my liking, if you can wait a
bit I'd like to suggest a code block.

One, can someone please explain why we're using strstr instead of strncmp?

Ok, so here are my suggestions: 

NS_NAMED_LITERAL_CSTRING(psNativeFont, "print.postscript.nativefont.");
  if (strncmp(aName, psNativeFont.get(), psNativeFont.Length())
    lang.Cut(0, psNativeFont.Length());
  }
or.
NS_NAMED_LITERAL_CSTRING(psNativeFont, "print.postscript.nativefont.");
  if (psNativeFont.Equals(Substring(aName, aName+psNativeFont.Length())))
    lang.Cut(0, psNativeFont.Length());
  }

Comment 7

[rods (gone)]

Attachment: new patch

With Marc's suggestion

Comment 8

[rods (gone)]

Attachment: final patch

70757 is wrong, this is the good patch

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 70759 [details] [diff] [review]
final patch

It doesn't look like this second patch has review yet, so I'll add
some review-ish comments.

>+// NOTE: If you change either of these two strings, 
>+//       you MUST change the strings in the struct just below this!

This could be automated, e.g., by making a macro for the strings

#define NATIVE_FONT_PREFIX "print.postscript.nativefont."

static const char kNativeFontPrefix[] = NATIVE_FONT_PREFIX; /* needed at all?
*/

{NATIVE_FONT_PREFIX "ja", "Ryumin-Light-EUC-H"},

>+static const char* kNativeFontPrefix  = "print.postscript.nativefont.";

static const char kNativeFontPrefix[] = "...";

would be preferred since it isn't an lvalue (less storage space,
more potential for compiler optimization).

>+  if (strstr(aName, kNativeFontPrefix)) {
>+    lang.Cut(0, strlen(kNativeFontPrefix));
>+
>+  } else if (strstr(aName, kUnicodeFontPrefix)) {
>+    lang.Cut(0, strlen(kUnicodeFontPrefix));

timeless's suggestion for strncmp here seems sound.  It also seems
odd to use strlen (a run-time cost) when you can use (*provided* that
you use the array syntax I mention in my previous comment or use the
literal or a macro that expands to the literal) sizeof(array)-1,
perhaps through a macro (|#define STRLEN(s) (sizeof(s)-1)|) or through
NS_LITERAL_CSTRING.
Maybe you really want these strings to be macros rather than constants?

>   nsStringKey key(lang);

It seems like it would be a bit nicer if these keys were
nsCStringKey (you could convert at the other site, after using GetUnicode.
Then you could use |operator+| with an nsAutoCString (or even
nsDependentCString) when calling the nsAutoCString constructors below.
But that's a bigger change than needed right now.

Then again, the current patch will fix the problem, so if you really
want to just get it in, r=dbaron, although I really don't like the
|strlen| calls.

Comment 10

[rods (gone)]

Attachment: patch

using dbaron's and timeless' suggestions.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 70809 [details] [diff] [review]
patch

>+    lang.Cut(0, psNativeFont.Length()-1);

Shouldn't this just be Length() rather than Length()-1?

Other than that, r=dbaron.

Comment 12

[rods (gone)]

Actually not, I took the snipet of code and compiled and ran it on Windows and I
discovered that I needed the -1 to get "lang" to equal to "ja"

Comment 13

[Marc Attinasi]

Looks like over-engineering to me.  All I wanted was an assertion to prevent
this problem from happening again...

Comment 14

[rods (gone)]

I have the patch ready to go, thanks Marc.

Comment 15

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 16

[timeless]

i'm pretty sure you could put the named_literal_string outside of function 
scope to use it throughout the file.  my snippet wasn't intended to show 
absolute correct usage, merely how the pieces could be used.

Comment 17

[rods (gone)]

fixed

Comment 18

[sujay]

Katakai, please verify this bug and mark verified-fixed...thanks!

Comment 19

[Masaki Katakai]

Verified. Thanks!