Bug 34544 - [Chromium] RenderTheme does not draw focus rings on SL for checkboxes, radio buttons
Summary: [Chromium] RenderTheme does not draw focus rings on SL for checkboxes, radio ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-03 15:25 PST by Avi Drissman
Modified: 2010-02-09 19:25 PST (History)
5 users (show)

See Also:


Attachments
Patch to fix. (8.19 KB, patch)
2010-02-03 15:27 PST, Avi Drissman
timothy: review-
Details | Formatted Diff | Diff
Updated per review (8.09 KB, patch)
2010-02-04 08:03 PST, Avi Drissman
levin: review-
Details | Formatted Diff | Diff
Update style, comment (7.14 KB, patch)
2010-02-05 12:24 PST, Avi Drissman
timothy: review-
Details | Formatted Diff | Diff
Update for style (8.00 KB, patch)
2010-02-09 08:27 PST, Avi Drissman
no flags Details | Formatted Diff | Diff
Update for style (fixed double ChangeLog) (7.21 KB, patch)
2010-02-09 08:29 PST, Avi Drissman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.