[Webkit-unassigned] [Bug 46973] Need to swap glyphs for vertical writing
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 15 11:45:52 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=46973
mitz at webkit.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #70837|review? |review-
Flag| |
--- Comment #19 from mitz at webkit.org 2010-10-15 11:45:51 PST ---
(From update of attachment 70837)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list