[webkit-reviews] review granted: [Bug 21751] Convert checkbox/radio buttons on Mac to new Theme API : [Attachment 24525] Patch #2

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 21751: Convert checkbox/radio buttons on Mac to new Theme API
https://bugs.webkit.org/show_bug.cgi?id=21751

Attachment 24525: Patch #2
https://bugs.webkit.org/attachment.cgi?id=24525&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
 243	 Length width() const { return m_width; }
 244	 Length height() const { return m_height; }
 245	 
 246	 void setWidth(Length w) { m_width = w; }
 247	 void setHeight(Length h) { m_height = h; }

Should we be returning/accepting const Length& here, like we do for the
constructor?

 116	 if (oldIndeterminate != indeterminate || checked != oldChecked) {
 117	     [cell setState:indeterminate ? NSMixedState : (checked ? NSOnState
: NSOffState)];
 118	     return;
 119	 }

The return statement doesn't seem useful here.

I still think it would be good to share more code between checkboxes and radio
buttons. At least paintCheckbox/paintRadio seem trivial to merge. Even a FIXME
would be good!

@@ bool RenderTheme::isFocused(const Render
418489	       return false;
419490	   Document* document = node->document();
420491	   Frame* frame = document->frame();
421	 return node == document->focusedNode() && frame &&
frame->selection()->isFocusedAndActive();
 492	 return node == document->focusedNode() && frame &&
frame->selection()->isFocusedAndActive() && o->style()->outlineStyleIsAuto();

I think you meant to undo this change.

r=me