Created attachment 75343 [details] A patch to make Chromium Linux render vertical text correctly As discussed in https://bugs.webkit.org/show_bug.cgi?id=48459, 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 WebKit Mac and Chromium Mac. The reason is that vertical text is implemented on any platform except Mac OS X (WebCore/platform/graphics/{mac, cocoa} ).
Comment on attachment 75343 [details] A patch to make Chromium Linux render vertical text correctly View in context: https://bugs.webkit.org/attachment.cgi?id=75343&action=review I'm not familiar with this area. So I make some comments on patch style. * Please add a ChangeLog entry. WebCore/ChangeLog. * Please make a patch at the top level directory. You made the patch at WebKit/WebCore, should make at WebKit/. You can use WebKitTools/Scripts/webkit-patch to make an acceptable patch. > platform/graphics/chromium/FontLinux.cpp:106 > + pos2[i].set(x+SkFloatToScalar(adv[i].width()), y); > + pos3[i].set(x+SkFloatToScalar(adv[i].width()), y-SkFloatToScalar(adv[i].width())); Add spaces around operators such as + - > platform/graphics/chromium/FontLinux.cpp:132 > + canvas->drawTextOnPath(glyphs+i, 2, path, 0, paint); Add spaces around + > platform/graphics/chromium/FontLinux.cpp:136 > + } else { > + canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint); > + } Remove { and } > platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:72 > + for (int i=0; i < bufferLength; i++) { > + hb_buffer_add_glyph(buffer, glyphs[i], 0, i); > + } Add spaces around =. Remove { and } > platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:80 > + HB_GSUB_Select_Script(hb_face->gsub, > + HB_MAKE_TAG('D', 'F', 'L', 'T'), &script_index); > + HB_GSUB_Select_Feature(hb_face->gsub, > + HB_MAKE_TAG('v', 'e', 'r', 't'), script_index, > + 0xffff, &feature_index); You don't need wrap these lines. WebKit style allow looooong lines. > platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:90 > + for (int i=0; i < bufferLength; i++) { > + glyphs[i] = (Glyph) buffer->out_string[i].gindex; > + } Remove { and }. We prefer C++-style cast.
Created attachment 75433 [details] A patch to make Chromium Linux render vertical text correctly add an entry to WebCore/ChangeLog and change a bit to conform WebKit coding style
Attachment 75433 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/graphics/chromium/FontCacheLinux.cpp', u'WebCore/platform/graphics/chromium/FontLinux.cpp', u'WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp', u'WebCore/platform/graphics/chromium/FontPlatformDataLinux.h', u'WebCore/platform/graphics/skia/FontCustomPlatformData.cpp', u'WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp']" exit_code: 1 WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:40: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75437 [details] A patch to make Chromium Linux render vertical text correctly change the place of #include <HarfbuzzSkia.h> to make the bot happy :-)
Created attachment 75444 [details] A patch to make Chromium Linux render vertical text correctly Change C cast to C++ cast. Hope I get all thing right this time
Comment on attachment 75437 [details] A patch to make Chromium Linux render vertical text correctly View in context: https://bugs.webkit.org/attachment.cgi?id=75437&action=review You need AGL's or evmar's review here. But testing is the major failing of this patch. Please clarify which tests cover this and ideally include the updated results. > WebCore/ChangeLog:8 > + No new tests. (OOPS!) This will cause the cq to fail. You should mention which tests this affects. > WebCore/platform/graphics/chromium/FontLinux.cpp:106 > + pos2[i].set(x + SkFloatToScalar(adv[i].width()), y); > + pos3[i].set(x + SkFloatToScalar(adv[i].width()), y-SkFloatToScalar(adv[i].width())); Seems we could use a local here to save about 50 chars of typing. :) > WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:68 > + if (hbFace->gsub) { > + HB_Buffer buffer; Either early return or using a helper function for this big clause would make this clearer. > WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:81 > + // FIXME : how/what to do This isn't a very helpful fixme (nor is it a complete sentence with capital and period).
Attachment 75437 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6732021
Attachment 75444 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6791016
Created attachment 75471 [details] A patch to make Chromium Linux render vertical text correctly Changed according to Eric Seidel's comments.
Created attachment 76016 [details] An initial patch to make Chromium Linux render vertical text correctly Make vertical position variables more descriptive
Comment on attachment 76016 [details] An initial patch to make Chromium Linux render vertical text correctly View in context: https://bugs.webkit.org/attachment.cgi?id=76016&action=review style comments. > WebCore/platform/graphics/chromium/FontPlatformDataLinux.cpp:83 > +#if 0 Unused code should be removed. > WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:65 > + if (!error) Should add { } because the block has two physical lines. > WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:70 > + } else > + // if there is no GSUB table, treat it as not covered > + error = 0Xffff; ditto.
Created attachment 76018 [details] an initial patch to make Chromium Linux render vertical text correctly same as 76016, removed unused code and add {}s
Comment on attachment 76018 [details] an initial patch to make Chromium Linux render vertical text correctly View in context: https://bugs.webkit.org/attachment.cgi?id=76018&action=review > WebCore/platform/graphics/chromium/FontLinux.cpp:163 > + canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint); will it be better to have another function instead of duplicating the above code?
Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly?
(In reply to comment #14) > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly? work for me:-) See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too
(In reply to comment #13) > (From update of attachment 76018 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76018&action=review > > > WebCore/platform/graphics/chromium/FontLinux.cpp:163 > > + canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint); > > will it be better to have another function instead of duplicating the above code? Yes, definitely I can do that. But that will pass some parameters to the function and create some overhead. Is it the supposed to be the right way?
Created attachment 76170 [details] Ruby text is sometimes missing I built chromium-linux with this patch and tried it out. Basically it works pretty well (good job!). But I've found a few rough edges: * When you scroll the document (horizontally), ruby text is sometimes incomplete or missing. See the attached screenshot. * If you move your mouse over the text, many console messages like the following will appear: >[24529:24529:2205928082:ERROR:webkit/glue/webcursor_gtk.cc(132)] Not implemented reached in int WebCursor::GetCursorType() const (I don't know this message is related to this patch though)
(In reply to comment #15) > (In reply to comment #14) > > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly? > > work for me:-) > See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too Cool! However, the positions of "、" and "。" look incorrect. They should be aligned to the right side of other characters. See http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/blockflow/Kusa-Makura-background-canvas-expected.png Is it expected?
(In reply to comment #18) > (In reply to comment #15) > > (In reply to comment #14) > > > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly? > > > > work for me:-) > > See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too > > Cool! > > However, the positions of "、" and "。" look incorrect. They should be aligned to the right side of other characters. See http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/blockflow/Kusa-Makura-background-canvas-expected.png > Is it expected? Nope, that's because my fallback font don't have the GSUB/vert table, I think. Those punctuations are supposed be replaced by vertical presentation glyph by looking up the GSUB/vert table
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > Can someone try this patch locally and see if LayoutTests/fast/blockflow/Kusa-Makura-background-canvas.html is rendered correctly? > > > > > > work for me:-) > > > See http://www.flickr.com/photos/koansin/5248686548/, scrollbar works too > > > > Cool! > > > > However, the positions of "、" and "。" look incorrect. They should be aligned to the right side of other characters. See http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/fast/blockflow/Kusa-Makura-background-canvas-expected.png > > Is it expected? > > Nope, that's because my fallback font don't have the GSUB/vert table, I think. > Those punctuations are supposed be replaced by vertical presentation glyph > by looking up the GSUB/vert table Argh, wrong answer, what you mentioned is 100% font problem. I was talking about another problem, such as 「」『』 replacement, see http://www.flickr.com/photos/koansin/5248695650/
Created attachment 76172 [details] Em dash is not aligned correctly I've noticed another issue: "em dash" character should be located at the center of the text, but actually does not align correctly. I'd like to leave the decision when to fix any of bugs I have found up to the patch author and reviewers; It sounds okay for me to file bugs for now and fix them later.
(In reply to comment #17) > Created an attachment (id=76170) [details] > Ruby text is sometimes missing > > I built chromium-linux with this patch and tried it out. > > Basically it works pretty well (good job!). But I've found a few rough edges: > > * When you scroll the document (horizontally), ruby text is sometimes incomplete or missing. See the attached screenshot. > I cannot reproduce it :-( Current I rotate glyphs one by one because I cannot find anything in Skia similar to Cairo's font matrix. Maybe this caused the refresh problem > * If you move your mouse over the text, many console messages like the following will appear: > >[24529:24529:2205928082:ERROR:webkit/glue/webcursor_gtk.cc(132)] Not implemented reached in int WebCursor::GetCursorType() const > > (I don't know this message is related to this patch though) It's there before my patch, I think.
Created attachment 76340 [details] snapshot of Kusamakura with proper font body {font-family: "Hiragino Micho Pro";} as expected, the locations of punctuations, including comma and em-dash, look right
If no one objects, I'll set r+, commit the patch, and update pixel results.
Created attachment 76742 [details] an initial patch to make Chromium Linux render vertical text correctly same as 76018, avoid looking up GSUB table when all the glyphs is Font::isCJKIdeographs()
Comment on attachment 76742 [details] an initial patch to make Chromium Linux render vertical text correctly ok. I'll commit it.
Landed as http://trac.webkit.org/changeset/74232 with some minor modifications.
Rebaseline done by http://trac.webkit.org/changeset/74233 Koan-Sin Tan, thank you for making this change!
(In reply to comment #28) > Rebaseline done by http://trac.webkit.org/changeset/74233 > > Koan-Sin Tan, thank you for making this change! No problem. Thanks for your review. I'd like to see CSS Writing Mode works on my favoriate browser. This is initial work. Some work remains to be done. E.g., 1. the system fallback font: on my Ubutun, it's WenQuanYi Zen Hei, which doesn't display those test cases well. This may be trivial and easy work, but need some time to construct a table, maybe. 2. Non- CJK glyphs are not supposed to be rotated like CJK glyphs. This is not easy, because it seems there is no way to check if a glyph is a CJK glyph or not in Font::drawGlyphs() without modifying struct GlyphData. 3. Mongolian and some other scripts also use vertical writing.