Closed Bug 1421088 Opened 6 years ago Closed 6 years ago

Prepare nsNativeThemeCocoa for remote theme rendering

Categories

(Core :: Widget: Cocoa, enhancement, P2)

All
macOS
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(29 files)

59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
For bug 1421087, theme rendering needs to be split into a data gathering phase and a rendering phase. This bug is about the Mac part of that work.
Attachment #8933502 - Flags: review?(spohl.mozilla.bugs)
I've pushed the roughly the first half of the patches to review. The rest is coming tomorrow.
Priority: -- → P2
Comment on attachment 8933491 [details]
Bug 1421088 - Simplify search field cell management and make the "is-within-toolbar" computation eager instead of lazy.

https://reviewboard.mozilla.org/r/204422/#review212354

::: widget/cocoa/nsNativeThemeCocoa.mm:317
(Diff revision 1)
> +
> +@implementation ToolbarSearchFieldCellWithFocusRing
> +
> +- (BOOL)_isToolbarMode
> +{
> +  // On 10.7, searchfields have two different styles, depending on whether

This comment doesn't seem applicable here anymore. Maybe move into |IsInsideToolbar|?

::: widget/cocoa/nsNativeThemeCocoa.mm:1052
(Diff revision 1)
>  
> +static bool
> +IsToolbarStyleContainer(nsIFrame* aFrame)
> +{
> +  nsIContent* content = aFrame->GetContent();
> +  if (!content)

nit: add braces

::: widget/cocoa/nsNativeThemeCocoa.mm:1058
(Diff revision 1)
> +    return false;
> +
> +  if (content->IsAnyOfXULElements(nsGkAtoms::toolbar,
> +                                  nsGkAtoms::toolbox,
> +                                  nsGkAtoms::statusbar))
> +    return true;

nit: add braces
Attachment #8933491 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933502 [details]
Bug 1421088 - Don't pass an NSWindow* to DrawUnifiedToolbar.

https://reviewboard.mozilla.org/r/204444/#review212382
Attachment #8933502 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933836 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawTabPanel.

https://reviewboard.mozilla.org/r/204762/#review212384
Attachment #8933836 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933837 [details]
Bug 1421088 - Remove unused parameters from DrawFocusOutline.

https://reviewboard.mozilla.org/r/204764/#review212390
Attachment #8933837 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933838 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawStatusBar.

https://reviewboard.mozilla.org/r/204766/#review212400
Attachment #8933838 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933839 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawResizer.

https://reviewboard.mozilla.org/r/204768/#review212404
Attachment #8933839 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933841 [details]
Bug 1421088 - Create DrawToolbar.

https://reviewboard.mozilla.org/r/204772/#review212410

::: widget/cocoa/nsNativeThemeCocoa.h:381
(Diff revision 1)
>                         const SpinButtonParams& aParams);
>    void DrawSpinButton(CGContextRef context,
>                        const HIRect& inBoxRect, SpinButton aDrawnButton,
>                        const SpinButtonParams& aParams);
> +  void DrawToolbar(CGContextRef cgContext, const CGRect& inBoxRect,
> +                   bool aIsMain);

I'm a bit torn about the inconsistent use of the 'a' prefix for parameters. I think in the interest of consistency in this file this is fine. This applies to the all the other reviews as well.
Attachment #8933841 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933842 [details]
Bug 1421088 - Create DrawMenuSeparator.

https://reviewboard.mozilla.org/r/204774/#review212424
Attachment #8933842 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933843 [details]
Bug 1421088 - Create DrawSourceList.

https://reviewboard.mozilla.org/r/204776/#review212428
Attachment #8933843 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933844 [details]
Bug 1421088 - Create a DrawNativeTitlebar overload that takes a UnifiedToolbarParams param.

https://reviewboard.mozilla.org/r/204778/#review212430

::: widget/cocoa/nsNativeThemeCocoa.h:386
(Diff revision 1)
>                        const SpinButtonParams& aParams);
>    void DrawToolbar(CGContextRef cgContext, const CGRect& inBoxRect,
>                     bool aIsMain);
>    void DrawUnifiedToolbar(CGContextRef cgContext, const HIRect& inBoxRect,
>                            const UnifiedToolbarParams& aParams);
> +  void DrawNativeTitlebar(CGContextRef aContext, CGRect aTitlebarRect,

nit: I'd say we should either stick to the 'a' prefix throughout, or to the existing param names such as 'cgContext'.
Attachment #8933844 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933492 [details]
Bug 1421088 - Split DrawPushButton into four functions and group the frame-dependent information into a struct.

https://reviewboard.mozilla.org/r/204424/#review212688

::: widget/cocoa/nsNativeThemeCocoa.h:159
(Diff revision 1)
> +                                 const HIRect& inBoxRect,
> +                                 ControlParams aControlParams);
> +  void DrawHelpButton(CGContextRef cgContext, const HIRect& inBoxRect,
> +                      ControlParams aControlParams);
> +  void DrawDisclosureButton(CGContextRef cgContext, const HIRect& inBoxRect,
> +                            ControlParams aControlParams, NSCellStateValue aState);

