Bug 97277 – [Chromium] Use OpenTypeVerticalData on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kenichi Ishibashi]

- To remove dependency on harfbuzz of GlyphPageTreeNodeSkia.
- Results of some vertical writing tests will improve.

[Kenichi Ishibashi]

Created attachment 165024 [details]
Patch

[Kenichi Ishibashi]

Hi Kent-san,

Could you take a look?

[Kent Tamura]

Comment on attachment 165024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165024&action=review

> Source/WebCore/ChangeLog:17
> +        (SimpleFontData): Moved declaration of m_verticalData.

Why?

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:53
> +#if ENABLE(OPENTYPE_VERTICAL)
> +class OpenTypeVerticalData;
> +#endif

nit: I don't think we need to warp a class declaration with an ENABLE flag.

> LayoutTests/ChangeLog:10
> +        * platform/chromium-linux/fast/dynamic/text-combine-expected.png:

The test results looks good.

[Kent Tamura]

(In reply to comment #3)
> > Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:53
> > +#if ENABLE(OPENTYPE_VERTICAL)
> > +class OpenTypeVerticalData;
> > +#endif
> 
> nit: I don't think we need to warp a class declaration with an ENABLE flag.

warp -> wrap

[Kenichi Ishibashi]

Created attachment 165026 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 165024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165024&action=review

Thank you for review!

>> Source/WebCore/ChangeLog:17
>> +        (SimpleFontData): Moved declaration of m_verticalData.
> 
> Why?

To suppress warnings (treated as errors) like:

../../third_party/WebKit/Source/WebCore/platform/graphics/SimpleFontData.h:241: error: 'WebCore::SimpleFontData::m_isBrokenIdeographFallback' will be initialized after
../../third_party/WebKit/Source/WebCore/platform/graphics/SimpleFontData.h:233: error:   'const WebCore::OpenTypeVerticalData* WebCore::SimpleFontData::m_verticalData'

>> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:53
>> +#endif
> 
> nit: I don't think we need to warp a class declaration with an ENABLE flag.

Done.

[Kenichi Ishibashi]

Created attachment 165036 [details]
More rebaselines

[Tony Chang]

Comment on attachment 165036 [details]
More rebaselines

View in context: https://bugs.webkit.org/attachment.cgi?id=165036&action=review

Quotation marks look much better!

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:359
> +    uint32_t tag = (table >> 24) | ((table >> 8) & 0xff00) | ((table & 0xff00) << 8) | ((table & 0xff) << 24);

Should this be a SkFontTableTag?  I would move this into a function to help document this code.  Something like, reverseByteOrder.

> Source/WebKit/chromium/features.gypi:205
> -      ['OS=="win"', {
> +      ['OS!="mac"', {

Do you mean to enable this on ios and android?  You could do 'OS=="win" or use_x11==1'.

[Kenichi Ishibashi]

Created attachment 165216 [details]
Patch for landing

[Kenichi Ishibashi]

Comment on attachment 165036 [details]
More rebaselines

View in context: https://bugs.webkit.org/attachment.cgi?id=165036&action=review

Thank you for review!

>> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:359
>> +    uint32_t tag = (table >> 24) | ((table >> 8) & 0xff00) | ((table & 0xff00) << 8) | ((table & 0xff) << 24);
> 
> Should this be a SkFontTableTag?  I would move this into a function to help document this code.  Something like, reverseByteOrder.

Done.

>> Source/WebKit/chromium/features.gypi:205
>> +      ['OS!="mac"', {
> 
> Do you mean to enable this on ios and android?  You could do 'OS=="win" or use_x11==1'.

Done.

[WebKit Review Bot]

Comment on attachment 165216 [details]
Patch for landing

Clearing flags on attachment: 165216

Committed r129273: <http://trac.webkit.org/changeset/129273>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.