Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 19 years ago

Sorry for the no good summary.

This is pango's issue. When pango chooses the font for the lang neutral characters(e.g., numbers), it decides the font from around characters. This is a good idea for the *segment*. But it's not valid for our rendering. In the test case, the number '4's are rendered different fonts on its position. This is not beautiful. We should always render same font irrespective of the place of the characters.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 19 years ago

screenshot:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3188

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 19 years ago

This patch separates the text to some segments. They are non-neutral(language dependent) characters segment and neutral(language independent) characters.

Therefore, the neutral characters are rendered by default font.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 19 years ago

And this bug has another problem. This bug makes overlapping problem.(But I don't understand the logic.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 19 years ago

But the patch fixes the overlapping problem too.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 19 years ago

I found another problem that being fixed by this patch.
See this testcase:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3169
This is rendered as following screenshot:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3170

When the justifying, the characters are mesured per character. So, the neutral characters('(' and ')') don't depend on the around characters at measuring. But at rendering, the neutral characters depend on the around characters. But this patch fixes the difference.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 8 • 19 years ago

Stuart:

Would you review this?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 18 years ago

Comment on attachment 223606 [details] [diff] [review]
Patch rv1.0

I cancel this review.

If a string has two or more languages, each languages are rendered by each system default fonts. I think that we may be able to fix both problem in a patch. I'll try it.
# And maybe also the font.name-list is.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 10 • 18 years ago

I think that if my plan will be success, Bug 334299 and Bug 334565 will be fixed by it. If I confirm it, I'll block these bugs by this bug.

	Jean-Marc Desperrier
	Comment 11 • 18 years ago

Masa, the problem in c#1, I think we need a larger perspective on whether what you try to implement is actually desirable. I believe the behavior was implemented knowingly  in Pango, and their point of view was the advantage outweighs the inconvenients. It would be good to have a feedback from the initial Pango implementers. It think that is a worst case, and in many real life situations it will look nicer to use lang neutral characters that come from the same font as the surrounding characters.

Do you have real life examples of this going bad ? (I understand you transferred this bug from bugzilla.mozilla.gr.jp ) Doesn't the problem only happen on UTF-8 pages ? (that really should use the lang attribute to avoid random language change)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 18 years ago

I think that if CSS style sheet has system font specify for font-family, we may need to discuss for the best approach. But the testcase is not having system font specify, so, it should be rendered by CSS rules.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 13 • 18 years ago

(In reply to comment #11)
> Do you have real life examples of this going bad ? (I understand you
> transferred this bug from bugzilla.mozilla.gr.jp ) Doesn't the problem only
> happen on UTF-8 pages ? (that really should use the lang attribute to avoid
> random language change)

No, the problem is occurred on many pages(including non-UTF-8 pages), so that doesn't occur only on specially testcases. The current pango code doesn't honor the lang attribute.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 18 years ago

This patch isn't not final, it is draft.
This shows my idea for new structure of gfxPangoTextRun.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 15 • 18 years ago

the letter-spacing problem is fixed.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 18 years ago

This patch fixes very many problems.

1. This chooses the good font in every segments.
2. Even if Japanese and Chinese characters are mixed in a text, this can render the one.
3. This fixes the bug 310746.
4. This fixes the bug 334299.

remains works:
1. Measure the performance.
2. Optimize and cleanup the code.
3. fix RTL problem(in RTL page, this hangs up.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 17 • 18 years ago

I tested the performance of this patch. That will give the damage for the tp value in tinderbox. In my environment, the page loading scores are:

current:
Avg. Median : 846 msec
Average     : 856 msec
Minimum     : 405 msec
Maximum     : 2864 msec

patched:
Avg. Median : 906 msec
Average     : 962 msec
Minimum     : 426 msec
Maximum     : 2716 msec

So, we need fast function for simple text like as gfxWindowsTextRun.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 18 • 18 years ago

RTL problem is fixed.
The final patch coming soon.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 19 • 18 years ago

This patch may be pre-final.
This patch fixes:
* adding "x-central-euro" for pango-lang to moz-lang table.
* use "x-unicode" if aLangGroup of ForEachFont is empty.
* fix bustage on Win/Mac.

I'll optimize in next patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 20 • 18 years ago

The performances:
current build
Avg. Median : 851 msec
Average     : 866 msec
Minimum     : 442 msec
Maximum     : 3097 msec

patched build
Avg. Median : 821 msec
Average     : 835 msec
Minimum     : 415 msec
Maximum     : 2130 msec

The patched build is little faster. But it may not be correct. However, there is no regression in performance :-)

The detail of the patch will be written in next comment.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 21 • 18 years ago

This patch has very many features:

1. bug 310746, this fixes the non-ASCII font name problem.
2. bug 334299, this uses font prefs for font switching if we cannot find the glyphs in default font.
3. This guarantees the same character is always rendered by same font. Therefore, this fixes comment 0, comment 1 and comment 7 problems.
4. This can render the CJK complex text(e.g., the Japanese text has Chinese only character). Because if we cannot find glyph in current font list, it adds the all CJK pref fonts to the list.
5. Expanding the moz-lang to pango-lang table.
6. Adding the pango-lang to moz-lang table and function.
7. Now, we are released from layout module of pango!

This is not using layout module of pango. We can get glyphs by |pango_shape| that needs |PangoAnalysis| struct. We have two ways for initializing |PangoAnalysis|.
1. using |pango_itemize|:
This way is used in |MeasureOrDrawItemizing|. This is not good for performance. But we can get the hints for missing glyphs by the language information of the text.
2. ourselves:
This way is used in |MeasureOrDrawFast|. In this case, this patch uses font language for the initializing. In most cases, we can render by this function. But if there are missing glyphs, we need to fall back to |MeasureOrDrawItemizing|.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 22 • 18 years ago

The previous patch has a nit.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 23 • 18 years ago

fix perf issue by non-drawable characters.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 24 • 18 years ago

Sorry for the spam. This fixes invalid length if the missing glyphs found in non-left-most of the segment.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 25 • 18 years ago

Updating for latest trunk. I'll attach rv1.4 for minor change.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 26 • 18 years ago

This fixes bug 343262 too, see the diff.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 27 • 18 years ago

Comment on attachment 236086 [details] [diff] [review]
Itemizing patch rv1.4

I found a hang up bug. Please wait.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 28 • 18 years ago

Every time I start to review you post a new patch up :)  I'll look at the next patch as soon as you post it; so far everything looked ok though I had a few questions.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 29 • 18 years ago

Sorry for the delay.

In Arabic text, sometimes, pango hung-upped if I sent the font that doesn't have Arabic glyphs to the pango shaper. This patch fixes this problem. (If the text has Arabic character(s), the FontSelector doesn't send non-Arabic fonts to the pango shaper.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 30 • 18 years ago

(In reply to comment #29)
> In Arabic text, sometimes, pango hung-upped if I sent the font that doesn't
> have Arabic glyphs to the pango shaper. This patch fixes this problem. (If the
> text has Arabic character(s), the FontSelector doesn't send non-Arabic fonts to
> the pango shaper.)

Ah, sorry. Of course, this way is not a best way. We should check whether the font has Arabic glyphs before calling the pango shaper. But I don't know how to do. So, I cannot use best way, now. But I'll try to fix it for better implementation.

	Stuart Parmenter
	Comment 31 • 18 years ago

(In reply to comment #29)
> Created an attachment (id=236116) [edit]
> Itemizing patch rv1.5
> 
> Sorry for the delay.
> 
> In Arabic text, sometimes, pango hung-upped if I sent the font that doesn't
> have Arabic glyphs to the pango shaper. This patch fixes this problem. (If the
> text has Arabic character(s), the FontSelector doesn't send non-Arabic fonts to
> the pango shaper.)
> 

I believe we want to send arabic text through the pango shaper.  We should figure out why pango is hanging -- it certainly doesn't hang for other applications using arabic.  Are we passing it in funny data?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 32 • 18 years ago

(In reply to comment #31)
> (In reply to comment #29)
> > Created an attachment (id=236116) [edit]
> > Itemizing patch rv1.5
> > 
> > Sorry for the delay.
> > 
> > In Arabic text, sometimes, pango hung-upped if I sent the font that doesn't
> > have Arabic glyphs to the pango shaper. This patch fixes this problem. (If the
> > text has Arabic character(s), the FontSelector doesn't send non-Arabic fonts to
> > the pango shaper.)
> > 
> 
> I believe we want to send arabic text through the pango shaper.  We should
> figure out why pango is hanging -- it certainly doesn't hang for other
> applications using arabic.  Are we passing it in funny data?
> 
I don't know what happens in pango. If |mDontTryDifferentLangFont| is always PR_FALSE, I can get this error message:

(Gecko 4159): Pango-WARNING **: shape engine failure, expect ugly output. the offending font is 'Sazanami Gothic Bold 11.19921875'

If other applications uses pango-layout-*, pango always sends the Arabic font to Arabic text segment at shaping even if any font is specified.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 33 • 18 years ago

And this happens when I selected the Arabic text in many cases.(Of course, it happen at page loading time.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 34 • 18 years ago

This fixes (but it's adhoc...) the problem of bug 334064 comment 20.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 35 • 18 years ago

Comment on attachment 236197 [details] [diff] [review]
Itemizing patch rv1.6

Wow, sorry. This patch cannot fix the problem completely. We should not use this way.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 36 • 18 years ago

Comment on attachment 236116 [details] [diff] [review]
Itemizing patch rv1.5

Sorry. I confirmed that the hang in Gecko, so not on pango. I'll find the cause and recreate the patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 37 • 18 years ago

This fixes Arabic text problem.
The cause of the hung-up is the context direction setting.
Even if the gfxTextRun is LTR, the text may have RTL text. In that case, this patch changes the context setting.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 38 • 18 years ago

removing the non-used member. sorry for the spam.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 39 • 18 years ago

Oops. sorry. the previous patch has a test code.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 40 • 18 years ago

the regression of bug 334064 comment 20 is fixed.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 41 • 18 years ago

fix a memory leak.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 42 • 18 years ago

Improving the font cache mechanism.
Now, we can get actual font name that is resolved the alias.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 43 • 18 years ago

Comment on attachment 236726 [details] [diff] [review]
Itemizing patch rv1.12

I have an idea for better code, please wait, sorry.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 44 • 18 years ago

The previous patch sometimes failed the font switching.
But this patch fixes the problem, maybe, the cause is a wrong font index management.
And this patch removes some warnings.

Maybe, this is a final if there are no bugs.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 45 • 18 years ago

Optimizing the font cache.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 46 • 18 years ago

Sorry, the previous patch has a nit.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 47 • 18 years ago

I think that we need more work for alias font name. Because the current patch still has some problem on some environments. However, I think that the code may be large. So, I don't like to merge the codes in the patch. And I'll continue to work on gfxPangoFonts for some issues.

Vlad, would you review the patch? Sorry for the delay.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 48 • 18 years ago

Ok.. Can we let 1.14.1 sit there a bit so that I can review it?  :)  Any followup stuff can come as separate patches, otherwise I'll never have a stable version of this to look over (it's getting pretty big as it is).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 49 • 18 years ago

(In reply to comment #48)
> Can we let 1.14.1 sit there a bit so that I can review it?

Vlad, sorry, I cannot understand this sentence, what you mean?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 50 • 18 years ago

(In reply to comment #49)
> (In reply to comment #48)
> > Can we let 1.14.1 sit there a bit so that I can review it?
> 
> Vlad, sorry, I cannot understand this sentence, what you mean?

I mean that it'll take me at least a day to review, and if the patch keeps changing then I have a hard time doing that (and Stuart will need to review as well, so both of us together will need a few days).  If the patch is good enough as-is, give me a day or two to review, and then assuming everything is good, let's get this checked in and do other work as additional patches.  There's a bunch of (good!) changes here already, so it would be better to review this as it is.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 51 • 18 years ago

I see.

regards

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 52 • 18 years ago

Comment on attachment 236788 [details] [diff] [review]
Itemizing patch rv1.14.1

This looks fine; sorry the review took so long.  Stuart may have some comments on the core gfxFont changes for font-family splitting, but I'll let him chime in here.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 53 • 18 years ago

Comment on attachment 236788 [details] [diff] [review]
Itemizing patch rv1.14.1

Thank you, vlad for your review! I'm asking to stuart. Do you have some comments for this patch?

	Stuart Parmenter
	Comment 54 • 18 years ago

Comment on attachment 236788 [details] [diff] [review]
Itemizing patch rv1.14.1

this patch really needs a lot more comments so that you can follow what is going on.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 55 • 18 years ago

(In reply to comment #54)
> (From update of attachment 236788 [details] [diff] [review] [edit])
> this patch really needs a lot more comments so that you can follow what is
> going on.

Where needs the comments? If you point, I'll attach the new patch.

	Stuart Parmenter
	Comment 56 • 18 years ago

Comment on attachment 236788 [details] [diff] [review]
Itemizing patch rv1.14.1

Please rewrite GetMozLanguage without gotos.  There isn't really any need for them in this function.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 58 • 18 years ago

checked-in.

Thank you.

	David Baron :dbaron:
	Comment 59 • 18 years ago

Comment on attachment 240086 [details] [diff] [review]
Itemizing patch rv1.15

>+/* static */
>+void
>+GetMozLanguage(const PangoLanguage *aLang, nsACString &aMozLang)
>+{
>+    aMozLang.Truncate();
>+    if (!aLang)
>+        return;
>+
>+    nsCAutoString lang(pango_language_to_string(aLang));
>+    if (lang.Equals("xx"))
>+        return;
>+
>+    do {
>+        for (PRUint32 i = 0; i < NUM_PANGO_ALL_LANG_GROUPS; ++i) {
>+            if (lang.Equals(PangoAllLangGroup[i].PangoLang)) {
>+                if (PangoAllLangGroup[i].mozLangGroup)
>+                    aMozLang.Assign(PangoAllLangGroup[i].mozLangGroup);
>+                return;
>+            }
>+        }
>+
>+        PRInt32 hyphen = lang.FindChar('-');
>+        if (hyphen != kNotFound) {
>+            lang.Cut(hyphen, lang.Length());
>+            continue;
>+        }
>+    } while (0);
>+}

This revised version of GetMozLanguage looks like it won't actually do the repeat if it fails in the first pass (unlike the version before comment 56), since |continue| still rechecks the loop condition before re-entering the loop (so in a while(0) there's no difference between |continue| and |break|).  Shouldn't you replace this:

        if (hyphen != kNotFound) {
            lang.Cut(hyphen, lang.Length());
            continue;
        }

with something like this:

        if (hyphen == kNotFound)
            break;
        lang.Cut(hyphen, lang.Length());

and then change while(0) to while(1) ?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 60 • 18 years ago

Wow, you're right.

I'll file a new bug.