nit: maybe s/aState/aCellState/ to be more descriptive?

::: widget/cocoa/nsNativeThemeCocoa.mm:948
(Diff revision 1)
> +{
> +  [aCell setEnabled:!aControlParams.disabled];
> +  [aCell setShowsFirstResponder:(aControlParams.focused &&
> +                                 !aControlParams.disabled &&
> +                                 aControlParams.insideActiveWindow)];
> +  [aCell setHighlighted:aControlParams.pressed];

Shouldn't this be:

[aCell setHighlighted:aControlParams.insideActiveWindow && aControlParams.pressed)];

or was this an intentional change?

::: widget/cocoa/nsNativeThemeCocoa.mm:1257
(Diff revision 1)
> +                      false); // Don't mirror icon in RTL.
> +
> +  NS_OBJC_END_TRY_ABORT_BLOCK;
> -  }
> +}
>  
> -#if DRAW_IN_FRAME_DEBUG
> +void

Has DRAW_IN_FRAME_DEBUG outlived its usefulness? Should we remove it throughout?

::: widget/cocoa/nsNativeThemeCocoa.mm:2543
(Diff revision 1)
>          }
>          DrawButton(cgContext, kThemePushButton, macRect, isInActiveWindow,
>                     kThemeButtonOff, kThemeAdornmentNone, eventState, aFrame);
>        } else if (IsButtonTypeMenu(aFrame)) {
>          DrawDropdown(cgContext, macRect, eventState, aWidgetType, aFrame);
> +      } else if (macRect.size.height > DO_SQUARE_BUTTON_HEIGHT) {

We're now checking macRect.size.height instead of nativeWidgetHeight. nativeWidgetHeight is a rounded value of nativeWidgetRect.Height() (and possibly multiplied by 0.5f if on hidpi) while macRect.size.height is not rounded. If this is an intentional change we should remove nativeWidgetHeight as it is no longer used.

::: widget/cocoa/nsNativeThemeCocoa.mm:2569
(Diff revision 1)
> +                     ComputeControlParams(aFrame, eventState));
> +      break;
> +
>      case NS_THEME_MAC_DISCLOSURE_BUTTON_OPEN:
> -    case NS_THEME_MAC_DISCLOSURE_BUTTON_CLOSED:
> -      DrawPushButton(cgContext, macRect, eventState, aWidgetType, aFrame,
> +    case NS_THEME_MAC_DISCLOSURE_BUTTON_CLOSED: {
> +      NSCellStateValue value = (aWidgetType == NS_THEME_MAC_DISCLOSURE_BUTTON_CLOSED)

nit: maybe s/value/cellState/ to be more descriptive?
Attachment #8933492 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933498 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawCheckboxOrRadio.

https://reviewboard.mozilla.org/r/204436/#review212692

::: widget/cocoa/nsNativeThemeCocoa.mm:1014
(Diff revision 1)
> +      return NSMixedState;
> +  }
> +}
> +
>  void
>  nsNativeThemeCocoa::DrawCheckboxOrRadio(CGContextRef cgContext, bool inCheckbox,

nit: s/inCheckbox/aIsCheckbox/

(This can be fixed in the additional patch that fixes all prefixes in these two files, as discussed via IRC)

::: widget/cocoa/nsNativeThemeCocoa.mm
(Diff revision 1)
> -    state = NSMixedState;
> -
> -  [cell setEnabled:!IsDisabled(aFrame, inState)];
> -  [cell setShowsFirstResponder:inState.HasState(NS_EVENT_STATE_FOCUS)];
> -  [cell setState:state];
> -  [cell setHighlighted:inState.HasAllStates(NS_EVENT_STATE_ACTIVE | NS_EVENT_STATE_HOVER)];

I see that highlighting here does not depend on aParams.controlParams.insideActiveWindow being true, while it did in DrawPushButton. Just pointing this out to make sure we're intentionally consolidating the two to ignore aParams.controlParams.insideActiveWindow.
Attachment #8933498 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933499 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawDropdown.

https://reviewboard.mozilla.org/r/204438/#review212694

::: widget/cocoa/nsNativeThemeCocoa.mm:1839
(Diff revision 1)
>  
> -  [mDropdownCell setPullsDown:(aWidgetType == NS_THEME_BUTTON)];
> +  [mDropdownCell setPullsDown:aParams.pullsDown];
> +  NSCell* cell = aParams.editable ? (NSCell*)mComboBoxCell : (NSCell*)mDropdownCell;
>  
> -  BOOL isEditable = (aWidgetType == NS_THEME_MENULIST_TEXTFIELD);
> -  NSCell* cell = isEditable ? (NSCell*)mComboBoxCell : (NSCell*)mDropdownCell;
> +  ApplyControlParamsToNSCell(aParams.controlParams, cell);
> +  [cell setControlTint:(aParams.controlParams.insideActiveWindow ? [NSColor currentControlTint] : NSClearControlTint)];

nit: over 80 characters, this and following line.
Attachment #8933499 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933493 [details]
Bug 1421088 - Create a separate method for drawing tree header cells.

https://reviewboard.mozilla.org/r/204426/#review212714

::: widget/cocoa/nsNativeThemeCocoa.mm:1501
(Diff revision 1)
> -    // Always remove the top border.
> +  // Always remove the top border.
> -    drawFrame.origin.y -= 1;
> +  drawFrame.origin.y -= 1;
> -    drawFrame.size.height += 1;
> +  drawFrame.size.height += 1;
> -    // Remove the left border in LTR mode and the right border in RTL mode.
> +  // Remove the left border in LTR mode and the right border in RTL mode.
> -    drawFrame.size.width += 1;
> +  drawFrame.size.width += 1;
> -    bool isLast = IsLastTreeHeaderCell(aFrame);
> +  if (aParams.lastTreeHeaderCell)

nit: add braces

::: widget/cocoa/nsNativeThemeCocoa.mm:1503
(Diff revision 1)
> -    // Remove the left border in LTR mode and the right border in RTL mode.
> +  // Remove the left border in LTR mode and the right border in RTL mode.
> -    drawFrame.size.width += 1;
> +  drawFrame.size.width += 1;
> -    bool isLast = IsLastTreeHeaderCell(aFrame);
> +  if (aParams.lastTreeHeaderCell)
> -    if (isLast)
> -      drawFrame.size.width += 1; // Also remove the other border.
> +    drawFrame.size.width += 1; // Also remove the other border.
> -    if (!IsFrameRTL(aFrame) || isLast)
> +  if (!aParams.controlParams.rtl || aParams.lastTreeHeaderCell)

nit: add braces
Attachment #8933493 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933492 [details]
Bug 1421088 - Split DrawPushButton into four functions and group the frame-dependent information into a struct.

https://reviewboard.mozilla.org/r/204424/#review212718
Comment on attachment 8933495 [details]
Bug 1421088 - Route more button types through DrawButton.

https://reviewboard.mozilla.org/r/204430/#review212720
Attachment #8933495 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933503 [details]
Bug 1421088 - Rename DrawFrame to DrawTextBox and stop passing an nsIFrame* to it.

https://reviewboard.mozilla.org/r/204446/#review212722

::: widget/cocoa/nsNativeThemeCocoa.mm:2979
(Diff revision 2)
>        }
>  
> -      DrawFrame(cgContext, kHIThemeFrameTextFieldSquare, macRect,
> -                IsDisabled(aFrame, eventState) || IsReadOnly(aFrame), eventState);
> +      bool isDisabled = IsDisabled(aFrame, eventState) || IsReadOnly(aFrame);
> +      DrawTextBox(cgContext, macRect, TextBoxParams{isDisabled, isFocused});
> +    }
>        break;

