Bug 234770

Summary: [Cocoa] Simplify some FontAttributes API tests
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: Tools / TestsAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, darin, hi, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 190120    
Attachments:
Description Flags
Depends on #234757
none
For EWS none

Description Wenson Hsieh 2021-12-31 14:02:29 PST
• Instead of defining PlatformColor/PlatformFont, use CocoaColor/CocoaFont from WebCore.
• Remove the Nullity flag in ShadowExpectation and ColorExpectation (and simply std::optional instead).
Comment 1 Wenson Hsieh 2021-12-31 15:13:21 PST
Created attachment 448135 [details]
Depends on #234757
Comment 2 Darin Adler 2021-12-31 21:40:13 PST
Comment on attachment 448135 [details]
Depends on #234757

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:106
> +    CGFloat red;
> +    CGFloat green;
> +    CGFloat blue;
> +    CGFloat alpha;

I suggest these still be initialized to zero, even though this is a simple structure. No reason to let them be uninitialized.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:111
> +    CGFloat opacity;
> +    CGFloat blurRadius;

Ditto. Same for the other structures below.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:119
> +static void checkColor(WebCore::CocoaColor *color, std::optional<ColorExpectation>&& expectation)

No reason to use an rvalue reference for the argument. It’s not important to move something full of a lot of scalars. I’d just use a value, not a reference at all.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:139
> +static void checkShadow(NSShadow *shadow, std::optional<ShadowExpectation>&& expectation)

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:154
> +static void checkFont(WebCore::CocoaFont *font, FontExpectation&& expectation)

Ditto.
Comment 3 Wenson Hsieh 2022-01-01 11:05:52 PST
Comment on attachment 448135 [details]
Depends on #234757

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

Thanks for the review!

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:106
>> +    CGFloat alpha;
> 
> I suggest these still be initialized to zero, even though this is a simple structure. No reason to let them be uninitialized.

Makes sense — added initial values to ColorExpectation, ShadowExpectation, FontExpectation and ParagraphStyleExpectation (specifically the NSTextAlignment member).

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/FontAttributes.mm:119
>> +static void checkColor(WebCore::CocoaColor *color, std::optional<ColorExpectation>&& expectation)
> 
> No reason to use an rvalue reference for the argument. It’s not important to move something full of a lot of scalars. I’d just use a value, not a reference at all.

Sounds good! Changed this (and the other places below) to pass by value instead.
Comment 4 Wenson Hsieh 2022-01-01 12:26:58 PST
Created attachment 448159 [details]
For EWS
Comment 5 EWS 2022-01-01 13:04:30 PST
Committed r287503 (245638@main): <https://commits.webkit.org/245638@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448159 [details].
Comment 6 Radar WebKit Bug Importer 2022-01-01 13:05:22 PST
<rdar://problem/87041744>