Bug 97277

Summary:

[Chromium] Use OpenTypeVerticalData on Linux

Product:

WebKit

Reporter:

Kenichi Ishibashi <bashi>

Component:

Platform

Assignee:

Kenichi Ishibashi <bashi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

kojii, senorblanco, tkent, tony, wangxianzhu, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
More rebaselines	none
Patch for landing	none

Kenichi Ishibashi

Reported 2012-09-20 17:46:09 PDT

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

Attachments
Patch (1.55 MB, patch) 2012-09-20 17:54 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch (1.55 MB, patch) 2012-09-20 18:11 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
More rebaselines (1.77 MB, patch) 2012-09-20 19:40 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Patch for landing (1.77 MB, patch) 2012-09-21 15:51 PDT, Kenichi Ishibashi	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Kenichi Ishibashi

Comment 1 2012-09-20 17:54:26 PDT

Created attachment 165024 [details] Patch

Kenichi Ishibashi

Comment 2 2012-09-20 17:57:50 PDT

Hi Kent-san, Could you take a look?

Kent Tamura

Comment 3 2012-09-20 18:03:09 PDT

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

Comment 4 2012-09-20 18:03:51 PDT

(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

Comment 5 2012-09-20 18:11:09 PDT

Created attachment 165026 [details] Patch

Kenichi Ishibashi

Comment 6 2012-09-20 18:11:42 PDT

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

Comment 7 2012-09-20 19:40:28 PDT

Created attachment 165036 [details] More rebaselines

Tony Chang

Comment 8 2012-09-21 11:57:01 PDT

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

Comment 9 2012-09-21 15:51:48 PDT

Created attachment 165216 [details] Patch for landing

Kenichi Ishibashi

Comment 10 2012-09-21 15:53:22 PDT

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 11 2012-09-21 16:36:09 PDT

Comment on attachment 165216 [details] Patch for landing Clearing flags on attachment: 165216 Committed r129273: <http://trac.webkit.org/changeset/129273>

WebKit Review Bot

Comment 12 2012-09-21 16:36:12 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.