Summary: | [Cocoa] Simplify some FontAttributes API tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
Component: | Tools / Tests | Assignee: | 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
Wenson Hsieh
2021-12-31 14:02:29 PST
Created attachment 448135 [details]
Depends on #234757
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 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. Created attachment 448159 [details]
For EWS
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]. |