Bug 51450

Summary: Glyphs in vertical text tests are rotated 90 degrees clockwise on Chromium Windows
Product: WebKit Reporter: Chun-Lung Huang <alvincl.huang>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, eric.carlson, eric, feature-media-reviews, hbono, itshustletime, jamesr, jshin, koansin.tan, kojii, rniwa, senorblanco, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on: 54846, 81332, 81389, 83512, 94822    
Bug Blocks: 94410, 94771    
Attachments:
Description Flags
a proposed patch for 51450
none
A proposed patch for 51450 with CHANGELOG.
none
A proposed patch for 51450 with CHANGELOG.
none
An updated patch for 51450
tkent: review-
An updated patch for 51450
hyatt: review-
cannot reproduce ASSERT_NOT_REACHED()
none
add more comments & make some changes according to others' comments
tkent: review-
change accroding to reviewer's comments
none
New approach without using @-font API and supports text-orientation (in progress)
none
Updated to fix problems in some TTF OpenType fonts
none
No longer depends on 48459, but needs ChangeLog and tests
none
Fixed WebCore.gypi, and features.gyp as per Kent's comments
none
Rebaseline
none
Rebaseline with --binary flag
none
Added ChangeLog and more tests
none
Added ChangeLog and more tests
none
Enable OPENTYPE_VERTICAL in features.gypi, enabled ~10 tests, and rebaselines
none
Resolved merge conflicts and updated test results
none
Wrong, --binary was missing
none
Resolved yet another merge conflict
none
Patch none

Description Chun-Lung Huang 2010-12-21 22:50:12 PST
Encouraged by my colleague to make Chromium Windows render vertical text correctly.

As mentioned in https://bugs.webkit.org/show_bug.cgi?id=48459
Chromium Windows does not render vertical text correctly.  Glyphs are rotated 90 degrees clockwise.
Comment 1 Chun-Lung Huang 2010-12-22 01:39:15 PST
Created attachment 77196 [details]
a proposed patch for 51450
Comment 2 Chun-Lung Huang 2010-12-23 01:12:39 PST
Created attachment 77313 [details]
A proposed patch for 51450 with CHANGELOG.
Comment 3 WebKit Review Bot 2010-12-23 01:14:02 PST
Attachment 77313 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp', u'WebCore/platform/graphics/chromium/FontChromiumWin.cpp', u'WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp', u'WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h']" exit_code: 1
WebCore/platform/graphics/chromium/FontChromiumWin.cpp:377:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Chun-Lung Huang 2010-12-23 01:37:31 PST
Created attachment 77314 [details]
A proposed patch for 51450 with CHANGELOG.

remove the accident [tab]
Comment 5 Chun-Lung Huang 2010-12-28 19:59:24 PST
Created attachment 77598 [details]
An updated patch for 51450

Handle the custom font as well.

The following test cases look ok to me.

broken-ideograph-small-caps.html
broken-ideographic-font.html
japanese-lr-selection.html
japanese-lr-text.html
japanese-rl-selection.html
japanese-rl-text.html
japanese-ruby-horizontal-bt.html
japanese-ruby-vertical-lr.html
japanese-ruby-vertical-rl.html
Comment 6 Kent Tamura 2010-12-29 06:59:41 PST
Comment on attachment 77598 [details]
An updated patch for 51450

View in context: https://bugs.webkit.org/attachment.cgi?id=77598&action=review

Thank you for making the patch.
Would you attach some layout test result images please?

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:573
> +FontPlatformData* FontCache::createFontPlatformData(const FontDescription& fontDescription, const AtomicString& oldFamily)

I don't think "oldFamily" is an appropriate name. 
How about keeping it as "family", and ...

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:583
> +    AtomicString family;

.. renaming this to "updatedFamilyName" and replacing the following "family" with "updatedFamilyName"?

> WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:586
> +    // Create FontPlatformData should consider the orientation.
> +    // For vertical: add "@" in front of the oldFamily name. (Windows)

This comment doesn't add any additional information.  Please remove it.

> WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:73
> -    FontOrientation orientation() const { return Horizontal; } // FIXME: Implement.
> +    FontOrientation orientation() const { return m_orientation; } // FIXME: Implement.

Please remove the FIXME comment.

> WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:83
> +    // Create FontCustomPlatformData should consider the orientation.
> +    // For vertical: add "@" in front of the m_name. (Windows)

