[webkit-reviews] review denied: [Bug 46973] Need to swap glyphs for vertical writing : [Attachment 70837] [another build test] Fixed another build error on non-Mac platforms...

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 15 11:45:51 PDT 2010 

mitz at webkit.org has denied Takumi Takano <takano1 at asia.apple.com>'s request for
review:
Bug 46973: Need to swap glyphs for vertical writing
https://bugs.webkit.org/show_bug.cgi?id=46973

Attachment 70837: [another build test] Fixed another build error on non-Mac
platforms...
https://bugs.webkit.org/attachment.cgi?id=70837&action=review

------- Additional Comments from mitz at webkit.org
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.

More information about the webkit-reviews mailing list