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

• Component: Text
• Status: REOPENED

[Adam Roben (:aroben)]

If you run fast/blockflow/japanese-*-text.html on Windows you'll see that the glyphs are rotated 90 degrees clockwise from how they appear on Mac. (See screenshots.) I've checked in Windows-specific results for these tests, but we should fix them to draw the glyphs in the correct orientation.

[Adam Roben (:aroben)]

<rdar://problem/8601253>

[Dimitri Glazkov (Google)]

Same in Chromium Linux/Win, btw.

I don’t think vertical text is implemented on any platform besides Mac OS X yet.

[Adam Roben (:aroben)]

Also:

fast/blockflow/border-vertical-lr.html

[Koan-Sin Tan]

Created attachment 75260 [details]
A patch to make Chromium Linux render vertical text correctly

A quick hack to make vertical text work correctly on Chromium Linux

[Adam Roben (:aroben)]

Comment on attachment 75260 [details]
A patch to make Chromium Linux render vertical text correctly

This patch does not belong on this bug. This bug is about Windows, not Linux.

[Koan-Sin Tan]

(In reply to comment #6)
> (From update of attachment 75260 [details])
> This patch does not belong on this bug. This bug is about Windows, not Linux.

Sorry about that, I saw Dimitri mentioned Chromium Linux/Win, so..
What should I do? File a new bug report? I have a vertical text patch 
for WebKit-Gtk, too

[Kent Tamura]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 75260 [details] [details])
> > This patch does not belong on this bug. This bug is about Windows, not Linux.
> 
> Sorry about that, I saw Dimitri mentioned Chromium Linux/Win, so..
> What should I do? File a new bug report? I have a vertical text patch 
> for WebKit-Gtk, too

Yes, please file a new bug for Chromium.

[Koan-Sin Tan]

Short answer to the problem: '@'. Yes, just one character.

My colleague and I set up windows development environment to work on this bug. Since we are not fluent Windows programmers, we read related code a little bit and googled related information while waiting for the compilation. Suddenly, we were enlightened that, in HTML/CSS code, adding '@'  prefix to the font family you wanna display vertically solved the problem. 

Yes, this is not the right answer. However, using '@' in FontPlatformData constructor, when in -webkit-writing-mode: vertical-{lr,rl}, looks right.

[Chun-Lung Huang]

Created attachment 77702 [details]
A patch to make WebKit Windows render vertical text correctly

Encouraged by my colleague to make WebKit(Windows) render vertical text correctly.

Some layout tests images
http://www.flickr.com/photos/burorly/sets/72157625585506341/

[Dave Hyatt]

Comment on attachment 77702 [details]
A patch to make WebKit Windows render vertical text correctly

If I'm understanding this correctly, you're just putting @ in front of the family name to get the fonts hashed separately to trip the broken ideographic font code path.  That isn't really supporting vertical text.  Punctuation glyphs won't be correct for example.

I would prefer orientation just be part of the constructed hash key rather than hacking the family name.  We have to do this with bold and oblique already, so I think that's a better approach than hacking the family name.

Even once you do this, though, you're only sort of supporting vertical text here.  The simple code path is using the broken ideographic font code, which won't really use vertical glyphs from a font with vertical tables (like punctuation glyphs), and the complex code path (that uses Uniscribe) isn't doing anything either.

[Koan-Sin Tan]

