[webkit-reviews] review granted: [Bug 21751] Convert checkbox/radio buttons on Mac to new Theme API : [Attachment 24522] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 20 08:13:11 PDT 2008
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 24522: Patch
https://bugs.webkit.org/attachment.cgi?id=24522&action=edit
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
81 // The size here is in zoomed coordinates already. If a new size is
returned, it also needs to be in zoomed coordinates.
82 virtual IntSize controlSize(ControlPart, const Font&, const IntSize&
size, float zoomFactor) const { return size; }
Should we call the parameter "zoomedSize"?
91 // Method for painting a control. The rect is in zoomed coordinates.
92 virtual void paint(ControlPart, ControlStates, GraphicsContext*, const
IntRect&, float zoomFactor, ScrollView*) const { };
96 // The rect passed in is in zoomed coordinates, so the inflation should
take that into account and make sure the inflation
97 // amount is also scaled by the zoomFactor.
98 virtual void inflateControlPaintRect(ControlPart, ControlStates,
IntRect&, float zoomFactor) const { }
Should we call the IntRect parameter in these two functions zoomedRect?
62 static IntSize sizeFromFont(const Font& font, const IntSize& size, float
zoomFactor, const IntSize* sizes)
It would be useful to document which of the size parameter, the sizes
parameter, and the return value should be in zoomed coordinates, either with a
comment or by making the parameter names clearer or both.
75 static void setControlSize(NSCell* cell, const IntSize* sizes, const
IntSize& minSize, float zoomLevel)
Ditto
78 if (minSize.width() >=
static_cast<int>(sizes[NSRegularControlSize].width() * zoomLevel) &&
79 minSize.height() >=
static_cast<int>(sizes[NSRegularControlSize].height() * zoomLevel))
80 size = NSRegularControlSize;
81 else if (minSize.width() >=
static_cast<int>(sizes[NSSmallControlSize].width() * zoomLevel) &&
82 minSize.height() >=
static_cast<int>(sizes[NSSmallControlSize].height() * zoomLevel))
83 size = NSSmallControlSize;
Is truncating really the right behavior here? Should we be rounding instead?
96 bool pressed = (states & PressedState);
I don't think the parentheses here (and on other similar lines) improve
clarity.
116 if (oldIndeterminate != indeterminate) {
117 [cell setState:indeterminate ? NSMixedState : (checked ? NSOnState
: NSOffState)];
118 return;
119 }
120 bool oldChecked = [cell state] == NSOnState;
121 if (checked != oldChecked)
122 [cell setState:checked ? NSOnState : NSOffState];
I think you could combine this into a single call to setState:
if (oldIndeterminate != indeterminate || oldChecked != checked)
[cell setState:indeterminate ? NSMixedState : (checked ? NSOnState :
NSOffState)];
128 static IntRect inflateRect(const IntRect& r, const IntSize& size, const
int* margins, float zoomLevel)
165 static IntSize checkboxSize(ControlPart part, const Font& font, const
IntSize& size, float zoomFactor)
175 static NSButtonCell* checkbox(ControlStates states, const IntRect& rect,
float zoomFactor)
195 static void paintCheckbox(ControlStates states, GraphicsContext* context,
const IntRect& rect, float zoomFactor, ScrollView* scrollView)
251 static NSButtonCell* radio(ControlStates states, const IntRect& rect,
float zoomFactor)
270 static void paintRadio(ControlStates states, GraphicsContext* context,
const IntRect& rect, float zoomFactor, ScrollView* scrollView)
Ditto for these functions re: which parameters/return value are zoomed.
177 static RetainPtr<NSButtonCell> checkboxCell;
I think we should just use bare (leaked) pointers for all these static NSCell
objects.
Is it possible to share code between paintCheckbox and paintRadio? It sure
seems like it!
230 static const int* radioMargins(NSControlSize controlSize)
231 {
232 static const int margins[3][4] =
233 {
234 { 2, 2, 4, 2 },
235 { 3, 2, 3, 2 },
236 { 1, 0, 2, 0 },
237 };
238 return margins[controlSize];
239 }
Should margins instead be a ControlBox[3]?
299 int ThemeMac::baselinePositionAdjustment(ControlPart part) const
300 {
301 if (part == CheckboxPart || part == RadioPart)
302 return -2;
Should you be using cUnchangedControlSize instead of -2 here?
345 void ThemeMac::inflateControlPaintRect(ControlPart part, ControlStates
states, IntRect& rect, float zoomFactor) const
346 {
347 switch (part) {
348 case CheckboxPart: {
349 // We inflate the rect as needed to account for padding
included in the cell to accommodate the checkbox
350 // shadow" and the check. We don't consider this part of the
bounds of the control in WebKit.
351 NSCell* cell = checkbox(states, rect, zoomFactor);
352 NSControlSize controlSize = [cell controlSize];
353 IntSize size = checkboxSizes()[controlSize];
354 size.setHeight(size.height() * zoomFactor);
355 size.setWidth(size.width() * zoomFactor);
356 rect = inflateRect(rect, size, checkboxMargins(controlSize),
zoomFactor);
357 break;
358 }
359 case RadioPart: {
360 // We inflate the rect as needed to account for padding
included in the cell to accommodate the radio button
361 // shadow". We don't consider this part of the bounds of the
control in WebKit.
362 NSCell* cell = radio(states, rect, zoomFactor);
363 NSControlSize controlSize = [cell controlSize];
364 IntSize size = radioSizes()[controlSize];
365 size.setHeight(size.height() * zoomFactor);
366 size.setWidth(size.width() * zoomFactor);
367 rect = inflateRect(rect, size, radioMargins(controlSize),
zoomFactor);
368 break;
369 }
370 default:
371 break;
372 }
373 }
Can we share more code here?
105 IntSize controlSize = m_theme->controlSize(part, style->font(),
IntSize(lengthToInt(style->width()), lengthToInt(style->height())),
style->effectiveZoom());
106 if (controlSize.width() != cUnchangedControlSize)
107 style->setWidth(intToLength(controlSize.width()));
108 if (controlSize.height() != cUnchangedControlSize)
109 style->setHeight(intToLength(controlSize.height()));
I wonder if it would be clearer for controlSize() to have two boolean out
parmeters, widthChanged and heightChanged, rather than using these magic
values.
374442 // FIXME: It would be nice to set this state on the RenderObjects
instead of
375443 // having to dig up to the FocusController at paint time.
444 ControlStates RenderTheme::controlStatesForRenderer(const RenderObject* o)
const
The FIXME is now misplaced.
@@ bool RenderTheme::isFocused(const Render
418510 return false;
419511 Document* document = node->document();
420512 Frame* frame = document->frame();
421 return node == document->focusedNode() && frame &&
frame->selection()->isFocusedAndActive();
513 return node == document->focusedNode() && frame &&
frame->selection()->isFocusedAndActive() && o->style()->outlineStyleIsAuto();
It seems wrong for isFocused to return false just because someone set
"outline:none" on this element. Should callers of isFocused instead be
responsible for checking this? Or maybe we should rename the function to
isShowingFocus?
r=me
More information about the webkit-reviews
mailing list