Closed Bug 1461222 Opened 2 years ago Closed 2 years ago

Make GetWidgetBorder/GetWidgetPadding return LayoutDeviceIntMargin

Categories

(Core :: Widget, enhancement, trivial)

enhancement
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(2 files)

GetWidgetBorder/GetWidgetPadding is currently using nsIntMargin
which is ambiguous since it doesn't have an associated unit.
We should use LayoutDeviceIntMargin instead to make it clear
the result is in device pixels.
Comment on attachment 8975367 [details] [diff] [review]
part 1 - Make GetWidgetBorder return LayoutDeviceIntMargin

Review of attachment 8975367 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for doing this. Every time I had to touch widget-related code I ended up having to triple-check units :)

::: widget/gtk/nsNativeThemeGTK.cpp
@@ +1408,5 @@
> +        // XXX will be removed in next part...
> +        aResult->top = border.top;
> +        aResult->right = border.right;
> +        aResult->bottom = border.bottom;
> +        aResult->left = border.left;

nit: Looks like this could really go away in this part, right? (GetCachedWidgetBorder is changed in this patch too). But not a big deal.
Attachment #8975367 - Flags: review?(emilio) → review+
Comment on attachment 8975368 [details] [diff] [review]
part 2 - Make GetWidgetPadding return LayoutDeviceIntMargin

Review of attachment 8975368 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you :)

::: layout/generic/ReflowInput.cpp
@@ +2546,5 @@
>  
>    const nsStyleDisplay* disp = mFrame->StyleDisplayWithOptionalParam(aDisplay);
>    bool isThemed = mFrame->IsThemed(disp);
>    bool needPaddingProp;
> +  LayoutDeviceIntMargin padding;

nit: The name `padding` can be confusing with `aPadding` too. Maybe `widgetPadding` would be a better name?

@@ +2551,5 @@
>    if (isThemed &&
>        presContext->GetTheme()->GetWidgetPadding(presContext->DeviceContext(),
>                                                  mFrame, disp->mAppearance,
> +                                                &padding)) {
> +    auto p2a = presContext->AppUnitsPerDevPixel();

nit: In other places you're just passing presContext->AppUnitsPerDevPixel() as a parameter, maybe worth being consistent?

::: layout/generic/nsFrame.cpp
@@ +1403,1 @@
>      if (presContext->GetTheme()->GetWidgetPadding(presContext->DeviceContext(),

nit: same remark regarding the variable name (there's another `padding` variable in scope). But again not a big deal.
Attachment #8975368 - Flags: review?(emilio) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> nit: Looks like this could really go away in this part, right?
> (GetCachedWidgetBorder is changed in this patch too). But not a big deal.

No, aResult is still nsIntMargin* here.  It changes in part 2.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Comment on attachment 8975368 [details] [diff] [review]
> part 2 - Make GetWidgetPadding return LayoutDeviceIntMargin

Fixed the nits.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff167076903f
part 1 - Make GetWidgetBorder return LayoutDeviceIntMargin. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97d25723b34
part 2 - Make GetWidgetPadding return LayoutDeviceIntMargin. r=emilio
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/ff167076903f
https://hg.mozilla.org/mozilla-central/rev/a97d25723b34
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.