Closed Bug 1313285 Opened 3 years ago Closed 3 years ago

Remove nsVideoFrame::mBorderPadding

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: dholbert, Assigned: ralin)

Details

Attachments

(1 file)

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?)
(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.
No problem, I'd like to fix this. Should this base on bug 1271765?
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.)
Assignee: nobody → ralin
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.)
Attachment #8814299 - Flags: review?(dholbert) → review+
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 :)
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/f3a0522f6dd8
remove nsVideoFrame::mBorderPadding. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3a0522f6dd8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.