Bug 214226 – Tidy up color usage in accessibility code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Tidy up color usage in accessibility code

[Sam Weinig]

Created attachment 404085 [details]
Patch

[Darin Adler]

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.

[Sam Weinig]

Created attachment 404102 [details]
Patch

[Darin Adler]

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.

[Sam Weinig]

Created attachment 404106 [details]
Patch

[Sam Weinig]

Created attachment 404107 [details]
Patch

[Sam Weinig]

(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;

[Sam Weinig]

Created attachment 404109 [details]
Patch

[EWS]

Committed r264284: <https://trac.webkit.org/changeset/264284>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404109 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65440513>