Bug 21775 – Convert buttons on OS X to new Theme API

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

Convert buttons on OS X to new Theme API

[Dave Hyatt]

Created attachment 24562 [details]
Patch

[Adam Roben (:aroben)]

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).

[Dave Hyatt]

Created attachment 24571 [details]
Patch #2

[Adam Roben (:aroben)]

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

[Dave Hyatt]

Created attachment 24573 [details]
Patch #3

[Adam Roben (:aroben)]

Comment on attachment 24573 [details]
Patch #3

r=me

[Dave Hyatt]

Fixed in r37790.