Bug 51450 – Glyphs in vertical text tests are rotated 90 degrees clockwise on Chromium Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Chun-Lung Huang]

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.

[Chun-Lung Huang]

Created attachment 77196 [details]
a proposed patch for 51450

[Chun-Lung Huang]

Created attachment 77313 [details]
A proposed patch for 51450 with CHANGELOG.

[WebKit Review Bot]

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.

[Chun-Lung Huang]

Created attachment 77314 [details]
A proposed patch for 51450 with CHANGELOG.

remove the accident [tab]

[Chun-Lung Huang]

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

[Kent Tamura]

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.

[Chun-Lung Huang]

(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/

[Chun-Lung Huang]

Created attachment 77658 [details]
An updated patch for 51450

Changes made according to Kent Tamura's comments.

[Kent Tamura]

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.

[Chun-Lung Huang]

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.

[Kent Tamura]

(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.

[Dave Hyatt]

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.

[Chun-Lung Huang]

(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

[Kent Tamura]

Bono-san, Jungshik, do you have any comments on this?

[Hironori Bono]

(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

[Chun-Lung Huang]

(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

[Hironori Bono]

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

[Jungshik Shin]

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?

[Chun-Lung Huang]

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

[Kent Tamura]

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.

[Chun-Lung Huang]

Created attachment 82893 [details]
change accroding to reviewer's comments

Thank you

[Kent Tamura]

Comment on attachment 82893 [details]
change accroding to reviewer's comments

ok.  I'll test the patch locally and commit it later.

[Kent Tamura]

Comment on attachment 82893 [details]
change accroding to reviewer's comments

Clearing flags on attachment: 82893

Committed r79169: <http://trac.webkit.org/changeset/79169>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(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

[WebKit Review Bot]

http://trac.webkit.org/changeset/79169 might have broken GTK Linux 32-bit Debug

[Ryosuke Niwa]

The approach taken by this patch isn't correct.  See http://stackoverflow.com/questions/2250437/how-to-draw-vertical-text-in-windows-gui.

[Hironori Bono]

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

[Ryosuke Niwa]

(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.

[Koji Ishii]

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.

[Koji Ishii]

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.

[Koji Ishii]

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

[Tony Chang]

(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.

[Koji Ishii]

(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?

[Tony Chang]

Sorry, I didn't realize those other bugs where blocking.

[Koji Ishii]

Created attachment 157870 [details]
No longer depends on 48459, but needs ChangeLog and tests

[WebKit Review Bot]

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.

[Kent Tamura]

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.

[Koji Ishii]

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.

[Koji Ishii]

Created attachment 157880 [details]
Fixed WebCore.gypi, and features.gyp as per Kent's comments

[WebKit Review Bot]

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.

[Koji Ishii]

Created attachment 159260 [details]
Rebaseline

[Koji Ishii]

Created attachment 159261 [details]
Rebaseline with --binary flag

[Koji Ishii]

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.

[Koji Ishii]

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.

[Koji Ishii]

Created attachment 159285 [details]
Added ChangeLog and more tests

[WebKit Review Bot]

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.

[Koji Ishii]

Created attachment 159286 [details]
Added ChangeLog and more tests

[Koji Ishii]

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.

[Tony Chang]

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.

[Kent Tamura]

"コ" 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

[Koji Ishii]

(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.

[Koji Ishii]

(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.

[Koji Ishii]

Comment on attachment 159286 [details]
Added ChangeLog and more tests

review cancelled to split the patch as suggested by Tony.

[Koji Ishii]

(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.

[Koji Ishii]

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.

[Koji Ishii]

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.

[Tony Chang]

(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.

[Koji Ishii]

Created attachment 161397 [details]
Resolved merge conflicts and updated test results

[Koji Ishii]

Created attachment 161398 [details]
Wrong, --binary was missing

[Kent Tamura]

(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
...

[Koji Ishii]

Created attachment 161436 [details]
Resolved yet another merge conflict

[Tony Chang]

(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).

[Koji Ishii]

Comment on attachment 161436 [details]
Resolved yet another merge conflict

ouch, found Kent's advise, canceling r?.

[Koji Ishii]

Created attachment 161715 [details]
Patch

[Koji Ishii]

(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.

[Kent Tamura]

Comment on attachment 161715 [details]
Patch

Clearing flags on attachment: 161715

Committed r127354: <http://trac.webkit.org/changeset/127354>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Followup changes:
http://trac.webkit.org/changeset/127355
http://trac.webkit.org/changeset/127358
http://trac.webkit.org/changeset/127359

[Koji Ishii]

(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!

[Kent Tamura]

It seems we have no performance regression by this.
I'll do rebaseline today.

[Kent Tamura]

(In reply to comment #71)
> I'll do rebaseline today.

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