Bug 167493 – [Cocoa] Prepare ComplexTextController for unit testing

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Prepare ComplexTextController for unit testing

[Myles C. Maxfield]

Created attachment 299922 [details]
Patch

[Myles C. Maxfield]

Created attachment 299923 [details]
Patch

[Dean Jackson]

Comment on attachment 299923 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:111
> +    Vector<CGSize> advances = { CGSizeZero, CGSizeMake(21.640625, 0.0), CGSizeMake(42.3046875, 0.0), CGSizeMake(55.8984375, 0.0), CGSizeMake(22.34375, 0.0) };
> +    Vector<CGPoint> origins = { CGPointMake(-15.15625, 18.046875), CGPointZero, CGPointZero, CGPointZero, CGPointZero };
> +#else
> +    Vector<CGSize> advances = { CGSizeMake(15.15625, -18.046875), CGSizeMake(21.640625, 0.0), CGSizeMake(42.3046875, 0.0), CGSizeMake(55.8984375, 0.0), CGSizeMake(22.34375, 0.0) };

Where do all these numbers come from? It's probably worth leaving a comment explaining them.

[Myles C. Maxfield]

Comment on attachment 299923 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:111
>> +    Vector<CGSize> advances = { CGSizeMake(15.15625, -18.046875), CGSizeMake(21.640625, 0.0), CGSizeMake(42.3046875, 0.0), CGSizeMake(55.8984375, 0.0), CGSizeMake(22.34375, 0.0) };
> 
> Where do all these numbers come from? It's probably worth leaving a comment explaining them.

From the ChangeLog: "These metrics
 16        have been gathered from real-world uses; however, a layout test is not appropriate
 17        because the fonts which produced these metrics are not licensed appropriately."

[Simon Fraser (smfr)]

Comment on attachment 299923 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        No new tests because there is no behavior change.

But you're adding an API test.

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:94
> +        static Ref<ComplexTextRun> createForTesting(Vector<CGSize> advances, Vector<CGPoint> origins, Vector<CGGlyph> glyphs, Vector<CFIndex> stringIndices, CGSize initialAdvance, const Font& font, const UChar* characters, unsigned stringLocation, size_t stringLength, CFRange runRange, bool ltr)

Do you really want to copy all those vectors by value?

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:186
> +ComplexTextController::ComplexTextRun::ComplexTextRun(Vector<CGSize> advances, Vector<CGPoint> origins, Vector<CGGlyph> glyphs, Vector<CFIndex> stringIndices, CGSize initialAdvance, const Font& font, const UChar* characters, unsigned stringLocation, size_t stringLength, CFRange runRange, bool ltr)

So much copying.

> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:54
> +    font.update(nullptr);

Maybe update should have a default argument.

> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:57
> +#if USE_LAYOUT_SPECIFIC_ADVANCES

Are you sure that TestWebKitAPI gets the right defines?

> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:70
> +    Ref<ComplexTextController::ComplexTextRun> run1 = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(21.875, 0) }, { CGPointZero }, { 5 }, { 5 }, CGSizeZero, font.primaryFont(), characters, 0, charactersLength, CFRangeMake(5, 1), false);

auto?

> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:71
> +    Ref<ComplexTextController::ComplexTextRun> run2 = ComplexTextController::ComplexTextRun::createForTesting(advances, origins, { 193, 377, 447, 431, 458 }, { 4, 3, 2, 1, 0 }, initialAdvance, font.primaryFont(), characters, 0, charactersLength, CFRangeMake(0, 5), false);

auto?

[WebKit Commit Bot]

Comment on attachment 299923 [details]
Patch

Clearing flags on attachment: 299923

Committed r211294: <http://trac.webkit.org/changeset/211294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

Comment on attachment 299923 [details]
Patch

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

>> Source/WebCore/ChangeLog:26
>> +        No new tests because there is no behavior change.
> 
> But you're adding an API test.

They are disabled until https://bugs.webkit.org/show_bug.cgi?id=166013

>> Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:57
>> +#if USE_LAYOUT_SPECIFIC_ADVANCES
> 
> Are you sure that TestWebKitAPI gets the right defines?

Yes, this is defined in ComplexTextController.h

[Myles C. Maxfield]

Reopening to attach new patch.

[Myles C. Maxfield]

Created attachment 299964 [details]
Follow-up patch

[Myles C. Maxfield]

Committed r211308: <http://trac.webkit.org/changeset/211308>