Tidy up color usage in accessibility code
Created attachment 404085 [details] Patch
Comment on attachment 404085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404085&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1997 > + SRGBA<uint8_t> result { 0, 0, 0, 0 }; I don’t see why this function benefits from a local variable. But if we did keep it, is there no "tidier" way to write this? Related: Is there a significant downside to having a structure like SRGBA be initialized by default? I would prefer one of these: SRGBA<uint8_t> result = Color::transparentBlack; auto result = Color::transparentBlack; SRGBA<uint8_t> result = { }; SRGBA<uint8_t> result { }; SRGBA<uint8_t> result; // <initialized to transparent black because we change SRGBA to just do that> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2001 > + return result; I propose one of these: return Color::transparentBlack; return { }; > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2004 > + return result; Ditto. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2006 > + result = downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBALossy<uint8_t>(); I propose: return downcast ... > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2009 > + return result; I propose one of these: return Color::transparentBlack; return { }; But also changing structure a bit perhaps.
Created attachment 404102 [details] Patch
Comment on attachment 404102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404102&action=review My comments on the previous patch still apply to this one. > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1425 > + auto color = self.axBackingObject->colorValue(); Build failures seems to be because this is missing the call to convertToComponentFloats.
Created attachment 404106 [details] Patch
Created attachment 404107 [details] Patch
(In reply to Darin Adler from comment #2) > Comment on attachment 404085 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404085&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1997 > > + SRGBA<uint8_t> result { 0, 0, 0, 0 }; > > I don’t see why this function benefits from a local variable. > > But if we did keep it, is there no "tidier" way to write this? Related: Is > there a significant downside to having a structure like SRGBA be initialized > by default? I would prefer one of these: > > SRGBA<uint8_t> result = Color::transparentBlack; > auto result = Color::transparentBlack; > SRGBA<uint8_t> result = { }; > SRGBA<uint8_t> result { }; > SRGBA<uint8_t> result; // <initialized to transparent black because we > change SRGBA to just do that> > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2001 > > + return result; > > I propose one of these: > > return Color::transparentBlack; > return { }; > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2004 > > + return result; > > Ditto. > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2006 > > + result = downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBALossy<uint8_t>(); > > I propose: > > return downcast ... > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2009 > > + return result; > > I propose one of these: > > return Color::transparentBlack; > return { }; > > But also changing structure a bit perhaps. Reworked it to remove the local variable and use early returns of Color::transparent;
Created attachment 404109 [details] Patch
Committed r264284: <https://trac.webkit.org/changeset/264284> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404109 [details].
<rdar://problem/65440513>