Attachment 69449[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 69600[details]
[build test] fixed a style error and non-Mac platform errors
[build test] I'm checking if my changes build on non-Mac platforms.
Thanks for the review. I'll fix them except following one.
> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:81
....
> Instead of making this change, you should change FontPlatformData::ctFont() to return a font with kCTFontOrientationAttribute set to kCTFontVerticalOrientation.
>
Apparently CTFont's CTFontGetGlyphsForCharacters() doesn't retrieve vertical variant glyphs even the font has kCTFontVerticalOrientation attribute, probably because glyph swapping for vertical layout is handled in CoreText's line layout engine, not at CTFont's level. I checked the source of CTFontGetGlyphsForCharacters() and confirmed it just directly consults font's cmap.
Can I leave this part as is?
...and a CTLine created from CTFont+kCTFontOrientationAttribute but without kCTVerticalFormsAttributeName doesn't do glyph swapping also. As far as I tested, we need to explicitly specify kCTVerticalFormsAttributeName in a CTLine to make glyph swapping to be happened.
Created attachment 70827[details]
[another build test] Incorporated mitz's feedback. Submitted to test builds on non-Mac platforms.
Incorporated mitz's feedback except the one for SimpleFontDataCoreText.cpp. Apparently making CTFont with kCTFontOrientationAttribute=kCTFontVerticalOrientation doesn't help to simplify the code. Please see my comments above...
Comment on attachment 70837[details]
[another build test] Fixed another build error on non-Mac platforms...
View in context: https://bugs.webkit.org/attachment.cgi?id=70837&action=review> WebCore/ChangeLog:11
> + Doesn't affect any tests yet.
Can you add tests that are affected by this? If not, why not?
> WebCore/ChangeLog:104
> +
Please add function-level comments about the changes.
> WebCore/loader/CachedFont.cpp:120
> +FontPlatformData CachedFont::platformDataFromCustomData(float size, bool bold, bool italic, FontOrientation fontOrientation, FontRenderingMode renderingMode)
Please rename the parameter to “orientation”. There’s no need for “font” in this context.
> WebCore/loader/CachedFont.h:30
> +#include "FontDescription.h"
Please define FontOrientation in its own header so that you don’t have to include FontDescription in this header.
> WebCore/loader/CachedFont.h:66
> + FontPlatformData platformDataFromCustomData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Please drop the parameter name “fontOrientation” altogether. WebKit style is not to name parameters in declarations when there is no ambiguity.
> WebCore/platform/graphics/FontCache.cpp:58
> + bool isPrinterFont = false, FontRenderingMode renderingMode = NormalRenderingMode, FontOrientation fontOrientation = Horizontal)
Again, please drop “font” from the parameter name.
> WebCore/platform/graphics/FontCache.cpp:85
> + FontOrientation m_fontOrientation;
And rename this to m_orientation.
> WebCore/platform/graphics/FontDescription.h:100
> + FontOrientation fontOrientation() const { return m_fontOrientation; }
Rename this accessor to orientation().
> WebCore/platform/graphics/FontDescription.h:120
> + void setFontOrientation(FontOrientation fontOrientation) { m_fontOrientation = fontOrientation; }
And this one to setOrientation().
> WebCore/platform/graphics/FontDescription.h:129
> + FontOrientation m_fontOrientation; // Font's orientation. Horizontal or vertical.
I don’t think the comment is necessary.
> WebCore/platform/graphics/cairo/FontCustomPlatformData.h:25
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/cairo/FontCustomPlatformData.h:42
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Don’t name the parameter.
> WebCore/platform/graphics/cairo/FontPlatformDataFreeType.h:58
> + FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);
Why is this change needed? You should omit the parameter name, if this is needed.
> WebCore/platform/graphics/cg/FontPlatformData.h:27
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/cg/FontPlatformData.h:53
> + FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);
Why is this change needed? If this is needed, you should omit the parameter name.
> WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:37
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:61
> + FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);
Why is this change needed? If this is needed, you should omit the parameter name.
> WebCore/platform/graphics/chromium/FontPlatformDataLinux.h:34
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/chromium/FontPlatformDataLinux.h:77
> + FontPlatformData(float textSize, bool fakeBold, bool fakeItalic, FontOrientation fontOrientation = Horizontal)
Why is this change needed? If this is needed, you should omit the parameter name.
> WebCore/platform/graphics/cocoa/FontPlatformData.h:27
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/cocoa/FontPlatformData.h:62
> + FontPlatformData(float size, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation = Horizontal)
I’m not sure it’s a good idea to have a default value for the orientation.
> WebCore/platform/graphics/cocoa/FontPlatformData.h:76
> + FontPlatformData(NSFont *nsFont, bool syntheticBold = false, bool syntheticOblique = false, FontOrientation fontOrientation = Horizontal);
Please omit the parameter name.
> WebCore/platform/graphics/cocoa/FontPlatformData.h:78
> + FontPlatformData(CGFontRef cgFont, ATSUFontID fontID, float size, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation = Horizontal)
And rename it to “orientation” here.
> WebCore/platform/graphics/cocoa/FontPlatformData.h:100
> + FontOrientation fontOrientation() const { return m_fontOrientation; }
Please rename this accessor to orientation().
> WebCore/platform/graphics/cocoa/FontPlatformData.h:104
> + FontOrientation m_fontOrientation;
And this member variable to m_orientation.
> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:47
> +FontPlatformData::FontPlatformData(NSFont *nsFont, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation)
Please rename the parameter to “orientation”.
> WebCore/platform/graphics/gtk/FontPlatformDataPango.h:58
> + FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);
Why is this change needed? If this is needed, you should omit the parameter name.
> WebCore/platform/graphics/haiku/FontCustomPlatformData.h:24
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/haiku/FontCustomPlatformData.h:41
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Omit the parameter name.
> WebCore/platform/graphics/haiku/FontPlatformData.h:48
> + FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);
Why is this change needed? If this is needed, you should omit the parameter name.
> WebCore/platform/graphics/mac/FontCustomPlatformData.cpp:41
> +FontPlatformData FontCustomPlatformData::fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation, FontRenderingMode)
Rename the parameter to “orientation”.
> WebCore/platform/graphics/mac/FontCustomPlatformData.h:24
> +#include "FontDescription.h"
Use a separate header for the FontOrientation enum.
> WebCore/platform/graphics/mac/FontCustomPlatformData.h:45
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Omit the parameter name.
> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:77
> + RetainPtr<CGFontRef> runCGFont(AdoptCF, CTFontCopyGraphicsFont(runFont, 0));
> + if (CFEqual(fontData->platformData().cgFont(), runCGFont.get())) {
Is there a reason to compare CGFonts and not CTFonts here?
> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:83
> + Vector<CGGlyph, 512> glyphs(static_cast<size_t>(glyphCount));
> + CTRunGetGlyphs(ctRun, CFRangeMake(0, 0), glyphs.data());
> + Vector<CFIndex, 512> stringIndices(static_cast<size_t>(glyphCount));
> + CTRunGetStringIndices(ctRun, CFRangeMake(0, 0), stringIndices.data());
This forces copying, but CTRunGetGlyphsPtr() and CTRunGetStringIndicesPtr() don’t. Of course, those aren’t guaranteed to work, so you need try them first, then use the above as a fallback if they fail.
> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:46
> + unsigned key = (typesettingFeatures + 1) | (platformData().fontOrientation() << 8);
There’s no need to include the orientation in the key, because it’s constant for a given SimpleFontData instance.
> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:52
> + bool allowLigatures = (!ignoreDefaultFeatures && platformData().allowsLigatures()) || (typesettingFeatures & Ligatures);
You can just change platformData().allowsLigatures() to return false for the vertical orientation, and avoid adding the ignoreDefaultFeatures parameter to this method.
> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:70
> + static const void* keysWithKerningDisabled[] = { kCTFontAttributeName,
> + kCTKernAttributeName,
> + kCTLigatureAttributeName,
> + kCTVerticalFormsAttributeName };
> + const void* valuesWithKerningDisabled[] = { platformData().ctFont(),
> + kerningAdjustment,
> + allowLigatures ? ligaturesAllowed : ligaturesNotAllowed,
> + (platformData().fontOrientation() == Vertical) ? kCFBooleanTrue : kCFBooleanFalse };
WebKit style is to not align things like this. Please just add the key and the value at the end.
> WebCore/platform/graphics/mac/SimpleFontDataMac.mm:421
> + boundingBox = CTFontGetBoundingRectsForGlyphs(m_platformData.ctFont(),
> + m_platformData.fontOrientation() == Vertical ? kCTFontVerticalOrientation : kCTFontHorizontalOrientation, &glyph, NULL, 1);
WebKit style is to use 0, not NULL.
> WebCore/platform/graphics/qt/FontCustomPlatformData.h:25
> +#include "FontDescription.h"
Use a separate header for FontOrientation.
> WebCore/platform/graphics/qt/FontCustomPlatformData.h:41
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Omit the parameter name.
> WebCore/platform/graphics/qt/FontPlatformData.h:66
> + FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);
I don’t think this change is needed. If it is, omit the parameter name.
> WebCore/platform/graphics/skia/FontCustomPlatformData.h:35
> +#include "FontDescription.h"
Use a separate header for FontOrientation.
> WebCore/platform/graphics/skia/FontCustomPlatformData.h:66
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal,
Omit the parameter name.
> WebCore/platform/graphics/win/FontCustomPlatformData.h:24
> +#include "FontDescription.h"
Use separate header.
> WebCore/platform/graphics/win/FontCustomPlatformData.h:46
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Omit parameter name.
> WebCore/platform/graphics/win/FontCustomPlatformDataCairo.h:43
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);
Don’t this this is needed. Omit parameter name.
> WebCore/platform/graphics/win/FontPlatformDataCairoWin.h:63
> + FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal);
Ditto.
> WebCore/platform/graphics/wince/FontPlatformData.h:45
> + FontPlatformData(float size, bool bold, bool oblique, FontOrientation fontOrientation = Horizontal);
Ditto.
> WebCore/platform/graphics/wx/FontCustomPlatformData.h:24
> +#include "FontDescription.h"
Use different header.
> WebCore/platform/graphics/wx/FontCustomPlatformData.h:41
> + FontPlatformData fontPlatformData(int size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal, FontRenderingMode = NormalRenderingMode);
Omit parameter name.
> WebCore/platform/graphics/wx/FontPlatformData.h:94
> + FontPlatformData(float size, bool bold, bool italic, FontOrientation fontOrientation = Horizontal)
Don’t think you need to change this. If you do, omit the parameter name.
Thank you very much for your feedback again. I've fixed them except following two.
> > WebCore/platform/graphics/cocoa/FontPlatformData.h:62
> > + FontPlatformData(float size, bool syntheticBold, bool syntheticOblique, FontOrientation fontOrientation = Horizontal)
>
> I’m not sure it’s a good idea to have a default value for the orientation.
>
Otherwise CachedFont.cpp:124, FontCache.cpp:241, SimpleFontData.cpp:67 cause compile errors on FontPlatformData creation as now we don't add "orientation" to non-Mac platform. Or, do you think it is better to add another constructor that takes only a size, a bold flag, and an oblique flag?
> > WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:77
> > + RetainPtr<CGFontRef> runCGFont(AdoptCF, CTFontCopyGraphicsFont(runFont, 0));
> > + if (CFEqual(fontData->platformData().cgFont(), runCGFont.get())) {
>
> Is there a reason to compare CGFonts and not CTFonts here?
>
CFEqual for CTFont considers font's CTFontDescriptor that contains various attributes on top of the font data itself. I wanted to avoid possible small discrepancies CT's layout engine might added.
Comment on attachment 71023[details]
Incorporated additional feedback from mitz.
View in context: https://bugs.webkit.org/attachment.cgi?id=71023&action=review
The patch looks good! My comments are all pretty minor.
> WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:175
> + return (m_orientation == Horizontal) && (![[m_font coveredCharacterSet] characterIsMember:'a']);
Please remove the extraneous parentheses.
> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:68
> + static Vector<CGGlyph, 512> *glyphVector = 0;
> + static Vector<CFIndex, 512> *indexVector = 0;
WebKit style is to put the space on the other side of the star. However, I suggest allocating these on the stack unconditionally instead of using pointers and heap allocation.
> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:70
> + for (CFIndex r = 0; r < runCount && !done ; r++) {
WebKit style is to write “++r” in for loops. It doesn’t make a difference for a type like CFIndex, but it’s better for some iterator types, so we do this for consistency.
> WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:79
> + if (CFEqual(fontData->platformData().cgFont(), runCGFont.get())) { // Use CGFont here as CFEqual for CTFont counts all attributes for font
Please put the comment on its own line and terminate it with a period!
> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:65
> + ? ligaturesAllowed : ligaturesNotAllowed, (platformData().orientation() == Vertical) ? kCFBooleanTrue : kCFBooleanFalse };
Parentheses unneeded here.
> WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:72
> + const void* valuesWithKerningEnabled[] = { platformData().ctFont(), allowLigatures ? ligaturesAllowed : ligaturesNotAllowed, (platformData().orientation() == Vertical) ? kCFBooleanTrue : kCFBooleanFalse };
And here.
> LayoutTests/fast/text/international/vertical-text-metrics-test.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
Is there any reason to use this legacy doctype for this test? If the test works with the HTML5 doctype or without a doctype, please change.
> LayoutTests/fast/text/international/vertical-text-metrics-test.html:17
> + if (window.layoutTestController)
> + layoutTestController.dumpAsText();
It’s good to have this dumps-as-text test for the metrics. Can you also make a pixel test for the glyph variants?
> LayoutTests/fast/text/international/vertical-text-metrics-test.html:23
> + var elem = pElems[i];
> + print("width=" + elem.offsetWidth);
Tabs here will prevent this patch from being landed.
> LayoutTests/fast/text/international/vertical-text-metrics-test.html:49
> +<span id="horizontal_TB">abãããã</span><br>
> +<span id="horizontal_BT">abãããã</span><br>
> +<span id="vertical_RL">abãããã</span><br>
> +<span id="vertical_LR">abãããã</span><br>
Is this testing only the simple text code path? It would be good to test the complex text code path as well. One way to force things down that path is to use “text-rendering: optimizelegibility”.
Created attachment 71131[details]
Made some more changes pointed by mitz
Added some more changes pointed by mitz.
One exception is for the comment below;
> > LayoutTests/fast/text/international/vertical-text-metrics-test.html:17
> > + if (window.layoutTestController)
> > + layoutTestController.dumpAsText();
>
> It’s good to have this dumps-as-text test for the metrics. Can you also make a pixel test for the glyph variants?
We cannot get correct pixel images at this time, because laying out the text box for vertical writing mode (e.g., flowing text vertically and stacking text boxes tb-rl) hasn't been finished yet...
(In reply to comment #23)
> Created an attachment (id=71131) [details]
> Made some more changes pointed by mitz
>
> Added some more changes pointed by mitz.
> One exception is for the comment below;
> > > LayoutTests/fast/text/international/vertical-text-metrics-test.html:17
> > > + if (window.layoutTestController)
> > > + layoutTestController.dumpAsText();
> >
> > It’s good to have this dumps-as-text test for the metrics. Can you also make a pixel test for the glyph variants?
>
> We cannot get correct pixel images at this time, because laying out the text box for vertical writing mode (e.g., flowing text vertically and stacking text boxes tb-rl) hasn't been finished yet…
The images need not be “correct”, they are useful if they can help to detect unintended changes in behavior, which could be regressions. So I think a pixel test would be quite valuable.
Comment on attachment 71131[details]
Made some more changes pointed by mitz
Rejecting patch 71131 from commit-queue.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
, Tests=304, 1 wallclock secs ( 0.63 cusr + 0.13 csys = 0.76 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/abarth/git/webkit-queue/LayoutTests
Testing 21599 test cases.
fast/text/international/vertical-text-metrics-test.html -> failed
Exiting early after 1 failures. 15784 tests run.
212.08s total testing time
15783 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output
Full output: http://queues.webkit.org/results/4578001
Comment on attachment 71270[details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)
Rejecting patch 71270 from commit-queue.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71270]" exit_code: 2
Cleaning working directory
Updating working directory
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Full output: http://queues.webkit.org/results/4520022
Comment on attachment 71270[details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)
I think this was a false rejection from a run-away commit-node.
Comment on attachment 71270[details]
Added pixel test. Changed tests to explicitly specify a font family. (Resubmitting for build error...)
Rejecting patch 71270 from commit-queue.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
l.
Files=14, Tests=304, 1 wallclock secs ( 0.74 cusr + 0.18 csys = 0.92 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21605 test cases.
fast/text/international/vertical-text-glyph-test.html -> failed
Exiting early after 1 failures. 15787 tests run.
239.82s total testing time
15786 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output
Full output: http://queues.webkit.org/results/4613003
r70250 changed some #if guards from USE(CORE_TEXT) to PLATFORM(MAC):
http://trac.webkit.org/changeset/70225/trunk/WebCore/platform/graphics/SimpleFontData.h
however PLATFORM(MAC) is only set for the Apple Mac port and so this broke the Chromium mac compile. Was there a specific reason to move away from the USE(CORE_TEXT) macro? Could these be changed from PLATFORM(MAC) to USE(CG) or something else that's also true for Chromium mac? thanks.
2010-10-01 04:40 PDT, Takumi Takano
2010-10-03 22:40 PDT, Takumi Takano
2010-10-13 00:58 PDT, Takumi Takano
2010-10-14 20:51 PDT, Takumi Takano
2010-10-14 21:29 PDT, Takumi Takano
2010-10-14 22:01 PDT, Takumi Takano
2010-10-15 00:59 PDT, Takumi Takano
2010-10-18 05:02 PDT, Takumi Takano
2010-10-18 22:11 PDT, Takumi Takano
2010-10-20 03:23 PDT, Takumi Takano
2010-10-20 03:51 PDT, Takumi Takano
commit-queue: commit-queue-
2010-10-20 16:53 PDT, mitz