[Cocoa] Prepare ComplexTextController for unit testing
Created attachment 299922 [details] Patch
Created attachment 299923 [details] Patch
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.
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."
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?
Comment on attachment 299923 [details] Patch Clearing flags on attachment: 299923 Committed r211294: <http://trac.webkit.org/changeset/211294>
All reviewed patches have been landed. Closing bug.
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
Reopening to attach new patch.
Created attachment 299964 [details] Follow-up patch
Committed r211308: <http://trac.webkit.org/changeset/211308>