nit: Move |break;| before closing brace.
Attachment #8933503 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933840 [details]
Bug 1421088 - Create DrawMultilineTextField.

https://reviewboard.mozilla.org/r/204770/#review212724
Attachment #8933840 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933504 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawSearchField.

https://reviewboard.mozilla.org/r/204448/#review212726
Attachment #8933504 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933494 [details]
Bug 1421088 - Give nsNativeThemeCocoa::DrawButton a different API.

https://reviewboard.mozilla.org/r/204428/#review212728

::: widget/cocoa/nsNativeThemeCocoa.mm:2676
(Diff revision 1)
>                             ComputeControlParams(aFrame, eventState), value);
>      }
>        break;
>  
> -    case NS_THEME_BUTTON_BEVEL:
> -      DrawButton(cgContext, kThemeMediumBevelButton, macRect,
> +    case NS_THEME_BUTTON_BEVEL: {
> +      bool isDefaultButton = IsDefaultButton(aFrame);

The existing code would be equivalent to:

bool isDefaultButton = IsDefaultButton(aFrame) && !eventState.HasState(NS_EVENT_STATE_ACTIVE);

Do we not need to check the active state for bevel buttons?
Attachment #8933494 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933500 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawSpinButton / DrawSpinButtons.

https://reviewboard.mozilla.org/r/204440/#review212756

::: widget/cocoa/nsNativeThemeCocoa.mm:1906
(Diff revision 1)
> +                                    const SpinButtonParams& aParams)
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>  
> +  HIThemeButtonDrawInfo bdi =
> +    SpinButtonDrawInfo(kThemeIncDecButtonMini, aParams);

This should be kThemeIncDecButton, no?
Attachment #8933500 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933496 [details]
Bug 1421088 - Move menu background drawing into a separate method.

https://reviewboard.mozilla.org/r/204432/#review212730

::: widget/cocoa/nsNativeThemeCocoa.mm:1127
(Diff revision 1)
> +static Color
> +VibrancyFillColor(nsIFrame* aFrame, nsITheme::ThemeGeometryType aThemeGeometryType)
> +{
> +  ChildView* childView = ChildViewForFrame(aFrame);
> +  if (childView) {
> +    return NSColorToColor([childView vibrancyFillColorForThemeGeometryType:aThemeGeometryType]);

nit: this code only moved around, but let's fix the fact that this is over 80 characters.

::: widget/cocoa/nsNativeThemeCocoa.mm:1138
(Diff revision 1)
> +DrawVibrancyBackground(CGContextRef cgContext, CGRect inBoxRect,
> +                       const Color& aColor, int aCornerRadiusIfOpaque = 0)
> +{
> +  NSRect rect = NSRectFromCGRect(inBoxRect);
> +  NSGraphicsContext* savedContext = [NSGraphicsContext currentContext];
> +  [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithGraphicsPort:cgContext flipped:YES]];

nit: over 80 characters

::: widget/cocoa/nsNativeThemeCocoa.mm:1142
(Diff revision 1)
> +  NSGraphicsContext* savedContext = [NSGraphicsContext currentContext];
> +  [NSGraphicsContext setCurrentContext:[NSGraphicsContext graphicsContextWithGraphicsPort:cgContext flipped:YES]];
> +  [NSGraphicsContext saveGraphicsState];
> +
> +  NSColor* fillColor = [NSColor colorWithDeviceRed:aColor.r
> +                                              green:aColor.g

nit: fix vertical alignment

::: widget/cocoa/nsNativeThemeCocoa.mm:1157
(Diff revision 1)
> +    // has: It stops the window server from applying window masks. We use
> +    // a window mask to get rounded corners on menus. So since the mask
> +    // doesn't work in "reduce vibrancy" mode, we need to do our own rounded
> +    // corner clipping here.
> +    [[NSBezierPath bezierPathWithRoundedRect:rect
> +                                      xRadius:aCornerRadiusIfOpaque

nit: fix vertical alignment

::: widget/cocoa/nsNativeThemeCocoa.mm:1205
(Diff revision 1)
> +      mdi.menuType = kThemeMenuTypeHierarchical;
> +    }
> +
> +    // The rounded corners draw outside the frame.
> +    CGRect deflatedRect = CGRectMake(inBoxRect.origin.x, inBoxRect.origin.y + 4,
> +                                      inBoxRect.size.width, inBoxRect.size.height - 8);

nit: over 80 characters
Attachment #8933496 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933497 [details]
Bug 1421088 - Change the API of DrawMenuIcon and add DrawMenuItem.

https://reviewboard.mozilla.org/r/204434/#review216246

::: widget/cocoa/nsNativeThemeCocoa.mm:1320
(Diff revision 1)
> +{
> +  bool isDisabled = IsDisabled(aFrame, aEventState);
> +
> +  MenuItemParams params;
> +  if (VibrancyManager::SystemSupportsVibrancy()) {
> +    ThemeGeometryType type = ThemeGeometryTypeForWidget(aFrame, NS_THEME_MENUITEM);

nit: over 80 characters

::: widget/cocoa/nsNativeThemeCocoa.mm:1325
(Diff revision 1)
> +    ThemeGeometryType type = ThemeGeometryTypeForWidget(aFrame, NS_THEME_MENUITEM);
> +    params.vibrancyColor = Some(VibrancyFillColor(aFrame, type));
> +  }
> +  params.checked = aIsChecked;
> +  params.disabled = isDisabled;
> +  params.selected = !isDisabled && CheckBooleanAttr(aFrame, nsGkAtoms::menuactive);

nit: over 80 characters
Attachment #8933497 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933501 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawSegment.

https://reviewboard.mozilla.org/r/204442/#review216304

::: widget/cocoa/nsNativeThemeCocoa.mm:2386
(Diff revision 1)
> -  BOOL isPressed = IsPressedButton(aFrame);
> -  if (isSelected && aSettings.ignoresPressedWhenSelected) {
> -    isPressed = NO;
> -  }
>  
> -  BOOL isRTL = IsFrameRTL(aFrame);
> +  NSControlSize controlSize = FindControlSize(inBoxRect.size.height, renderSettings.heights, 4.0f);

nit: over 80 characters.

::: widget/cocoa/nsNativeThemeCocoa.mm:2391
(Diff revision 1)
> -  BOOL drawRightSeparator = SeparatorResponsibility(aFrame, right) == aFrame;
> -  NSControlSize controlSize = FindControlSize(drawRect.size.height, aSettings.heights, 4.0f);
>  
>    RenderWithCoreUI(drawRect, cgContext, [NSDictionary dictionaryWithObjectsAndKeys:
> -            aSettings.widgetName, @"widget",
> -            (isActive ? @"kCUIPresentationStateActiveKey" : @"kCUIPresentationStateInactive"), @"kCUIPresentationStateKey",
> +            renderSettings.widgetName, @"widget",
> +            (aParams.insideActiveWindow ? @"kCUIPresentationStateActiveKey" : @"kCUIPresentationStateInactive"), @"kCUIPresentationStateKey",

nit: keeping the formatting identical helped for the review. let's change it to <= 80 characters before landing.
Attachment #8933501 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933505 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawProgress.

https://reviewboard.mozilla.org/r/204450/#review216332

::: widget/cocoa/nsNativeThemeCocoa.mm:3020
(Diff revision 2)
>          if (!QueueAnimatedContentForRefresh(aFrame->GetContent(), 30)) {
>            NS_WARNING("Unable to animate progressbar!");
>          }
>        }
> -      DrawProgress(cgContext, macRect, IsIndeterminateProgress(aFrame, eventState),
> -                   !IsVerticalProgress(aFrame),
> +      DrawProgress(cgContext, macRect,
> +                   ComputeProgressParams(aFrame, eventState, true));

Should we be passing |!IsVerticalProgress(aFrame)| instead of |true| to ComputeProgressParams() here, or is this guaranteed to always be true for NS_THEME_PROGRESSBAR since vertical progressbars are handled by NS_THEME_PROGRESSBAR_VERTICAL?
Attachment #8933505 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933506 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawMeter.

https://reviewboard.mozilla.org/r/204452/#review216354
Attachment #8933506 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933507 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawScale, and create DrawScrollbarTrack and DrawScrollbarThumb.

https://reviewboard.mozilla.org/r/204454/#review216812

::: widget/cocoa/nsNativeThemeCocoa.mm:2298
(Diff revision 2)
> +nsNativeThemeCocoa::ComputeHTMLScaleParams(nsIFrame* aFrame,
> +                                           EventStates aEventState)
> +{
> +  nsRangeFrame *rangeFrame = do_QueryFrame(aFrame);
> +  if (!rangeFrame) {
> +    return ScaleParams();

Previously, we wouldn't call DrawScale at all if rangeFrame was NULL. Is calling DrawScale with ScaleParams() guaranteed to lead to the same behavior?

::: widget/cocoa/nsNativeThemeCocoa.mm:2332
(Diff revision 2)
>    tdi.bounds = inBoxRect;
> -  tdi.min = inMinValue;
> -  tdi.max = inMaxValue;
> -  tdi.value = inCurrentValue;
> +  tdi.min = aParams.min;
> +  tdi.max = aParams.max;
> +  tdi.value = aParams.value;
>    tdi.attributes = kThemeTrackShowThumb;
> -  if (!inIsVertical)
> +  if (aParams.horizontal)

nit: Let's add braces throughout.

::: widget/cocoa/nsNativeThemeCocoa.mm:2341
(Diff revision 2)
> -  if (inState.HasState(NS_EVENT_STATE_FOCUS))
> +  if (aParams.focused)
>      tdi.attributes |= kThemeTrackHasFocus;
> -  if (IsDisabled(aFrame, inState))
> +  if (aParams.disabled)
>      tdi.enableState = kThemeTrackDisabled;
>    else
> -    tdi.enableState = FrameIsInActiveWindow(aFrame) ? kThemeTrackActive : kThemeTrackInactive;
> +    tdi.enableState = aParams.insideActiveWindow ? kThemeTrackActive : kThemeTrackInactive;

nit: over 80 characters, here and elsewhere in this patch.

::: widget/cocoa/nsNativeThemeCocoa.mm:3221
(Diff revision 2)
>      case NS_THEME_SCROLLBAR:
>        break;
>      case NS_THEME_SCROLLBARTHUMB_VERTICAL:
> -    case NS_THEME_SCROLLBARTHUMB_HORIZONTAL: {
> -      BOOL isOverlay = nsLookAndFeel::UseOverlayScrollbars();
> -      BOOL isHorizontal = (aWidgetType == NS_THEME_SCROLLBARTHUMB_HORIZONTAL);
> +    case NS_THEME_SCROLLBARTHUMB_HORIZONTAL:
> +      DrawScrollbarThumb(cgContext, macRect,
> +        ComputeScrollbarParams(aFrame, aWidgetType == NS_THEME_SCROLLBARTHUMB_HORIZONTAL));

nit: over 80 characters.
Attachment #8933507 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933845 [details]
Bug 1421088 - Create a WidgetInfo struct and route drawing through it.

https://reviewboard.mozilla.org/r/204780/#review216846

::: widget/cocoa/nsNativeThemeCocoa.mm:3237
(Diff revision 1)
>        break;
>  
>      case NS_THEME_TREETWISTY:
> -      DrawButton(cgContext, macRect,
> +      widgetInfo = Some(WidgetInfo::Button(
> -                 ButtonParams{ComputeControlParams(aFrame, eventState),
> +        ButtonParams{ComputeControlParams(aFrame, eventState),
> -                              ButtonType::eTreeTwistyPointingRight});
> +                    ButtonType::eTreeTwistyPointingRight}));

nit: the vertical alignment seems off, here and elsewhere in this file.

::: widget/cocoa/nsNativeThemeCocoa.mm:3370
(Diff revision 1)
> +    }
> +  }
> +
> +  if (widgetInfo) {
> +    switch (widgetInfo->Widget()) {
> +      case Widget::eColorFill: {

nit: we should either use braces after case statements throughout, or not at all.

::: widget/cocoa/nsNativeThemeCocoa.mm:3373
(Diff revision 1)
> +  if (widgetInfo) {
> +    switch (widgetInfo->Widget()) {
> +      case Widget::eColorFill: {
> +        Color params = widgetInfo->Params<Color>();
> +        SetCGContextFillColor(cgContext, params);
> +        CGContextFillRect(cgContext, macRect);

I may be missing something, but shouldn't we be calling DrawVibrancyBackground here for VibrancyFillColors?
Attachment #8933845 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933846 [details]
Bug 1421088 - Move rendering code out into a separate method.

https://reviewboard.mozilla.org/r/204782/#review217148
Attachment #8933846 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8933847 [details]
Bug 1421088 - Move code out of DrawWidgetBackground into a new method called ComputeWidgetInfo.

https://reviewboard.mozilla.org/r/204784/#review217150

::: widget/cocoa/nsNativeThemeCocoa.mm:2976
(Diff revision 2)
>            ButtonParams{ComputeControlParams(aFrame, eventState),
>                         ButtonType::eSquareBezelPushButton}));
> -      } else {
> -        widgetInfo = Some(WidgetInfo::Button(
> +      }
> +      return Some(WidgetInfo::Button(
> -          ButtonParams{ComputeControlParams(aFrame, eventState),
> +        ButtonParams{ComputeControlParams(aFrame, eventState),
> -                       ButtonType::eRoundedBezelPushButton}));
> +                      ButtonType::eRoundedBezelPushButton}));

nit: vertical alignment for ButtonParams is off in various places
Attachment #8933847 - Flags: review?(spohl.mozilla.bugs) → review+
Phew! Thanks for your patience while I worked through the reviews here, Markus.
Thanks for the patience while reviewing all of this! I'm going to start addressing the review comments now.
Comment on attachment 8933492 [details]
Bug 1421088 - Split DrawPushButton into four functions and group the frame-dependent information into a struct.

https://reviewboard.mozilla.org/r/204424/#review212688

> Shouldn't this be:
> 
> [aCell setHighlighted:aControlParams.insideActiveWindow && aControlParams.pressed)];
> 
> or was this an intentional change?

It was partly intentional, partly indifferent. I think I ended up using ApplyControlParamsToNSCell for multiple kinds of widgets, and those weren't completely consistent about what value they passed to setHighlighted, so I picked the simplest one.
If you click on a control in an inactive window, the mousedown event turns the window active, so there's usually no way to hit the case where pressed is true and insideActiveWindow is false.

> Has DRAW_IN_FRAME_DEBUG outlived its usefulness? Should we remove it throughout?

It probably has. Filed bug 1454096.

> We're now checking macRect.size.height instead of nativeWidgetHeight. nativeWidgetHeight is a rounded value of nativeWidgetRect.Height() (and possibly multiplied by 0.5f if on hidpi) while macRect.size.height is not rounded. If this is an intentional change we should remove nativeWidgetHeight as it is no longer used.

Good catch, this was a mistake. To make matters more confusing, the "Create a WidgetInfo struct" patch actually reverted this change.
Comment on attachment 8933494 [details]
Bug 1421088 - Give nsNativeThemeCocoa::DrawButton a different API.

https://reviewboard.mozilla.org/r/204428/#review212728

> The existing code would be equivalent to:
> 
> bool isDefaultButton = IsDefaultButton(aFrame) && !eventState.HasState(NS_EVENT_STATE_ACTIVE);
> 
> Do we not need to check the active state for bevel buttons?

This was an unintentional change, but it doesn't seem to matter: I tested this combination by setting style="-moz-appearance: button-bevel" on a default button, and the default button adornment was completely ignored. Moreover, the only use of -moz-appearance: button-bevel in our codebase is for the button that launches the color picker. Those buttons are never default buttons. I filed bug 1454200 about removing this button type.
Comment on attachment 8933498 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawCheckboxOrRadio.

https://reviewboard.mozilla.org/r/204436/#review212692

> nit: s/inCheckbox/aIsCheckbox/
> 
> (This can be fixed in the additional patch that fixes all prefixes in these two files, as discussed via IRC)

I'll probably do the parameter renaming in a follow-up bug.

> I see that highlighting here does not depend on aParams.controlParams.insideActiveWindow being true, while it did in DrawPushButton. Just pointing this out to make sure we're intentionally consolidating the two to ignore aParams.controlParams.insideActiveWindow.

Good catch, this is the issue that I addressed in a comment on the other patch. It's intentional.
Comment on attachment 8933500 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawSpinButton / DrawSpinButtons.

https://reviewboard.mozilla.org/r/204440/#review212756

> This should be kThemeIncDecButton, no?

Good catch! You're right.
Comment on attachment 8933501 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawSegment.

https://reviewboard.mozilla.org/r/204442/#review216304

> nit: keeping the formatting identical helped for the review. let's change it to <= 80 characters before landing.

Changed to use NSDictionary literal syntax @{ key: value } which made it a lot clearer.
Comment on attachment 8933505 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawProgress.

https://reviewboard.mozilla.org/r/204450/#review216332

> Should we be passing |!IsVerticalProgress(aFrame)| instead of |true| to ComputeProgressParams() here, or is this guaranteed to always be true for NS_THEME_PROGRESSBAR since vertical progressbars are handled by NS_THEME_PROGRESSBAR_VERTICAL?

Yes, we should be passing !IsVerticalProgress(aFrame) here. HTML <progress> elements use NS_THEME_PROGRESSBAR for both horizontal and vertical progress bars.
Comment on attachment 8933507 [details]
Bug 1421088 - Don't pass an nsIFrame* to DrawScale, and create DrawScrollbarTrack and DrawScrollbarThumb.

https://reviewboard.mozilla.org/r/204454/#review216812

> Previously, we wouldn't call DrawScale at all if rangeFrame was NULL. Is calling DrawScale with ScaleParams() guaranteed to lead to the same behavior?

Good catch. I've changed the function to return a Maybe<> and the caller to not call DrawScale if Nothing() is returned.
Comment on attachment 8933845 [details]
Bug 1421088 - Create a WidgetInfo struct and route drawing through it.

https://reviewboard.mozilla.org/r/204780/#review216846

> I may be missing something, but shouldn't we be calling DrawVibrancyBackground here for VibrancyFillColors?

This patch replaces many calls to DrawVibrancyBackground with uses of WidgetInfo::ColorFill. DrawVibrancyBackground was really just a color fill for the case where aCornerRadiusIfOpaque is zero, which is the case for all these replaced instances, so this change preserves behavior.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/3718eabad30e
Simplify search field cell management and make the "is-within-toolbar" computation eager instead of lazy. r=spohl
https://hg.mozilla.org/integration/autoland/rev/03a6d5b78824
Split DrawPushButton into four functions and group the frame-dependent information into a struct. r=spohl
https://hg.mozilla.org/integration/autoland/rev/bb00ac312fd2
Create a separate method for drawing tree header cells. r=spohl
https://hg.mozilla.org/integration/autoland/rev/f91c55d12fcc
Give nsNativeThemeCocoa::DrawButton a different API. r=spohl
https://hg.mozilla.org/integration/autoland/rev/d7a371dd0062
Route more button types through DrawButton. r=spohl
https://hg.mozilla.org/integration/autoland/rev/7d5825e53354
Move menu background drawing into a separate method. r=spohl
https://hg.mozilla.org/integration/autoland/rev/9fdfc9fd39e5
Change the API of DrawMenuIcon and add DrawMenuItem. r=spohl
https://hg.mozilla.org/integration/autoland/rev/50dff2a5018d
Don't pass an nsIFrame* to DrawCheckboxOrRadio. r=spohl
https://hg.mozilla.org/integration/autoland/rev/430dc21c9d09
Don't pass an nsIFrame* to DrawDropdown. r=spohl
https://hg.mozilla.org/integration/autoland/rev/e531083a8507
Don't pass an nsIFrame* to DrawSpinButton / DrawSpinButtons. r=spohl
https://hg.mozilla.org/integration/autoland/rev/6c79b2b7c515
Don't pass an nsIFrame* to DrawSegment. r=spohl
https://hg.mozilla.org/integration/autoland/rev/20858ebf5ca3
Don't pass an NSWindow* to DrawUnifiedToolbar. r=spohl
https://hg.mozilla.org/integration/autoland/rev/67a8548cf31c
Rename DrawFrame to DrawTextBox and stop passing an nsIFrame* to it. r=spohl
https://hg.mozilla.org/integration/autoland/rev/ab2ff6cc2bbc
Don't pass an nsIFrame* to DrawSearchField. r=spohl
https://hg.mozilla.org/integration/autoland/rev/36adcc259a83
Don't pass an nsIFrame* to DrawProgress. r=spohl
https://hg.mozilla.org/integration/autoland/rev/add23e8bcdf9
Don't pass an nsIFrame* to DrawMeter. r=spohl
https://hg.mozilla.org/integration/autoland/rev/7ce6baa5953f
Don't pass an nsIFrame* to DrawScale, and create DrawScrollbarTrack and DrawScrollbarThumb. r=spohl
https://hg.mozilla.org/integration/autoland/rev/c1ae9f757238
Don't pass an nsIFrame* to DrawTabPanel. r=spohl
https://hg.mozilla.org/integration/autoland/rev/162bca0e0090
Remove unused parameters from DrawFocusOutline. r=spohl
https://hg.mozilla.org/integration/autoland/rev/fca91b7851fa
Don't pass an nsIFrame* to DrawStatusBar. r=spohl
https://hg.mozilla.org/integration/autoland/rev/903d6e8ae860
Don't pass an nsIFrame* to DrawResizer. r=spohl
https://hg.mozilla.org/integration/autoland/rev/29fa9b230754
Create DrawMultilineTextField. r=spohl
https://hg.mozilla.org/integration/autoland/rev/8ec637889522
Create DrawToolbar. r=spohl
https://hg.mozilla.org/integration/autoland/rev/9e10ce956b57
Create DrawMenuSeparator. r=spohl
https://hg.mozilla.org/integration/autoland/rev/66258c83a953
Create DrawSourceList. r=spohl
https://hg.mozilla.org/integration/autoland/rev/f60cf13a310f
Create a DrawNativeTitlebar overload that takes a UnifiedToolbarParams param. r=spohl
https://hg.mozilla.org/integration/autoland/rev/32da46a98a6d
Create a WidgetInfo struct and route drawing through it. r=spohl
https://hg.mozilla.org/integration/autoland/rev/90617339a4d1
Move rendering code out into a separate method. r=spohl
https://hg.mozilla.org/integration/autoland/rev/86f9b0d937ea
Move code out of DrawWidgetBackground into a new method called ComputeWidgetInfo. r=spohl
https://hg.mozilla.org/mozilla-central/rev/3718eabad30e
https://hg.mozilla.org/mozilla-central/rev/03a6d5b78824
https://hg.mozilla.org/mozilla-central/rev/bb00ac312fd2
https://hg.mozilla.org/mozilla-central/rev/f91c55d12fcc
https://hg.mozilla.org/mozilla-central/rev/d7a371dd0062
https://hg.mozilla.org/mozilla-central/rev/7d5825e53354
https://hg.mozilla.org/mozilla-central/rev/9fdfc9fd39e5
https://hg.mozilla.org/mozilla-central/rev/50dff2a5018d
https://hg.mozilla.org/mozilla-central/rev/430dc21c9d09
https://hg.mozilla.org/mozilla-central/rev/e531083a8507
https://hg.mozilla.org/mozilla-central/rev/6c79b2b7c515
https://hg.mozilla.org/mozilla-central/rev/20858ebf5ca3
https://hg.mozilla.org/mozilla-central/rev/67a8548cf31c
https://hg.mozilla.org/mozilla-central/rev/ab2ff6cc2bbc
https://hg.mozilla.org/mozilla-central/rev/36adcc259a83
https://hg.mozilla.org/mozilla-central/rev/add23e8bcdf9
https://hg.mozilla.org/mozilla-central/rev/7ce6baa5953f
https://hg.mozilla.org/mozilla-central/rev/c1ae9f757238
https://hg.mozilla.org/mozilla-central/rev/162bca0e0090
https://hg.mozilla.org/mozilla-central/rev/fca91b7851fa
https://hg.mozilla.org/mozilla-central/rev/903d6e8ae860
https://hg.mozilla.org/mozilla-central/rev/29fa9b230754
https://hg.mozilla.org/mozilla-central/rev/8ec637889522
https://hg.mozilla.org/mozilla-central/rev/9e10ce956b57
https://hg.mozilla.org/mozilla-central/rev/66258c83a953
https://hg.mozilla.org/mozilla-central/rev/f60cf13a310f
https://hg.mozilla.org/mozilla-central/rev/32da46a98a6d
https://hg.mozilla.org/mozilla-central/rev/90617339a4d1
https://hg.mozilla.org/mozilla-central/rev/86f9b0d937ea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Regressions: 1659046
You need to log in before you can comment on or make changes to this bug.