This comment doesn't add any additional information.  Please remove it.
Comment 7 Chun-Lung Huang 2010-12-29 19:26:03 PST
(In reply to comment #6)
> (From update of attachment 77598 [details])
> Thank you for making the patch.
> Would you attach some layout test result images please?

Some layout test images
http://www.flickr.com/photos/burorly/sets/72157625703510098/
Comment 8 Chun-Lung Huang 2010-12-29 21:00:39 PST
Created attachment 77658 [details]
An updated patch for 51450

Changes made according to Kent Tamura's comments.
Comment 9 Kent Tamura 2011-01-05 17:08:31 PST
I tried the patch locally, and found a renderer crashed at ASSERT_NOT_REACHED() in FontCache::getLastResortFallbackFound() FontCacheChromiumWin.cpp when I tried to load fast/blockflow/Kusa-Makura-background-canvas.html.
Comment 10 Chun-Lung Huang 2011-01-05 22:16:46 PST
Created attachment 78097 [details]
cannot reproduce ASSERT_NOT_REACHED()

(In reply to comment #9)
> I tried the patch locally, and found a renderer crashed at ASSERT_NOT_REACHED() in FontCache::getLastResortFallbackFound() FontCacheChromiumWin.cpp when I tried to load fast/blockflow/Kusa-Makura-background-canvas.html.

Hmmmm... I cannot reproduce this in my environment.
(Windows 7 traditional chinese version, Chromium built in debug mode)
I've tried to change the font-family in the html.
Still cannot reproduce it.

I am tring to reproduce it in a pure English environment.
Any suggestions? Please let me know.
Comment 11 Kent Tamura 2011-01-05 22:23:10 PST
(In reply to comment #10)
> I am tring to reproduce it in a pure English environment.
> Any suggestions? Please let me know.

yeah, I tried on Windows 7 English edition.
Comment 12 Dave Hyatt 2011-01-06 15:32:34 PST
Comment on attachment 77658 [details]
An updated patch for 51450

See my comments in https://bugs.webkit.org/show_bug.cgi?id=48459.  The same applies here.
Comment 13 Chun-Lung Huang 2011-01-19 22:47:12 PST
(In reply to comment #11)
> (In reply to comment #10)
> > I am tring to reproduce it in a pure English environment.
> > Any suggestions? Please let me know.
> yeah, I tried on Windows 7 English edition.

Finally got some time to reproduce it in Windows 7 English Edition.

The problem is that some fonts, in this case 'Times New Romain', do not have the vertical form.  If I ask for '@Times New Romain', Windows GDI's font mapper attempts to match the it with an existing physical font. If it fails to find an exact match, it provides an alternative whose characteristics match as many of the requested characteristics as possible. (from MSDN)

It seems that CreateFontIndirect() took locale into consideration.
In my environment, traditional Chinese edition, it will return '@新細明體'.
In yours, English edition, it will return '@Malgun Gothic'.
/* @FontCacheChromiumWin.cpp:253 */

I think both of them can be used, however, it cannot pass the name checking statement: "if (!equalIgnoringCase(updatedFamilyName, winName))"
/* FontCache::createFontPlatformData()@FontCacheChromiumWin.cpp:591 */
/* @Times New Romain <> @新細明體(aka "PMingLiu"), in my case*/
/* @Times New Romain <> @Malgun Gothic, in yours */

So both of them will get to getLastResortFallbackFont().  The difference is here.
In my case, it will find fallback font "微軟正黑體"(aka "Microsoft JhengHei") and returned simpleFont @FontCacheChromiumWin.cpp:540
In your case, it cannot find fallback font and crashed at ASSERT_NOT_REACHED() @FontCacheChromiumWin.cpp:551

*********************************************************

=== Possible Solution ===
Becuase Windows GDI's font mapper has already found a similar font that you have for you.  How about just use it for vertical text!  If you wanna locally try this patch without crash. Please replace FontCacheChromiumWin.cpp:591 to

if (!winName.startsWith("@") && !equalIgnoringCase(updatedFamilyName, winName)) {

I think this would work for you.  Please try it. :)

=== Another Solution?? ===
Yes. We can handle fallback in FontCache.cpp and CSSFontSelector.cpp but it is not platform dependent.  I don't think I want to touch this now.

Any comments?

MSDN reference: http://msdn.microsoft.com/en-us/library/dd183500%28v=VS.85%29.aspx
Comment 14 Kent Tamura 2011-01-19 22:54:48 PST
Bono-san, Jungshik, do you have any comments on this?
Comment 15 Hironori Bono 2011-01-20 00:39:42 PST
(In reply to comment #14)
> Bono-san, Jungshik, do you have any comments on this?

For his solution to prevent the crash, I do not have any ideas about it since I do not have any backgrounds about why this function compares the returned font. Nevertheless, to read the comments in the function, we seem to compare the returned family name also for filtering out your case: preventing returning '@Malgun Gothic' when we need a '@Times New Roman' font. So, it is better to ask Jungshik or Darin, who probably know the backgrounds about this code. (I'm adding more fall-back fonts to FontCache::getLastResortFallbackFont() to prevent crashes or assertions at Bug 52422, though.)

By the way, I'm a little wondering why this change ALWAYS uses '@'-fonts for rendering vertical text. (I would note that I may be wrong since I do not have any context about '-webkit-writing-mode'.) It is pretty reasonable to use '@'-fonts so we can use alternative fonts that have vertical glyphs when we render JAPANESE text with the '-webkit-writing-mode: vertical-rl; font-family: Times New Roman;' styles. On the other hand, it may be better to just rotate the 'Times New Roman' font than to use alternative fonts when we render ASCII text with the same styles? (Even though I do not have clear ideas now, it may be better to add '@' only when we need CJK fonts?)
Also, it may be a good idea to check if the 'family' is started with '@' in Line 583 of FontCacheChromiumWin.cpp to prevent adding '@' twice when users explicitly use '@'-fonts in the 'font-family' property?

Regards,

Hironori Bono
Comment 16 Chun-Lung Huang 2011-01-20 02:52:44 PST
(In reply to comment #15)
> (In reply to comment #14)
> > Bono-san, Jungshik, do you have any comments on this?

Bono-san,
Thank you for your comments. :)

1. I've noticed that the comparison code is also for filtering out my cases.  I guess the code is used to get more control when font fallback occurs.  Bypassing the comparison code is to let people in other locale could try what the '@' can do without crash. (that's why I didn't make a new patch.)

2. Yes. I think it might be better to prevent crashes at FontCache::getLastResortFallbackFont() with more fallback fonts.
Fallback "Times New Roman" in vertical to other fonts in order to have the whole artical with more 'consistent' visual effect; or just keep using "Times New Roman" while other characters are in other fonts.  It can be discussed.

>(Even though I do not have clear ideas now, it may be better to add '@' only when we need CJK fonts?)

3. hmmm... I've considered this.  But personally, I prefer replacing it when rendering a multilingual article. (as No. 2.)
In my environment, Win 7 traditional Chinese Edition, I don't need to escape the comparison code and it will finally fallback to "微軟正黑體" (aka "Microsoft JhengHei") and returned simpleFont @FontCacheChromiumWin.cpp:540. The visual effect is great.

> Also, it may be a good idea to check if the 'family' is started with '@' in Line 583 of FontCacheChromiumWin.cpp to prevent adding '@' twice when users explicitly use '@'-fonts in the 'font-family' property?

4. You are right. I have to do this.  Even though it will not crash if we ask for "@@xxxxx" (because the Windows API will return "@xxxxx").

Thank you.

Alvin
Comment 17 Hironori Bono 2011-01-25 01:37:41 PST
Greetings,

Thank you for your change.

(In reply to comment #16)
> 3. hmmm... I've considered this.  But personally, I prefer replacing it when rendering a multilingual article. (as No. 2.)
> In my environment, Win 7 traditional Chinese Edition, I don't need to escape the comparison code and it will finally fallback to "微軟正黑體" (aka "Microsoft JhengHei") and returned simpleFont @FontCacheChromiumWin.cpp:540. The visual effect is great.

My previous comment just described the behavior of Mac Safari (top-of-trunk) and Mac Chromium (top-of-trunk). In my personal opinion, I would like to implement consistent behaviors with these browsers so web-site developers do not have to add browser-specific code in their web page.

Nevertheless, I do not have any intention to block this change because I also realize everyone has its own taste. (I would note I'm not a WebKit reviewer.)

Regards,

Hironori Bono
Comment 18 Jungshik Shin 2011-02-04 13:40:27 PST
Sorry I didn't get to this bug sooner. 

(In reply to comment #15)
> (In reply to comment #14)
> > Bono-san, Jungshik, do you have any comments on this?
> 
> For his solution to prevent the crash, I do not have any ideas about it since I do not have any backgrounds about why this function compares the returned font. Nevertheless, to read the comments in the function, we seem to compare the returned family name also for filtering out your case: preventing returning '@Malgun Gothic' when we need a '@Times New Roman' font. So, it is better to ask Jungshik or Darin, who probably know the backgrounds about this code. (I'm adding more fall-back fonts to FontCache::getLastResortFallbackFont() to prevent crashes or assertions at Bug 52422, though.)

Yes, we want to respect CSS font fallback list (specified by web page authors) as much as possible. That is, we don't want Windows GDI font mapper to give us something we didn't ask for by some font mapping magic behind the scene. 

BTW, should the concern by hyatt (comment 12 and bug 48459) be addressed before going further?
Comment 19 Chun-Lung Huang 2011-02-17 04:08:53 PST
Created attachment 82783 [details]
add more comments & make some changes according to others' comments

(In reply to comment #9)
> I tried the patch locally, and found a renderer crashed at ASSERT_NOT_REACHED() in FontCache::getLastResortFallbackFound() FontCacheChromiumWin.cpp when I tried to load fast/blockflow/Kusa-Makura-background-canvas.html.
(In reply to comment #11)
> yeah, I tried on Windows 7 English edition.

Hi all,

I saw Bono-san's patch for bug 52422.  It prevents the crash mentioned in comments #9~#11 when my patch was applied.
So I just made a new patch according to others' comments.

Not perfect but it could get the basics working.  Please do consider that an "@" can nearly do all the jobs.

Alvin
Comment 20 Kent Tamura 2011-02-17 04:23:59 PST
Comment on attachment 82783 [details]
add more comments & make some changes according to others' comments

View in context: https://bugs.webkit.org/attachment.cgi?id=82783&action=review

Some comments for small issues.

> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:620
> +        updatedFamilyName = String(L"@") + String(family);

I think you don't need to do String(L"@").  Just '"@" + String(family)' should work.

> Source/WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:82
> +    String newMName;

This should be "newName" because this variable is not a member of the class, so m makes no sense.

> Source/WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:84
> +        newMName = String(L"@") + m_name.charactersWithNullTermination();

.charactersWithNullTermination() is not needed here.  This should be just 'newName = "@" + m_name;'

> Source/WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:86
> +        newMName = m_name.charactersWithNullTermination();

.charactersWithNullTermination() is not needed.
Comment 21 Chun-Lung Huang 2011-02-17 18:59:21 PST
Created attachment 82893 [details]
change accroding to reviewer's comments

Thank you
Comment 22 Kent Tamura 2011-02-17 21:34:19 PST
Comment on attachment 82893 [details]
change accroding to reviewer's comments

ok.  I'll test the patch locally and commit it later.
Comment 23 Kent Tamura 2011-02-20 19:28:32 PST
Comment on attachment 82893 [details]
change accroding to reviewer's comments

Clearing flags on attachment: 82893

Committed r79169: <http://trac.webkit.org/changeset/79169>
Comment 24 Kent Tamura 2011-02-20 19:28:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Kent Tamura 2011-02-20 20:49:07 PST
(In reply to comment #23)
> Committed r79169: <http://trac.webkit.org/changeset/79169>

I rolled it out.

The results of tests with CJK characters were good.
But the results of tests without CJK characters were not good.  It looked the font was falled back to a CJK font.  It's not acceptable for one who wants to use vertical writing with Latin letters.

I hope a patch for this bug won't break non-CJK character vertical tests such as fast/blockflow/border-radius-clipping-vertical-lr.html
Comment 26 WebKit Review Bot 2011-02-21 22:28:43 PST
http://trac.webkit.org/changeset/79169 might have broken GTK Linux 32-bit Debug
Comment 27 Ryosuke Niwa 2011-06-16 17:40:36 PDT
The approach taken by this patch isn't correct.  See http://stackoverflow.com/questions/2250437/how-to-draw-vertical-text-in-windows-gui.
Comment 28 Hironori Bono 2011-06-16 22:49:47 PDT
Niwa-san,

(In reply to comment #27)
> The approach taken by this patch isn't correct.  See http://stackoverflow.com/questions/2250437/how-to-draw-vertical-text-in-windows-gui.

lfEscapement just rotates each glyph, i.e. it does not replace the glyph with the one optimized for vertical-writing. Many Japanese fonts have special glyphs optimized for vertical writing and an @-font forces Windows to use these optimized glyphs if possible. I assume he would like to use them instead of just rotating each glyph by 90 degrees. (To read this bug straightforwardly, lfEscapment looks sufficient, though.)

Regards,

Hironori Bono
Comment 29 Ryosuke Niwa 2011-06-16 23:09:22 PDT
(In reply to comment #28)
> lfEscapement just rotates each glyph, i.e. it does not replace the glyph with the one optimized for vertical-writing. Many Japanese fonts have special glyphs optimized for vertical writing and an @-font forces Windows to use these optimized glyphs if possible. I assume he would like to use them instead of just rotating each glyph by 90 degrees. (To read this bug straightforwardly, lfEscapment looks sufficient, though.)

I talked with Chun-Lung on IRC today, and we concluded that we'd need to use @-font for CJK and rotated glyph for non-CJK characters due to the font fallback issue Kent-san pointed out.

I also suggested that he should try tweaking lfOutPrecision to see if that solves the fallback issue.
Comment 30 Koji Ishii 2012-04-05 02:38:09 PDT
Created attachment 135787 [details]
New approach without using @-font API and supports text-orientation (in progress)

This is still work in progress, but I'm making a patch that:
* Doesn't use Windows @-font API
* Gives consistent glyph orientation with Mac platform
* Supports text-orientation
This patch uses the same classes as bug 48459, and therefore following patches are required before this one.
Bug 81326, bug 81389, bug 81332, bug 48459
Note that bug 48459 is for CGWin, but it has some necessary changes in shared code for this patch to work.
Comment 31 Koji Ishii 2012-04-09 19:57:07 PDT
It turned out that Chromium Windows lacks some font-related APIs/data and because of that, this patch works only with Adobe CFF fonts such as Kozuka.

I added bug 83512 to fix unitsPerEm, and also implementing SimpleFontData::platformBoundsForGlyph() which currently returns an empty rectangle.

I'll update the patch once I've got more testing and fixes.
Comment 32 Koji Ishii 2012-04-25 12:10:22 PDT
Created attachment 138853 [details]
Updated to fix problems in some TTF OpenType fonts

Changes from the last patch:
* More efficient cache of OpenTypeVerticalData
* Fixed issues in some TTF OpenType fonts
* Merged fix in bug 83512
Comment 33 Tony Chang 2012-04-27 10:54:12 PDT
(In reply to comment #32)
> Created an attachment (id=138853) [details]
> Updated to fix problems in some TTF OpenType fonts
> 
> Changes from the last patch:
> * More efficient cache of OpenTypeVerticalData
> * Fixed issues in some TTF OpenType fonts
> * Merged fix in bug 83512

Do you want someone to provide feedback on this patch?  You might get a faster response if you set r? after adding the ChangeLog and tests.
Comment 34 Koji Ishii 2012-04-27 21:42:40 PDT
(In reply to comment #33)
> (In reply to comment #32)
> > Created an attachment (id=138853) [details] [details]
> > Updated to fix problems in some TTF OpenType fonts
> > 
> > Changes from the last patch:
> > * More efficient cache of OpenTypeVerticalData
> > * Fixed issues in some TTF OpenType fonts
> > * Merged fix in bug 83512
> 
> Do you want someone to provide feedback on this patch?  You might get a faster response if you set r? after adding the ChangeLog and tests.

Bugs that are blocking this (bug 81389, bug 81332, and bug 48459) are still waiting for review, and this patch may need changes as they're reviewed, in that case, I guess I have to ask reviewers' time again, correct? I wasn't sure if I could set r? under such situation so I had been waiting for these bugs reviewed and committed. Do you mind to tell me if that's ok?
Comment 35 Tony Chang 2012-05-02 14:50:17 PDT
Sorry, I didn't realize those other bugs where blocking.
Comment 36 Koji Ishii 2012-08-11 05:50:10 PDT
Created attachment 157870 [details]
No longer depends on 48459, but needs ChangeLog and tests
Comment 37 WebKit Review Bot 2012-08-11 05:52:24 PDT
Attachment 157870 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.gyp/WebCore.gyp', u..." exit_code: 1
Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Kent Tamura 2012-08-11 06:13:11 PDT
Comment on attachment 157870 [details]
No longer depends on 48459, but needs ChangeLog and tests

View in context: https://bugs.webkit.org/attachment.cgi?id=157870&action=review

> Source/WebCore/WebCore.gyp/WebCore.gyp:1829
> +        ['OS=="win"', {
> +          'sources/': [
> +            ['include', 'platform/graphics/opentype/OpenTypeTypes\\.h$'],
> +            ['include', 'platform/graphics/opentype/OpenTypeVerticalData\\.cpp$'],
> +            ['include', 'platform/graphics/opentype/OpenTypeVerticalData\\.h$'],
> +          ],
> +        }],

You don't need to add new OS==win conditional block.  We should add these files in the existing OS==win conditional block above.

> Source/WebCore/config.h:166
> +/* Vertical text flow for OpenType fonts using our own code */
> +#if PLATFORM(CHROMIUM) && OS(WINDOWS)
> +#define ENABLE_OPENTYPE_VERTICAL 1
> +#endif

I prefer adding ENABLE_OPENTYPE_VERTICAL to Source/WebKit/chromium/features.gypi.
Comment 39 Koji Ishii 2012-08-11 17:00:34 PDT
Comment on attachment 157870 [details]
No longer depends on 48459, but needs ChangeLog and tests

View in context: https://bugs.webkit.org/attachment.cgi?id=157870&action=review

>> Source/WebCore/WebCore.gyp/WebCore.gyp:1829
>> +        }],
> 
> You don't need to add new OS==win conditional block.  We should add these files in the existing OS==win conditional block above.

Thank you Kent for the prompt advice, fixed.

>> Source/WebCore/config.h:166
>> +#endif
> 
> I prefer adding ENABLE_OPENTYPE_VERTICAL to Source/WebKit/chromium/features.gypi.

Thank you for this one too, fixed.

>> Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:39
>> +#include "FontCache.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

I suppose this is a false alarm as files above are Windows header file and uses <>.
Should I fix this? If so, how? Advices are appreciated in advance.
Comment 40 Koji Ishii 2012-08-11 17:01:48 PDT
Created attachment 157880 [details]
Fixed WebCore.gypi, and features.gyp as per Kent's comments
Comment 41 WebKit Review Bot 2012-08-11 17:03:35 PDT
Attachment 157880 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.gyp/WebCore.gyp', u..." exit_code: 1
Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Koji Ishii 2012-08-17 22:07:36 PDT
Created attachment 159260 [details]
Rebaseline
Comment 43 Koji Ishii 2012-08-17 22:15:33 PDT
Created attachment 159261 [details]
Rebaseline with --binary flag
Comment 44 Koji Ishii 2012-08-17 22:20:55 PDT
Reviewed and enabled all tests in fast/writing-mode with following exceptions:
* fast/writing-mode/border-image-* seems to have different issue, needs to be tracked separately.
* fast/writing-mode/vertical-baseline-alignment.html has wrong baseline and wrong expectation file, filed as bug 94410.
* fast/text/international/text-combine-image-test.html is to be changed in bug 93832.
Comment 45 Koji Ishii 2012-08-17 23:30:52 PDT
One more issue: fast/text/international/text-spliced-font-expected.png has wrong expectation for complex path. This is captured in bug 86071, each platform needs fix but in general, WebKit handles vertical flow poorly in complex path.

On Windows, the complex code path issue is more complicated than other platform because Uniscribe does not support East Asian vertical flow. I had some discussion with bashi at google. I think the best long term plan for Chromium Win is to move to DirectWrite, which supports both East Asian vertical flow and complex scripts. We may also need to rely on harfbuzz-ng for complex OpenType handling as spec'ed in CSS Writing Modes Level 3, but our discussion hasn't come to a conclusion whether just DirectWrite is good enough to cover the spec or not.

This bug is to cover simple code path only, which covers most cases required in East Asia. I'll keep watching for more comprehensive solution for complex code path.
Comment 46 Koji Ishii 2012-08-19 01:07:22 PDT
Created attachment 159285 [details]
Added ChangeLog and more tests
Comment 47 WebKit Review Bot 2012-08-19 01:11:00 PDT
Attachment 159285 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebKit/chromium/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebKit/chromium/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebKit/chromium/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebKit/chromium/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebKit/chromium/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 15 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 48 Koji Ishii 2012-08-19 01:23:16 PDT
Created attachment 159286 [details]
Added ChangeLog and more tests
Comment 49 Koji Ishii 2012-08-20 04:47:10 PDT
Comment on attachment 159286 [details]
Added ChangeLog and more tests

This patch covers simple cases only, but is good enough for most simple vertical flow documents.

* fast/writing-mode/vertical-baseline-alignment.html has wrong baseline and wrong expectation file, filed as bug 94410.
* fast/writing-mode/border-image-* seems to have different issue, needs to be tracked separately. Maybe related with 94410, but haven't looked into in depth yet.
* Complex code path isn't supported yet, and therefore complex code path part of fast/text/international/text-spliced-font isn't fixed in this patch.
Comment 50 Tony Chang 2012-08-20 14:51:29 PDT
This looks great!

The patch is still pretty big making it hard for me to review it.  Here are some suggestions on how to make it smaller:

1) First land code that is not behind #if ENABLE(OPENTYPE_VERTICAL) as separate patches. Ideally, there would be test cases for each function as they are implemented, but that may not be possible.

2) Land the code needed that is behind #if ENABLE(OPENTYPE_VERTICAL).

3) Land the change to features.gypi + layout test results.

That way, if there are any perf regressions, it's easy to roll out step (3) only.
Comment 51 Kent Tamura 2012-08-20 18:24:02 PDT
"コ" in the following images lacks the bottom line.  Is it expected?

fast/writing-mode/border-vertical-lr-expected.png
fast/writing-mode/japanese-lr-selection-expected.png
fast/writing-mode/japanese-lr-text-expected.png
fast/writing-mode/japanese-rl-selection-expected.png
fast/writing-mode/japanese-rl-text-expected.png
Comment 52 Koji Ishii 2012-08-21 05:39:03 PDT
(In reply to comment #51)
> "コ" in the following images lacks the bottom line.  Is it expected?
> 
> fast/writing-mode/border-vertical-lr-expected.png
> fast/writing-mode/japanese-lr-selection-expected.png
> fast/writing-mode/japanese-lr-text-expected.png
> fast/writing-mode/japanese-rl-selection-expected.png
> fast/writing-mode/japanese-rl-text-expected.png

Actually, no, I wondered but just thought DRT did some scaling...sorry, I'll check more carefully on the next submit. Thank you for pointing this out.
Comment 53 Koji Ishii 2012-08-21 05:41:15 PDT
(In reply to comment #50)
> This looks great!
> 
> The patch is still pretty big making it hard for me to review it.  Here are some suggestions on how to make it smaller:
> 
> 1) First land code that is not behind #if ENABLE(OPENTYPE_VERTICAL) as separate patches. Ideally, there would be test cases for each function as they are implemented, but that may not be possible.
> 
> 2) Land the code needed that is behind #if ENABLE(OPENTYPE_VERTICAL).
> 
> 3) Land the change to features.gypi + layout test results.
> 
> That way, if there are any perf regressions, it's easy to roll out step (3) only.

Thank you Tony for the prompt and wonderful suggestion. A couple of changes in font metrics required rebaselining non-vertical cases, so I agree that your suggestion makes such changes separated from others. I'll reactivate 83512 and put item 1 into it, make another bug, and then back to this bug for item 3.

Thank you again.
Comment 54 Koji Ishii 2012-08-21 05:42:09 PDT
Comment on attachment 159286 [details]
Added ChangeLog and more tests

review cancelled to split the patch as suggested by Tony.
Comment 55 Koji Ishii 2012-08-25 07:00:09 PDT
(In reply to comment #52)
> (In reply to comment #51)
> > "コ" in the following images lacks the bottom line.  Is it expected?
> 
> Actually, no, I wondered but just thought DRT did some scaling...sorry, I'll check more carefully on the next submit. Thank you for pointing this out.

Turns out this issue occurs when no Japanese fonts are in the font list in the CSS. It then fallback to system, and it looks like a Chinese font is chosen. Adding "MS Gothic" to font-family fixes the issue. I'll fix all relevant CSS files on the next submit.
Comment 56 Koji Ishii 2012-08-25 07:05:11 PDT
Created attachment 160564 [details]
Enable OPENTYPE_VERTICAL in features.gypi, enabled ~10 tests, and rebaselines

The patch only for:
> 3) Land the change to features.gypi + layout test results.
but a lot of tests were rebaselined and therefore the patch isn't small.

This patch also includes a fix (rebaseline) of bug 94771 after reviewing the new image. In the previous patch, I was too focused on fast/* and missed failure in other directories, sorry about that. This time I reviewed all tests including outside of fast/* more carefully.
Comment 57 Koji Ishii 2012-08-25 07:09:25 PDT
Forgot to note that this patch requires bug 94822, but no longer depends on bug 48459. I can't see "edit" button on "Depends on" links and couldn't figure out how to remove 48459, if someone can remove it or tell me how I can, that'd be helpful.

Also I hesitated to submit for EWS as I'm not sure whether EWS applies "Depends on" patches or not.
Comment 58 Tony Chang 2012-08-27 10:38:40 PDT
(In reply to comment #57)
> Also I hesitated to submit for EWS as I'm not sure whether EWS applies "Depends on" patches or not.

EWS doesn't apply dependent patches, so you'll have to wait until bug 94822 is resolved.

For reference, when this patch lands, you want to watch the page cyclers on http://build.chromium.org/p/chromium.webkit/waterfall?show=Win7%20Perf .  If any regress, we should revert and try to fix.
Comment 59 Koji Ishii 2012-08-29 21:30:33 PDT
Created attachment 161397 [details]
Resolved merge conflicts and updated test results
Comment 60 Koji Ishii 2012-08-29 21:32:33 PDT
Created attachment 161398 [details]
Wrong, --binary was missing
Comment 61 Kent Tamura 2012-08-29 21:55:55 PDT
(In reply to comment #60)
> Created an attachment (id=161398) [details]
> Wrong, --binary was missing

Why don't you use "webkit-patch upload"?

Anyway, the pixel results look correct to me.
I prefer removing the test results from the patch. because
 - The patch is too large.
 - The results in the patch might have differences from results produced by our buildbots
 - The results might have differences between Windows XP, Windows Vista, and Windows 7.

Adding entries like the following to TestExpectations would be enough. I'll update the expectation files later in this case.

// Need rebaseline
BUGWK51450 WIN : fast/writing-mode/japanese-lr-text.html = IMAGE+TEXT
BUGWK51450 WIN : fast/writing-mode/japanese-rl-text-with-broken-font.html = IMAGE+TEXT
...
Comment 62 Koji Ishii 2012-08-30 03:37:50 PDT
Created attachment 161436 [details]
Resolved yet another merge conflict
Comment 63 Tony Chang 2012-08-30 10:19:58 PDT
(In reply to comment #61)
> Anyway, the pixel results look correct to me.
> I prefer removing the test results from the patch. because

This is good advice.  We can land the features.gypi and TestExpectations change then watch the perf bots.  If the perf bots don't regress, we can land the images and additional TestExpectations changes (anyone could do this step).
Comment 64 Koji Ishii 2012-08-30 15:50:36 PDT
Comment on attachment 161436 [details]
Resolved yet another merge conflict

ouch, found Kent's advise, canceling r?.
Comment 65 Koji Ishii 2012-08-31 09:09:32 PDT
Created attachment 161715 [details]
Patch
Comment 66 Koji Ishii 2012-08-31 09:21:39 PDT
(In reply to comment #61)
> (In reply to comment #60)
> > Created an attachment (id=161398) [details] [details]
> > Wrong, --binary was missing
> 
> Why don't you use "webkit-patch upload"?

Just wasn't familiar with it and was hesitate as I couldn't be confident enough to try it out. Tried this time.

> Anyway, the pixel results look correct to me.
> I prefer removing the test results from the patch. because
>  - The patch is too large.
>  - The results in the patch might have differences from results produced by our buildbots
>  - The results might have differences between Windows XP, Windows Vista, and Windows 7.
> 
> Adding entries like the following to TestExpectations would be enough. I'll update the expectation files later in this case.
> 
> // Need rebaseline
> BUGWK51450 WIN : fast/writing-mode/japanese-lr-text.html = IMAGE+TEXT
> BUGWK51450 WIN : fast/writing-mode/japanese-rl-text-with-broken-font.html = IMAGE+TEXT
> ...

Thanks for the advise, yeah, rebaseline this many files was a little pain as it was so easy for the merge to conflict. This is really helpful, thank you again.
Comment 67 Kent Tamura 2012-08-31 21:05:46 PDT
Comment on attachment 161715 [details]
Patch

Clearing flags on attachment: 161715

Committed r127354: <http://trac.webkit.org/changeset/127354>
Comment 68 Kent Tamura 2012-08-31 21:05:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 70 Koji Ishii 2012-09-01 07:14:36 PDT
(In reply to comment #69)
> Followup changes:
> http://trac.webkit.org/changeset/127355
> http://trac.webkit.org/changeset/127358
> http://trac.webkit.org/changeset/127359

Thank you so much for landing, for followups. So much appreciated!
Comment 71 Kent Tamura 2012-09-04 16:33:48 PDT
It seems we have no performance regression by this.
I'll do rebaseline today.
Comment 72 Kent Tamura 2012-09-04 21:30:11 PDT
(In reply to comment #71)
> I'll do rebaseline today.

Done: http://trac.webkit.org/changeset/127543