Closed
Bug 1421088
Opened 6 years ago
Closed 6 years ago
Prepare nsNativeThemeCocoa for remote theme rendering
Categories
(Core :: Widget: Cocoa, enhancement, P2)
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8933502 -
Flags: review?(spohl.mozilla.bugs)
Assignee | ||
Comment 18•6 years ago
|
||
I've pushed the roughly the first half of the patches to review. The rest is coming tomorrow.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Priority: -- → P2
Comment 38•6 years ago
|
||
mozreview-review |
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 39•6 years ago
|
||
mozreview-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 40•6 years ago
|
||
mozreview-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 41•6 years ago
|
||
mozreview-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 42•6 years ago
|
||
mozreview-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 43•6 years ago
|
||
mozreview-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 44•6 years ago
|
||
mozreview-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 45•6 years ago
|
||
mozreview-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 46•6 years ago
|
||
mozreview-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 47•6 years ago
|
||
mozreview-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 48•6 years ago
|
||
mozreview-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 49•6 years ago
|
||
mozreview-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 50•6 years ago
|
||
mozreview-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 51•6 years ago
|
||
mozreview-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 52•6 years ago
|
||
mozreview-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 53•6 years ago
|
||
mozreview-review |
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 54•6 years ago
|
||
mozreview-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 55•6 years ago
|
||
mozreview-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 56•6 years ago
|
||
mozreview-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 57•6 years ago
|
||
mozreview-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 58•6 years ago
|
||
mozreview-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 59•6 years ago
|
||
mozreview-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 60•6 years ago
|
||
mozreview-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 61•6 years ago
|
||
mozreview-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 62•6 years ago
|
||
mozreview-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 63•6 years ago
|
||
mozreview-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 64•6 years ago
|
||
mozreview-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 65•6 years ago
|
||
mozreview-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 66•6 years ago
|
||
mozreview-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 67•6 years ago
|
||
mozreview-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+
Comment 68•6 years ago
|
||
Phew! Thanks for your patience while I worked through the reviews here, Markus.
Assignee | ||
Comment 69•6 years ago
|
||
Thanks for the patience while reviewing all of this! I'm going to start addressing the review comments now.
Assignee | ||
Comment 70•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 71•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 72•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 73•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 74•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 104•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 105•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 106•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 122•6 years ago
|
||
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
Comment 123•6 years ago
|
||
bugherder |
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
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox59:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•