Convert buttons on OS X to new Theme API
Created attachment 24562 [details] Patch
Comment on attachment 24562 [details] Patch 41 LengthBox(int v) Maybe we should make this a named constructor, like LengthBox::square or LengthBox::fixedSquare or LengthBox::squareWithFixedSideLength? If we only ever use it for constructing 0-sized LengthBoxes (as your patch does), maybe we should remove the parameter and call it LengthBox::empty? @@ bool Theme::controlSupportsBorder(Contro 3636 case SearchFieldPart: 3737 case CheckboxPart: 3838 case RadioPart: 39 return false; 39 return LengthBox(0); Shouldn't we be returning zoomedBox here instead of LengthBox(0)? 92 // Allows the theme to modify the existing padding/border. 93 virtual LengthBox controlPadding(ControlPart, const Font&, const LengthBox& zoomedBox, float zoomFactor) const; 94 virtual LengthBox controlBorder(ControlPart, const Font&, const LengthBox& zoomedBox, float zoomFactor) const; Do you think it's worth mentioning that a return value of LengthBox(0) means that the control doesn't support border/padding? 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 think I'd prefer "minimum" over "min" in the name of this function. Should we stick "zoomed" in there, too? 48 virtual bool controlRequiresPreWhiteSpace(ControlPart part) const { return part == PushButtonPart; } I think we try to avoid inlining virtual functions. 314 static NSButtonCell* button(ControlPart part, ControlStates states, const IntRect& zoomedRect, float zoomFactor) 315 { 316 static NSButtonCell* buttonCell; We put the space before the * for ObjC types. 325 if (part == SquareButtonPart || 326 zoomedRect.height() > buttonSizes()[NSRegularControlSize].height() * zoomFactor) { I think it's better to have the boolean operator at the beginning of the second line in multi-line expressions like this. 346 static void paintButton(ControlPart part, ControlStates states, GraphicsContext* context, const IntRect& zoomedRect, float zoomFactor, ScrollView* scrollView) 347 { 348 // Determine the width and height needed for the control and prepare the cell for painting. 349 NSButtonCell* buttonCell = button(part, states, zoomedRect, zoomFactor); Ditto here re: * placement 350 LocalCurrentGraphicsContext localContext(context); 351 352 context->save(); Why is it necessary to save/restore the local context? 356 zoomedSize.setWidth(zoomedRect.width()); 357 zoomedSize.setHeight(zoomedSize.height() * zoomFactor); Why do we only multiply the height by the zoomFactor? Have you checked the default button pulse after applying your patch? 406 FontDescription ThemeMac::controlFont(ControlPart part, const Font& font, float zoomFactor) const 407 { 408 switch (part) { 409 case PushButtonPart: { 410 FontDescription fontDescription; 411 fontDescription.setIsAbsoluteSize(true); 412 fontDescription.setGenericFamily(FontDescription::SerifFamily); 413 414 NSFont* nsFont = [NSFont systemFontOfSize:[NSFont systemFontSizeForControlSize:controlSizeForFont(font)]]; 415 fontDescription.firstFamily().setFamily([nsFont familyName]); 416 fontDescription.setComputedSize([nsFont pointSize] * zoomFactor); 417 fontDescription.setSpecifiedSize([nsFont pointSize] * zoomFactor); 418 return fontDescription; 419 } 420 default: 421 return Theme::controlFont(part, font, zoomFactor); 422 } 423 } With only two cases it might be clearer to just use an if: if (part != PushButtonPart) return Theme::controlFont(...); .... 440 LengthSize ThemeMac::minControlSize(ControlPart part, const Font& font, float zoomFactor) const 441 { 442 switch (part) { 443 case SquareButtonPart: 444 case DefaultButtonPart: 445 case ButtonPart: 446 return LengthSize(Length(0, Fixed), Length(static_cast<int>(15 * zoomFactor), Fixed)); Is truncation here correct? If it isn't, maybe we should add a FIXME. 452 LengthBox ThemeMac::controlBorder(ControlPart part, const Font& font, const LengthBox& zoomedBox, float zoomFactor) const 453 { 454 switch (part) { 455 case SquareButtonPart: 456 case DefaultButtonPart: 457 case ButtonPart: { 458 return LengthBox(0, zoomedBox.right().value(), 0, zoomedBox.left().value()); 459 } The braces here aren't needed. 465 LengthBox ThemeMac::controlPadding(ControlPart part, const Font& font, const LengthBox& zoomedBox, float zoomFactor) const 466 { 467 switch (part) { 468 case PushButtonPart: { Why don't we lump DefaultButtonPart in with PushButtonPart in this function? We do that in other similar functions. I assume we aren't mentioning ButtonPart here because we have different rules for <button> vs. <input type=button>? @@ void ThemeMac::inflateControlPaintRect(C 336504 zoomedRect = inflateRect(zoomedRect, zoomedSize, radioMargins(controlSize), zoomFactor); 337505 break; 338506 } 507 case PushButtonPart: 508 case DefaultButtonPart: 509 case ButtonPart: { 510 NSButtonCell* cell = button(part, states, zoomedRect, zoomFactor); 511 NSControlSize controlSize = [cell controlSize]; 512 513 // We inflate the rect as needed to account for the Aqua button's shadow. 514 if ([cell bezelStyle] == NSRoundedBezelStyle) { 515 IntSize zoomedSize = buttonSizes()[controlSize]; 516 zoomedSize.setHeight(zoomedSize.height() * zoomFactor); 517 zoomedSize.setWidth(zoomedRect.width()); 518 zoomedRect = inflateRect(zoomedRect, zoomedSize, buttonMargins(controlSize), zoomFactor); 519 } Misplaced * before cell. You can move controlSize within the if block. Why are we only multiplying the height by the zoomFactor? 91 if (!borderBox.top().value()) 92 style->resetBorderTop(); 93 else 94 style->setBorderTopWidth(borderBox.top().value()); Why not just flip this if/else so you can get rid of the negation? I think that would be clearer. Ditto for the other if/else pairs in this block of code, of course. 510 if (isDefault(o) && o->document()->page()->focusController()->isActive()) Should we be checking SelectionController::isFocusedAndActive instead of FocusController::isActive? (i.e., do we want to allow default buttons pulsing in any frame other than the focused frame?) This change seems worth a comment in the ChangeLog (as do many others, but particularly this one since it isn't directly related to moving things into Theme).
Created attachment 24571 [details] Patch #2
Comment on attachment 24571 [details] Patch #2 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
Created attachment 24573 [details] Patch #3
Comment on attachment 24573 [details] Patch #3 r=me
Fixed in r37790.