Bug 79336 - [Chromium] Add HarfBuzzShaperBase class
Summary: [Chromium] Add HarfBuzzShaperBase class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks: 69826
  Show dependency tree / graph
 
Reported: 2012-02-22 23:36 PST by Kenichi Ishibashi
Modified: 2012-02-24 07:17 PST (History)
6 users (show)

See Also:

Attachments
Patch (29.63 KB, patch)
2012-02-22 23:52 PST, Kenichi Ishibashi 		no flags Details | Formatted Diff | Diff
Patch for landing (31.10 KB, patch)
2012-02-23 16:01 PST, Kenichi Ishibashi 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-02-22 23:36:40 PST 
I'd like add a base class of HarfBuzz text shaper to share the code between old HarfBuzz and HarfBuzz-ng, to address Evan's comment on https://bugs.webkit.org/show_bug.cgi?id=69826#c18.
Comment 1 Kenichi Ishibashi 2012-02-22 23:52:45 PST 
Created attachment 128414 [details]
Patch
Comment 2 Kenichi Ishibashi 2012-02-22 23:53:51 PST 
Hi Evan,

Could you please review the patch?
Comment 3 Evan Martin 2012-02-23 07:52:39 PST 
Looks great, thanks for refactoring!
Comment 4 Tony Chang 2012-02-23 10:29:38 PST 
Comment on attachment 128414 [details]
Patch

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

> Source/WebCore/WebCore.gypi:3472
> +            'platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp',
> +            'platform/graphics/harfbuzz/HarfBuzzShaperBase.h',

Can you also include the cpp file in PlatformBlackBerry.cmake?

> Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:303
> +        if (static_cast<unsigned>(logClustersIndex) < m_item.item.length && isWordEnd(m_item.item.pos + logClustersIndex))

Should logClustersIndex be unsigned or size_t?  That might remove some of the casts.
Comment 5 Kenichi Ishibashi 2012-02-23 16:01:26 PST 
Created attachment 128584 [details]
Patch for landing
Comment 6 Kenichi Ishibashi 2012-02-23 16:02:52 PST 
Comment on attachment 128414 [details]
Patch

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

Thank you for review Evan and Tony!

>> Source/WebCore/WebCore.gypi:3472
>> +            'platform/graphics/harfbuzz/HarfBuzzShaperBase.h',
> 
> Can you also include the cpp file in PlatformBlackBerry.cmake?

Done.

>> Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:303
>> +        if (static_cast<unsigned>(logClustersIndex) < m_item.item.length && isWordEnd(m_item.item.pos + logClustersIndex))
> 
> Should logClustersIndex be unsigned or size_t?  That might remove some of the casts.

Done.
Comment 7 WebKit Review Bot 2012-02-23 22:59:46 PST 
Comment on attachment 128584 [details]
Patch for landing

Clearing flags on attachment: 128584

Committed r108733: <http://trac.webkit.org/changeset/108733>
Comment 8 WebKit Review Bot 2012-02-23 22:59:51 PST 
All reviewed patches have been landed.  Closing bug.