Bug 234770 - [Cocoa] Simplify some FontAttributes API tests
Summary: [Cocoa] Simplify some FontAttributes API tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 190120
  Show dependency treegraph
 
Reported: 2021-12-31 14:02 PST by Wenson Hsieh
Modified: 2022-01-01 13:05 PST (History)
6 users (show)

See Also:


Attachments
Depends on #234757 (16.67 KB, patch)
2021-12-31 15:13 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (16.85 KB, patch)
2022-01-01 12:26 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>