Bug 48459 - Glyphs in vertical text tests are rotated 90 degrees clockwise on Windows
Summary: Glyphs in vertical text tests are rotated 90 degrees clockwise on Windows
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, LayoutTestFailure, PlatformOnly
Depends on: 59566 81332 81389
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-27 13:07 PDT by Adam Roben (:aroben)
Modified: 2015-12-02 11:01 PST (History)
16 users (show)

See Also:


Attachments
A patch to make Chromium Linux render vertical text correctly (7.66 KB, patch)
2010-12-01 00:09 PST, Koan-Sin Tan
no flags Details | Formatted Diff | Diff
A patch to make WebKit Windows render vertical text correctly (8.84 KB, patch)
2010-12-30 21:01 PST, Chun-Lung Huang
no flags Details | Formatted Diff | Diff
add more comments and prevent crash in English environment (10.86 KB, patch)
2011-02-01 02:03 PST, Chun-Lung Huang
no flags Details | Formatted Diff | Diff
regenerate patch against the current source tree (9.86 KB, patch)
2011-04-22 02:12 PDT, Chun-Lung Huang
no flags Details | Formatted Diff | Diff
regenerate patch with test cases (1.04 MB, text/plain)
2011-10-05 18:21 PDT, Yuan-Yi Chang
no flags Details
regenerate patch with test cases (replace tabs to spaces) (1.04 MB, text/plain)
2011-10-05 18:28 PDT, Yuan-Yi Chang
no flags Details
regenerate patch with test cases (444.61 KB, text/plain)
2011-10-05 23:25 PDT, Yuan-Yi Chang
no flags Details
regenerate patch with test cases (444.61 KB, patch)
2011-10-05 23:34 PDT, Yuan-Yi Chang
aroben: review-
aroben: commit-queue-
Details | Formatted Diff | Diff
Windows-specific pixel results (1.65 MB, patch)
2011-10-06 20:21 PDT, Yuan-Yi Chang
aroben: review+
changyy.csie: commit-queue?
Details | Formatted Diff | Diff
regenerate patch with test cases (444.70 KB, patch)
2011-10-10 18:29 PDT, Yuan-Yi Chang
no flags Details | Formatted Diff | Diff
New approach without using @-font API (in progress) (39.48 KB, patch)
2012-03-04 02:17 PST, Koji Ishii
no flags Details | Formatted Diff | Diff
130022: New approach without using @-font API (in progress) (43.81 KB, patch)
2012-03-05 13:03 PST, Koji Ishii
no flags Details | Formatted Diff | Diff
New approach without using @-font API (in progress) (15.59 KB, text/plain)
2012-03-16 11:42 PDT, Koji Ishii
no flags Details
New approach without using @-font API (in progress) (16.17 KB, patch)
2012-04-01 10:36 PDT, Koji Ishii
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-10-27 13:07:24 PDT
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.
Comment 1 Adam Roben (:aroben) 2010-10-27 13:16:31 PDT
<rdar://problem/8601253>
Comment 2 Dimitri Glazkov (Google) 2010-10-27 14:14:00 PDT
Same in Chromium Linux/Win, btw.
Comment 3 mitz 2010-10-27 14:15:41 PDT
I don’t think vertical text is implemented on any platform besides Mac OS X yet.
Comment 4 Adam Roben (:aroben) 2010-10-27 14:42:31 PDT
Also:

