Bug 34544 – [Chromium] RenderTheme does not draw focus rings on SL for checkboxes, radio buttons

• Component: Platform
• Status: RESOLVED FIXED

[Avi Drissman]

Downstream bug at http://code.google.com/p/chromium/issues/detail?id=27493 .

Explanation and fix in patch, to come.

[Avi Drissman]

Created attachment 48071 [details]
Patch to fix.

[Timothy Hatcher]

Comment on attachment 48071 [details]
Patch to fix.

I don't know why I was CCed on this bug. But since I was, I looked. And I couldn't get past the wall of text comment and the poor class name, namespaces and methods that have 7604051 littered all over. Be more creative and name these things better.

[Eric Seidel (no email)]

I CC'd you on it because you speak Obj-C much better than I (and much of the WebKit project) do these days.

[Avi Drissman]

Created attachment 48143 [details]
Updated per review

[Avi Drissman]

(In reply to comment #2)
> And I
> couldn't get past the wall of text comment and the poor class name, namespaces
> and methods that have 7604051 littered all over. Be more creative and name
> these things better.

Timothy:

I take it that there were two issues that you had a concern with:

1. The use of "7604051" all over the place.
2. The size of the comment.

Do I read your review correctly?

In response to 1, I agree. The patch section is pretty well-delimited, so internally it's not necessary to label everything. (Updated patch attached.)

As for 2, I disagree. While I know that WebKit style tends to be comment-light, there's nothing in the style guide that prohibits explanatory comments. Since the bug and the fix are both rather opaque, I feel that a comment is definitely worthwhile in this instance.

[David Levin]

Comment on attachment 48143 [details]
Updated per review

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.


> +@interface TCMVisibleView : NSView
> +
> +@end
> +
> +@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.

[Avi Drissman]

Created attachment 48248 [details]
Update style, comment

[Avi Drissman]

(In reply to comment #6)
> (From update of attachment 48143 [details])
> I don't feel comfortable reviewing Objective-C but I can help with general
> stuff to make it easier for an Objective-C reviewer.

Made all changes suggested; tweaked your revised comment slightly but it's still significantly shorter.

Timothy, looks like you're our Obj-C guy. What do you think?

[Avi Drissman]

Ping?

[Timothy Hatcher]

Comment on attachment 48248 [details]
Update style, comment


> ++ (NSView*)TCMInterposing_focusView;

Should have a space between the type and the star for ObjC types.

> +BOOL CurrentOSHasSetFocusRingStyleInBitmapBug()

You should use bool. Should start with a lowercase "c".

> +    UInt32 *pixelPlane = &pixel;
> +    UInt32 **pixelPlanes = &pixelPlane;

Put the stars next to the type here.

> +    NSBitmapImageRep *bitmap = [[NSBitmapImageRep alloc] initWithBitmapDataPlanes:(unsigned char **)pixelPlanes

Use UInt8 to match the UInt32s used earlier. No space should be between the type and stars.

> +    NSRectFill(NSMakeRect(0,0,1,1));

Spaces needed after the commas.

> +bool swizzle()

Could use a more descriptive name since the swizzles specific methods.

> +class ScopedFixer
> +{

Brace should go on the previous line.

> ++ (NSView*)TCMInterposing_focusView

Space between the star and ObjC type.

> +    NSView* view = [self TCMInterposing_focusView];

Srar should be next to the variable.

[Timothy Hatcher]

Comment on attachment 48248 [details]
Update style, comment

> ++ (NSView*)TCMInterposing_focusView
> +{
> +    NSView* view = [self TCMInterposing_focusView];

Also this should have a comment so it is clear you are calling the old method here because the were swizzled.

[Avi Drissman]

Created attachment 48418 [details]
Update for style

[Avi Drissman]

Created attachment 48419 [details]
Update for style (fixed double ChangeLog)

[WebKit Commit Bot]

Comment on attachment 48419 [details]
Update for style (fixed double ChangeLog)

Clearing flags on attachment: 48419

Committed r54582: <http://trac.webkit.org/changeset/54582>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.