[webkit-reviews] review denied: [Bug 26821] REGRESSION(r45285): focus rings are black on windows safari : [Attachment 32312] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alice Liu <alice.liu at apple.com>'s
request for review:
Bug 26821: REGRESSION(r45285): focus rings are black on windows safari
https://bugs.webkit.org/show_bug.cgi?id=26821

Attachment 32312: patch
https://bugs.webkit.org/attachment.cgi?id=32312&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good.

> +static Color customFocusRingColor;

This would fail on Mac (scripts would catch it around link time) because it
creates a constructor that runs at framework load time and a destructor that
runs at exit time. For an example of the correct idiom for this type of thing,
you could look at a function like keepAliveSet() in Frame.cpp that shows how to
use DEFINE_STATIC_LOCAL to avoid the exit-time destructor and a function to
avoid a load-time constructor.

> +void RenderTheme::setCustomFocusRingColor(Color c)

We'd typically use "const Color&" as the type for the argument to avoid copying
the color.

> +    virtual Color platformFocusRingColor() const { return Color(); }

I think platformFocusRingColor should probably be pure virtual instead of
returning a transparent color. Or it could return opaque black?

review- because of the load-time and exit-time issue