fast/blockflow/border-vertical-lr.html
Comment 5 Koan-Sin Tan 2010-12-01 00:09:28 PST
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
Comment 6 Adam Roben (:aroben) 2010-12-01 12:42:41 PST
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.
Comment 7 Koan-Sin Tan 2010-12-01 16:44:04 PST
(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
Comment 8 Kent Tamura 2010-12-01 17:43:56 PST
(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.
Comment 9 Koan-Sin Tan 2010-12-14 22:31:00 PST
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.
Comment 10 Chun-Lung Huang 2010-12-30 21:01:47 PST
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/
Comment 11 Dave Hyatt 2011-01-06 15:28:27 PST
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.
Comment 12 Koan-Sin Tan 2011-01-06 16:38:40 PST
(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 '@'.
Comment 14 Koan-Sin Tan 2011-01-06 16:58:35 PST
(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 :-)
Comment 15 Chun-Lung Huang 2011-01-06 21:04:18 PST
(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.
Comment 16 Dave Hyatt 2011-01-19 13:38:18 PST
Oh interesting!  I did not realize @ was a built-in special behavior!  In that case, the patch is probably fine.
Comment 17 Dave Hyatt 2011-01-19 13:40:20 PST
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.
Comment 18 Dave Hyatt 2011-01-19 13:41:32 PST
(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?
Comment 19 Chun-Lung Huang 2011-02-01 02:03:41 PST
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
Comment 20 Eric Seidel (no email) 2011-04-06 10:45:40 PDT
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.
Comment 21 Brent Fulgham 2011-04-10 21:17:57 PDT
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.
Comment 22 Chun-Lung Huang 2011-04-22 02:12:59 PDT
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
Comment 23 WebKit Commit Bot 2011-04-26 17:17:51 PDT
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>
Comment 24 WebKit Commit Bot 2011-04-26 17:17:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2011-04-26 18:08:26 PDT
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
Comment 26 Kent Tamura 2011-04-26 18:24:55 PDT
(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.
Comment 27 Adam Roben (:aroben) 2011-10-03 06:31:34 PDT
Reopening this bug because the fix was rolled out. (See bug 59566.)
Comment 28 Yuan-Yi Chang 2011-10-05 18:21:20 PDT
Created attachment 109897 [details]
regenerate patch with test cases
Comment 29 Yuan-Yi Chang 2011-10-05 18:28:30 PDT
Created attachment 109898 [details]
regenerate patch with test cases (replace tabs to spaces)
Comment 30 WebKit Review Bot 2011-10-05 22:50:55 PDT
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.
Comment 31 Yuan-Yi Chang 2011-10-05 23:25:01 PDT
Created attachment 109921 [details]
regenerate patch with test cases
Comment 32 WebKit Review Bot 2011-10-05 23:29:31 PDT
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.
Comment 33 Yuan-Yi Chang 2011-10-05 23:34:46 PDT
Created attachment 109923 [details]
regenerate patch with test cases
Comment 34 Adam Roben (:aroben) 2011-10-06 06:19:04 PDT
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.
Comment 35 Yuan-Yi Chang 2011-10-06 19:53:30 PDT
(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.
Comment 36 Yuan-Yi Chang 2011-10-06 20:21:36 PDT
Created attachment 110085 [details]
Windows-specific pixel results
Comment 37 Adam Roben (:aroben) 2011-10-07 07:48:17 PDT
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 <alvincl.huang@gmail.com>
> +        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.
Comment 38 Adam Roben (:aroben) 2011-10-07 07:48:22 PDT
Comment on attachment 110085 [details]
Windows-specific pixel results

Are these really the only tests we have that use vertical text?
Comment 39 Adam Roben (:aroben) 2011-10-07 07:48:59 PDT
I think we just need an updated description in the WebCore ChangeLog, and then I can cq+ both of these patches.
Comment 40 Yuan-Yi Chang 2011-10-10 18:29:42 PDT
Created attachment 110455 [details]
regenerate patch with test cases

An updated description in the WebCore ChangeLog
Comment 41 Yuan-Yi Chang 2011-10-10 19:10:13 PDT
(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.
Comment 42 Eric Seidel (no email) 2011-12-21 15:32:46 PST
Ping, aroben?
Comment 43 Adam Roben (:aroben) 2011-12-22 07:19:13 PST
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.
Comment 44 Koji Ishii 2012-03-04 02:17:01 PST
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.
Comment 45 WebKit Review Bot 2012-03-04 02:20:45 PST
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.
Comment 46 Koji Ishii 2012-03-04 03:47:50 PST
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.
Comment 47 Koji Ishii 2012-03-05 13:03:04 PST
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.
Comment 48 Koji Ishii 2012-03-10 02:45:03 PST
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.
Comment 49 Koji Ishii 2012-03-16 02:56:41 PDT
A class to read OpenType table was split into bug 81326.
Comment 50 Koji Ishii 2012-03-16 11:42:26 PDT
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.
Comment 51 Koji Ishii 2012-04-01 10:36:03 PDT
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.
Comment 52 drmashu 2013-06-13 18:55:59 PDT
I want to apply this to the SVG.
Comment 53 drmashu 2013-06-20 22:00:34 PDT
To be diverted to the SVG from HTML, a portion for selecting the glyph.
Comment 54 Koji Ishii 2013-07-01 08:59:51 PDT
(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.
Comment 55 Anders Carlsson 2014-02-05 10:51:06 PST
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.