Bug 34544

Summary: [Chromium] RenderTheme does not draw focus rings on SL for checkboxes, radio buttons
Product: WebKit Reporter: Avi Drissman <avi>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, levin, mitz, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Attachments:
Description Flags
Patch to fix.
timothy: review-
Updated per review
levin: review-
Update style, comment
timothy: review-
Update for style
none
Update for style (fixed double ChangeLog) none

Description Avi Drissman 2010-02-03 15:25:44 PST
Downstream bug at http://code.google.com/p/chromium/issues/detail?id=27493 .

Explanation and fix in patch, to come.
Comment 1 Avi Drissman 2010-02-03 15:27:26 PST
Created attachment 48071 [details]
Patch to fix.
Comment 2 Timothy Hatcher 2010-02-03 17:03:35 PST
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.
Comment 3 Eric Seidel (no email) 2010-02-03 18:05:58 PST
I CC'd you on it because you speak Obj-C much better than I (and much of the WebKit project) do these days.
Comment 4 Avi Drissman 2010-02-04 08:03:22 PST
Created attachment 48143 [details]
Updated per review
Comment 5 Avi Drissman 2010-02-04 08:08:02 PST
(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.
Comment 6 David Levin 2010-02-05 10:38:24 PST
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.
Comment 7 Avi Drissman 2010-02-05 12:24:54 PST
Created attachment 48248 [details]
Update style, comment
Comment 8 Avi Drissman 2010-02-05 12:26:30 PST
(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?
Comment 9 Avi Drissman 2010-02-09 07:24:59 PST
Ping?
Comment 10 Timothy Hatcher 2010-02-09 07:48:58 PST
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.
Comment 11 Timothy Hatcher 2010-02-09 07:55:41 PST
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.
Comment 12 Avi Drissman 2010-02-09 08:27:07 PST
Created attachment 48418 [details]
Update for style
Comment 13 Avi Drissman 2010-02-09 08:29:10 PST
Created attachment 48419 [details]
Update for style (fixed double ChangeLog)
Comment 14 WebKit Commit Bot 2010-02-09 19:25:49 PST
Comment on attachment 48419 [details]
Update for style (fixed double ChangeLog)

Clearing flags on attachment: 48419

Committed r54582: <http://trac.webkit.org/changeset/54582>
Comment 15 WebKit Commit Bot 2010-02-09 19:25:59 PST
All reviewed patches have been landed.  Closing bug.