(In reply to comment #11)
> (From update of attachment 77702 [details])
> If I'm understanding this correctly, you're just putting @ in front of the family name to get the fonts hashed separately to trip the broken ideographic font code path.  That isn't really supporting vertical text.  Punctuation glyphs won't be correct for example.
> 

First, thanks for the great work you did for writing modes.

What amazed us when my colleague was trying various approaches on Windows is that @ works like a charm. Punctuation glyphs work really correctly. That's why we use it finally. Please take a look at 

http://www.flickr.com/photos/burorly/5308587584/in/set-72157625585506341/

> I would prefer orientation just be part of the constructed hash key rather than hacking the family name.  We have to do this with bold and oblique already, so I think that's a better approach than hacking the family name.
> 

OK, We'l try that. But please do consider that @ can nearly all the jobs

> Even once you do this, though, you're only sort of supporting vertical text here.  The simple code path is using the broken ideographic font code, which won't really use vertical glyphs from a font with vertical tables (like punctuation glyphs), and the complex code path (that uses Uniscribe) isn't doing anything either.

I don't know anything the complex code path. But what surprised us is that,  with '@', we got less broken fonts. E.g., kaiu.ttf (DFKai-SB,標楷體), which doesn't have any vertical metrics (vhea, vmtx, etc.) so it will be in broken font path in WebKit OS X,does look right with '@'.

For reference on what prepending an @ does in GDI:
<http://blogs.msdn.com/b/michkap/archive/2005/08/04/447759.aspx>
<http://blogs.msdn.com/b/michkap/archive/2007/03/31/2000190.aspx>

[Koan-Sin Tan]

(In reply to comment #13)
> For reference on what prepending an @ does in GDI:
> <http://blogs.msdn.com/b/michkap/archive/2005/08/04/447759.aspx>
> <http://blogs.msdn.com/b/michkap/archive/2007/03/31/2000190.aspx>

Thanks, Dan, the figure in the article pointed by the first URL is a good one. It shows punctuations are rotate and replaced correctly :-)

[Chun-Lung Huang]

(In reply to comment #11)
> (From update of attachment 77702 [details])
> I would prefer orientation just be part of the constructed hash key rather than hacking the family name.  We have to do this with bold and oblique already, so I think that's a better approach than hacking the family name.

Thank you.  We thought that way at beginning.  After search and test, however, we found that Win32 Text APIs already did things for vertical writing(and line breaking/word breaking as well).  It seems that @ can do nearly all the jobs.
http://msdn.microsoft.com/en-us/goglobal/bb688137

On Windows platform, if we change the original html script by manually adding '@' in front of the font family, we could get what we need with WebKit Nightly Builds.
http://www.flickr.com/photos/burorly/5332235280/in/set-72157625585506341/
(punctuations are rotated and replaced correctly)

If we locally apply this patch and load the test case 'english-rl-text.html', the letters in vertical writing mode look correctly, not rotated even '@' was added by the patch.
http://www.flickr.com/photos/burorly/5332283402/in/set-72157625585506341/
same as 'broken-ideographic-font.html'
http://www.flickr.com/photos/burorly/5308588600/in/set-72157625585506341/

I'll try to test this in pure English environment to see if it still works well.


Thanks for Dan and Koan-Sin's comments.

[Dave Hyatt]

Oh interesting!  I did not realize @ was a built-in special behavior!  In that case, the patch is probably fine.

[Dave Hyatt]

Comment on attachment 77702 [details]
A patch to make WebKit Windows render vertical text correctly

I guess I will tentatively r+ this, although I'm not sure what behavior you get with the @ added.  By not setting the orientation in SimpleFontData, you will end up with that code thinking the font doesn't support vertical, but if the built-in rendering just "does the right thing" then maybe that's ok.  I suspect it won't be sufficient though, especially when we get around to supporting text-orientation.  Still, if this gets the basics working then that seems pretty good.

[Dave Hyatt]

(In reply to comment #12)

> I don't know anything the complex code path. But what surprised us is that,  with '@', we got less broken fonts. E.g., kaiu.ttf (DFKai-SB,標楷體), which doesn't have any vertical metrics (vhea, vmtx, etc.) so it will be in broken font path in WebKit OS X,does look right with '@'.

Maybe they are clever enough to do fallback for vertical punctuation glyphs?

[Chun-Lung Huang]

Created attachment 80731 [details]
add more comments and prevent crash in English environment

1. Thanks for all comments.  I've added some descriptive comments and tested new patch in English environment.  It will not crash in English environment and basics are working.  What should I do next?

2. BTW, after doing 'svn update', I found that the newest WebKit nightly build, WebKit r77046(Windows), cannot correctly render pages with {-webkit-writing-mode: vertical-rl;}.  If you scroll left, WebKit shows you nothing!
http://www.flickr.com/photos/burorly/5406277169/

It was there before my patch and I found that 'WebKit-r76750' works fine but 'WebKit-r76926' begins to behave unexpected.  A bug?


Alvin

[Eric Seidel (no email)]

Comment on attachment 77702 [details]
A patch to make WebKit Windows render vertical text correctly

Cleared David Hyatt's review+ from obsolete attachment 77702 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Brent Fulgham]

Comment on attachment 80731 [details]
add more comments and prevent crash in English environment

Could you please regenerate this patch against the current source tree?  Some of your changes (e.g., the orientation initialization stuff) is now in the ToT.  This will make your patch smaller and much easier to review.

[Chun-Lung Huang]

Created attachment 90680 [details]
regenerate patch against the current source tree

(In reply to comment #21)

> Could you please regenerate this patch against the current source tree?  Some of your changes (e.g., the orientation initialization stuff) is now in the ToT.  This will make your patch smaller and much easier to review.

done

Alvin

[WebKit Commit Bot]

Comment on attachment 90680 [details]
regenerate patch against the current source tree

Clearing flags on attachment: 90680

Committed r84989: <http://trac.webkit.org/changeset/84989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84989 might have broken Windows 7 Release (Tests)
The following tests are not passing:
fast/blockflow/Kusa-Makura-background-canvas.html
fast/blockflow/border-vertical-lr.html
fast/blockflow/fallback-orientation.html
fast/blockflow/japanese-lr-selection.html
fast/blockflow/japanese-lr-text.html
fast/blockflow/japanese-rl-selection.html
fast/blockflow/japanese-rl-text.html
fast/blockflow/japanese-ruby-vertical-lr.html
fast/blockflow/japanese-ruby-vertical-rl.html
fast/dynamic/text-combine.html
fast/repaint/japanese-rl-selection-clear.html
fast/repaint/japanese-rl-selection-repaint.html
fast/ruby/base-shorter-than-text.html
fast/text/emphasis-vertical.html
fast/text/international/text-combine-image-test.html
fast/text/international/vertical-text-glyph-test.html
fast/text/justify-ideograph-vertical.html

[Kent Tamura]

(In reply to comment #25)
> http://trac.webkit.org/changeset/84989 might have broken Windows 7 Release (Tests)
> The following tests are not passing:
> fast/blockflow/Kusa-Makura-background-canvas.html
> fast/blockflow/border-vertical-lr.html
> fast/blockflow/fallback-orientation.html
> fast/blockflow/japanese-lr-selection.html
> fast/blockflow/japanese-lr-text.html
> fast/blockflow/japanese-rl-selection.html
> fast/blockflow/japanese-rl-text.html
> fast/blockflow/japanese-ruby-vertical-lr.html
> fast/blockflow/japanese-ruby-vertical-rl.html
> fast/dynamic/text-combine.html
> fast/repaint/japanese-rl-selection-clear.html
> fast/repaint/japanese-rl-selection-repaint.html
> fast/ruby/base-shorter-than-text.html
> fast/text/emphasis-vertical.html
> fast/text/international/text-combine-image-test.html
> fast/text/international/vertical-text-glyph-test.html
> fast/text/justify-ideograph-vertical.html

The purpose of the patch is to fix their wrong results.  We need to examine pixel results.

[Adam Roben (:aroben)]

Reopening this bug because the fix was rolled out. (See bug 59566.)

[Yuan-Yi Chang]

Created attachment 109897 [details]
regenerate patch with test cases

[Yuan-Yi Chang]

Created attachment 109898 [details]
regenerate patch with test cases (replace tabs to spaces)

[WebKit Review Bot]

Attachment 109898 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp:72:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuan-Yi Chang]

Created attachment 109921 [details]
regenerate patch with test cases

[WebKit Review Bot]

Attachment 109921 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuan-Yi Chang]

Created attachment 109923 [details]
regenerate patch with test cases

[Adam Roben (:aroben)]

Comment on attachment 109923 [details]
regenerate patch with test cases

If you want your patch to be reviewed, you should mark it r?. If you want it to be committed after it's reviewed, you should mark it cq?. I'll assume both of those are true in this case and you just didn't know.

I think many of these tests have Windows-specific pixel results (the *-expected.png files). They need to be regenerated too. You can get pixel results to be generated by passing -p to run-webkit-tests.

[Yuan-Yi Chang]

(In reply to comment #34)
> (From update of attachment 109923 [details])
> If you want your patch to be reviewed, you should mark it r?. If you want it to be committed after it's reviewed, you should mark it cq?. I'll assume both of those are true in this case and you just didn't know.
> 
> I think many of these tests have Windows-specific pixel results (the *-expected.png files). They need to be regenerated too. You can get pixel results to be generated by passing -p to run-webkit-tests.

Thanks for your help. I also made the pixel results in previous patch which was too large to be uploaded. I will make a new one for them.

[Yuan-Yi Chang]

Created attachment 110085 [details]
Windows-specific pixel results

[Adam Roben (:aroben)]

Comment on attachment 109923 [details]
regenerate patch with test cases

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

> Source/WebCore/ChangeLog:16
> +        This patch was generated by Chun-Lung Huang <[e-mail]>
> +        at 2011-04-22.

I think it would be even more helpful to say the revision the patch was originally landed in, and explain why it was rolled out.

[Adam Roben (:aroben)]

Comment on attachment 110085 [details]
Windows-specific pixel results

Are these really the only tests we have that use vertical text?

[Adam Roben (:aroben)]

I think we just need an updated description in the WebCore ChangeLog, and then I can cq+ both of these patches.

[Yuan-Yi Chang]

Created attachment 110455 [details]
regenerate patch with test cases

An updated description in the WebCore ChangeLog

[Yuan-Yi Chang]

(In reply to comment #38)
> (From update of attachment 110085 [details])
> Are these really the only tests we have that use vertical text?

According to the Comment #25 (WebKit Review Bot 2011-04-26 18:08:26 PST), I only  built the expected tests of them.

In my environment (Win 7 64-Bit English version & VC++ 2005 Express & some fonts regenerated from Mac OSX 10.6), I got lots tests failed without this patch. The errors may be related with the fonts, so I didn't run all tests.

[Eric Seidel (no email)]

Ping, aroben?

[Adam Roben (:aroben)]

Comment on attachment 110455 [details]
regenerate patch with test cases

I'm nervous about landing these patches while the bots are so red. It will be very hard to detect regressions they may cause. I'm not sure what the best way forward here is.

[Koji Ishii]

Created attachment 130022 [details]
New approach without using @-font API (in progress)

Here's a new proposal to fix two issues in the original patch:
1. It does not support text-orientation property
2. It relies on @-font Windows API, which rotates some code points automatically. Since CSS defines orientations, that automatic behavior can be harmful.

The discussion started at:
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019562.html
Ryosuke responded to prefer WebKit reads raw OpenType data here:
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019565.html
and David Kilzer prefers not to rely on WebKitSystemInterface here:
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019647.html

So, I created a new class OpenTypeVerticalData to handle this. The class itself is ~20k, and the patch is still in progress, but I'd like to post the patch in case someone could look at and give early feedback.

The patch is still work in progress in the following points:
1. Several FIXME comments are in the new code.
2. Vertical advance and glyph substitution are done, but vertical origin isn't done yet.
3. Since it's reading OpenType tables directly, there's a risk of buffer-overrun if the font is broken. I added checks in the last minutes, but I think I need more and the code needs careful reviews.
4. By looking at the results, there seems to be some rounding errors in small font sizes which doesn't look nice. I'd like to look into that further to see if there were any good way to avoid it.
5. Non-OpenType font isn't supported very well. I'd like to have a fallback, 2nd-level support for such fonts.
6. I'm still new to webkit (this is 2nd patch,) I'll review coding style guidelines further. Note that I did read arrays are preferred than iterator for readability reasons, but I'm using iterator since pointers are needed to check buffer overruns.

This is my weekend project, so I'm not making as fast progress as I wish it to be, but any feedback are greatly appreciated and I'd like to make this to a good state in not so long future.

[WebKit Review Bot]

Attachment 130022 [details] did not pass style-queue:

Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:20:  The parameter name "fontData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:21:  The parameter name "fontData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:25:  The parameter name "glyph" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:26:  The parameter name "glyphPage" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:317:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:322:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:324:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:340:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:341:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:342:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:343:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:344:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:345:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:346:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:347:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:352:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:355:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:356:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:357:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:358:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:362:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:363:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:450:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/win/FontCGWin.cpp:453:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/FontPlatformData.h:192:  The parameter name "orientation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:194:  The parameter name "orientation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/win/SimpleFontDataWin.cpp:288:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/win/SimpleFontDataWin.cpp:292:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/SimpleFontData.h:212:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/SimpleFontData.h:213:  The parameter name "size" adds no information, so it should Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.vcproj/WebCore.vcpr..." exit_code: 1
be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/win/GlyphPageTreeNodeCGWin.cpp:59:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:38:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:41:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:47:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:168:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:180:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:185:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:191:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:203:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:233:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:292:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:334:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:433:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:458:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:484:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:506:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:509:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:510:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 49 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Koji Ishii]

Forgot to run check-webkit-style before upload, sorry about that.

Also forgot another work item:
7. OpenTypeVerticalData loads information for all glyphs in constructor. I might want to move some of them to on-demand, my thinking is to use GlyphMetricsMap class for per-page loading and caching.

[Koji Ishii]

Created attachment 130188 [details]
130022: New approach without using @-font API (in progress)

Revised the patch I posted at comment #44.

Changes from the previous patch:
1. Fixed styles.
2. Vertical origin using VORG/vmtx, which is required for CFF OpenType fonts.
3. GSUB coverage format 2 support for some Adobe fonts.
4. Support !useGDI()

There are still several work items to do:
1. Several FIXME comments are in the new code.
2. Currently OpenTypeVerticalData loads multiple times for the same font. Having it in FontPlatformData might help some. Also some of OpenTypeVerticalData are size-independent, so I might try to share among multiple SimpleFontData/FontPlatformData.
3. Since it's reading OpenType tables directly, there's a risk of buffer-overrun if the font is broken. I added checks in the last minutes, but I think I need more and the code needs careful reviews.
4. By looking at the results, there seems to be some rounding errors in small font sizes which doesn't look nice. I'd like to look into that further to see if there were any good way to avoid it.
5. Non-OpenType font isn't supported very well. I'd like to have a fallback, 2nd-level support for such fonts, but not sure.
6. I'm still new to webkit (this is 2nd patch,) I'll review coding style guidelines further. Note that I did read arrays are preferred than iterator for readability reasons, but I'm using iterator since pointers are needed to check buffer overruns.
7. OpenTypeVerticalData loads information for all glyphs in its constructor. I might want to move some of them to on-demand, my thinking is to use GlyphMetricsMap class for per-page loading and caching.
8. Complex path doesn't work because it reads advance from Uniscribe. I'm not sure how I can make it work at this point.

[Koji Ishii]

Regarding the SimpleFontData v.s. FontPlatformData, my conclusion was to keep vertical data in SimpleFontData because it's where SVG font data lives. I'm not going to support SVG fonts in this patch, but if we were in future, we can switch the logic between OpenType and SVG in SimpleFontData.

I'd like to look into FontCache and try to put vertical data there, so that its instance exists only per font file and SimpleFontData simply points to a shared instance.

[Koji Ishii]

A class to read OpenType table was split into bug 81326.

[Koji Ishii]

Created attachment 132331 [details]
New approach without using @-font API (in progress)

Still work in progress; this patch requires patches in bug 81326 and bug 81332 applied.

[Koji Ishii]

Created attachment 135007 [details]
New approach without using @-font API (in progress)

Updated to rebase to new patches posted in bug 81332 and bug 81389.

I want to apply this to the SVG.

To be diverted to the SVG from HTML, a portion for selecting the glyph.

[Koji Ishii]

(In reply to comment #52)
> I want to apply this to the SVG.

This bug is specific to Windows version of Safari and therefore I no longer work on this. I suppose you mean other platforms.

It's probably better to file another bug, specifying on which platform (or all) you want to fix for.

[Anders Carlsson]

Comment on attachment 110455 [details]
regenerate patch with test cases

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.