1313285 - Remove nsVideoFrame::mBorderPadding

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

When reviewing bug 1271765, I noticed we have this member-variable nsVideoFrame::mBorderPadding, which is set during Reflow() and only used during Reflow().

Pretty sure it can just be removed, and its usages can just be replaced by the thing that we set it from (aReflowInput.ComputedPhysicalBorderPadding())

ralin, since you've been working on this file, perhaps you'd be up for taking a crack at this? (or perhaps you noticed a usage of this variable outside of ::Reflow that I missed?)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> When reviewing bug 1271765, I noticed we have this member-variable
> nsVideoFrame::mBorderPadding, which is set during Reflow() and only used
> during Reflow().

Backing up this statement: this DXR search...
  https://dxr.mozilla.org/mozilla-central/search?q=%2Bvar-ref%3AnsVideoFrame%3A%3AmBorderPadding
...shows that all of the references to this variable are within lines 305 and 378 of nsVideoFrame.cpp, which are all within the Reflow implementation.

Comment 2

[Ray Lin[:ralin]]

No problem, I'd like to fix this. Should this base on bug 1271765?

Comment 3

[Daniel Holbert [:dholbert]]

Yup, that's what I'd suggest. Thanks!  I'll go ahead and set you as the assignee, but feel free to reset that if you can't get to this after all. (I expect this should be pretty trivial, though.)

Comment 4

[Ray Lin[:ralin]]

Attachment: Bug 1313285 - remove nsVideoFrame::mBorderPadding.

Review commit: https://reviewboard.mozilla.org/r/95550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95550/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8814299 [details]
Bug 1313285 - remove nsVideoFrame::mBorderPadding.

https://reviewboard.mozilla.org/r/95550/#review95606

Looks good -- thanks! Just one nit:

::: layout/generic/nsVideoFrame.cpp:313
(Diff revision 1)
>    if (!isBSizeShrinkWrapping) {
>      borderBoxBSize = contentBoxBSize +
>        aReflowInput.ComputedLogicalBorderPadding().BStartEnd(myWM);
>    }
>  
> -  mBorderPadding   = aReflowInput.ComputedPhysicalBorderPadding();
> +  nsMargin borderPadding   = aReflowInput.ComputedPhysicalBorderPadding();

While you're modifying this line, you might as well drop the extra space characters before the "=".

(I assume they were there for alignment with some adjacent assignments at one point in the past -- but there's no reason to have them now.)

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8814299 [details]
Bug 1313285 - remove nsVideoFrame::mBorderPadding.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95550/diff/1-2/

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8814299 [details]
Bug 1313285 - remove nsVideoFrame::mBorderPadding.

https://reviewboard.mozilla.org/r/95550/#review95606

> While you're modifying this line, you might as well drop the extra space characters before the "=".
> 
> (I assume they were there for alignment with some adjacent assignments at one point in the past -- but there's no reason to have them now.)

fixed now. I'll flag checkin-needed once passed on try server.

Thanks for your prompt review :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3a0522f6dd8
remove nsVideoFrame::mBorderPadding. r=dholbert

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3a0522f6dd8