[webkit-reviews] review granted: [Bug 21775] Convert buttons on OS X to new Theme API : [Attachment 24571] 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 21775: Convert buttons on OS X to new Theme API
https://bugs.webkit.org/show_bug.cgi?id=21775

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
 89	// Returns the minimum size for a control in zoomed coordinates.  
 90	virtual LengthSize minControlSize(ControlPart, const Font&, float
zoomFactor) const { return LengthSize(Length(0, Fixed), Length(0, Fixed)); }

I still think "minimumControlSize" would be nicer.

 299 // Buttons really only constrain height.  They respect width.

We normally just use a single space after a period.

 352	 LocalCurrentGraphicsContext localContext(context);
 353 
 354	 context->save();

Is the save/restore actually needed since we have a local context?

 91		    if (borderBox.top().value())
 92			style->setBorderTopWidth(borderBox.top().value());
 93		    else
 94			style->resetBorderTop();

If the reset* calls are only needed to appease DRT, you could add a comment to
that effect.

 440	 float zoomLevel = o->style()->effectiveZoom();

I do think zoomFactor is a clearer name than zoomLevel. A zoomLevel of 1.0f
makes it sound like the user zoomed once, while in fact there is no zooming
going on at all.

r=me