Remove nsVideoFrame::mBorderPadding

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dholbert, Assigned: ralin)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
(Assignee)

Comment 2

2 years ago
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 hidden (mozreview-request)
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 hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
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 :)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 8

2 years ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/f3a0522f6dd8
remove nsVideoFrame::mBorderPadding. r=dholbert
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3a0522f6dd8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.