[webkit-reviews] review denied: [Bug 34544] [Chromium] RenderTheme does not draw focus rings on SL for checkboxes, radio buttons : [Attachment 48143] Updated per review

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Avi Drissman <avi at drissman.com>'s
request for review:
Bug 34544: [Chromium] RenderTheme does not draw focus rings on SL for
checkboxes, radio buttons
https://bugs.webkit.org/show_bug.cgi?id=34544

Attachment 48143: Updated per review
https://bugs.webkit.org/attachment.cgi?id=48143&action=review

------- Additional Comments from David Levin <levin at chromium.org>
I don't feel comfortable reviewing Objective-C but I can help with general
stuff to make it easier for an Objective-C reviewer.


> Index: WebCore/platform/chromium/ThemeChromiumMac.mm
> @@ -47,6 +48,7 @@ using namespace std;
>  //	rendering.
>  // - In updateStates() the code to update the cells' inactive state.
>  // - In paintButton() the code to save/restore the window's default button
cell.
> +// - The Fix7604051 code and the calls to it.

How about:
// - A focus indication fix for radio buttons/checkboxes on Snow Leopard.
instead?


> +// --- START FIX FOR RADAR 7604051 ---

Is there a better name for the bug other than "RADAR 7604051" (like "START FIX
FOR radio button/checkbox focus indication on Snow Leopard.)


How about shortening the comment to this?

On Snow Leopard in the focus ring drawing code, it calls +[NSView focusView] to
get 
the currently focused view. The focus ring drawing code then calls a
rect-returning
method on that view, and uses that rect to clip.  If there is no focus view,
the rect
returned from that method is garbage which results in clipping out the
selection glow
entirely.

To fix this, do some quick swizzling to make sure that the focus ring code is
works
properly if a test of drawing the focus ring into contexts fails.

FIXME: After rdar://problem/7604051 is fixed on all supported platforms, remove
this code.


> + at interface TCMVisibleView : NSView
> +
> + at end
> +
> + at implementation TCMVisibleView
> +
> +- (struct CGRect)_focusRingVisibleRect {

Braces for functions start on the next line (many instances of this but I'm
only flagging the first).


> +namespace Fix7604051 {

Perhaps FocusIndicationFix 

> +    return (pixel == 0);

WebKit style is to avoid comparisons to 0. Use "return !pixel;" instead.