1424382 - Use BaseRect methods instead of directly member variables in layout/

Status: NEW

(Core :: Graphics, enhancement, P3)

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

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

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review217590

First wave of review feedback, for how far I got before lunch (I haven't looked past nsCanvasFrame.cpp yet):

::: layout/base/PresShell.cpp:5001
(Diff revision 1)
>                                 const LayoutDeviceIntPoint aPoint,
>                                 LayoutDeviceIntRect* aScreenRect,
>                                 uint32_t aFlags)
>  {
>    nsPresContext* pc = GetPresContext();
> -  if (!pc || aArea.width == 0 || aArea.height == 0)
> +  if (!pc || aArea.Width() == 0 || aArea.Height() == 0)

Maybe use aArea.IsZero() here?

::: layout/base/nsLayoutUtils.cpp:1106
(Diff revision 1)
>        float top = margins.top * scale;
>        float bottom = margins.bottom * scale;
> -      screenRect.y -= top;
> -      screenRect.height += top + bottom;
> +      screenRect.SetRectY(screenRect.Y() - top,
> +                          screenRect.Height() + top + bottom);

I think this would be more readable/grokkable if you used SetBoxY instead of SetRectY, like so:

  screenRect.SetBoxY(screenRect.Y() - top,
                     screenRect.YMost() + bottom);

That makes it clearer that 'top' and 'bottom' are just offsets for our current Y and YMost. (The "Height() + top + bottom" arg in the current patch is a bit harder for me to reason about in the abstract, without doing a quick sketch.)

::: layout/base/nsLayoutUtils.cpp:1117
(Diff revision 1)
>        float left = margins.left * scale;
>        float right = margins.right * scale;
> -      screenRect.x -= left;
> -      screenRect.width += left + right;
> +      screenRect.SetRectX(screenRect.X() - left,
> +                          screenRect.Width() + left + right);

Similarly, I think this could be simplified to:
  screenRect.SetBoxX(screenRect.X() - left,
                     screenRect.XMost() + right);

::: layout/base/nsLayoutUtils.cpp:7243
(Diff revision 1)
>  
>    Maybe<SVGImageContext> svgContext(Some(SVGImageContext(Some(aImageSize))));
>    SVGImageContext::MaybeStoreContextPaint(svgContext, aForFrame, aImage);
>  
>    /* Fast path when there is no need for image spacing */
> -  if (aRepeatSize.width == aDest.width && aRepeatSize.height == aDest.height) {
> +  if (aRepeatSize.width == aDest.Width() && aRepeatSize.height == aDest.Height()) {

I believe this could be simplified (using BaseRect access methods) to this:
  if (aRepeatSize == aDest.Size()) {

...or if that doesn't work for some reason, this (a bit clumsier but still shorter than the current code):
  if (aDest.IsEqualSize(aRepeatSize.width, aRepeatSize.height)) {

::: layout/base/nsLayoutUtils.cpp:8963
(Diff revision 1)
> -    contentBounds.width += aScrollableFrame->GetScrollPortRect().width;
> -    contentBounds.height += aScrollableFrame->GetScrollPortRect().height;
> +    contentBounds.SizeTo(contentBounds.Width() + aScrollableFrame->GetScrollPortRect().Width(),
> +                         contentBounds.Height() + aScrollableFrame->GetScrollPortRect().Height());

This seems needlessly repetitive (old code says "contentBounds" 2 times vs. new code says it 3 times).

Could we instead do the following to only have to say contentBounds once:
 contentBounds +=
  nsSize(aScrollableFrame->GetScrollPortRect().Width(),
         aScrollableFrame->GetScrollPortRect().Height());
...using BaseRect's operator+ overload that takes a SizeT, here:
https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/gfx/2d/BaseRect.h#361

(Note that contentBounds is of type "nsRect" here.)

::: layout/forms/nsComboboxControlFrame.cpp:1612
(Diff revision 1)
> -  clipRect.width -= onePixel;
> -  clipRect.height -= onePixel;
> +  clipRect.SetWidth(clipRect.Width() - onePixel);
> +  clipRect.SetHeight(clipRect.Height() - onePixel);

As above, I think we could avoid introducing variable-name repetition here by using an overloaded arithmetic method:
  clipRect -= nsSize(onePixel, onePixel);

(clipRect is of type nsRect)

::: layout/generic/TextOverflow.cpp:261
(Diff revision 1)
>  void
>  nsDisplayTextOverflowMarker::PaintTextToContext(gfxContext* aCtx,
>                                                  nsPoint aOffsetFromRect)
>  {
>    WritingMode wm = mFrame->GetWritingMode();
> -  nsPoint pt(mRect.x, mRect.y);
> +  nsPoint pt(mRect.X(), mRect.Y());

If you like, this seems more direct & less repetitive:
  nsPoint pt = mRect.TopLeft();

::: layout/generic/nsBulletFrame.cpp:807
(Diff revision 1)
>        if (imageCon) {
>          nsRect dest(padding.left, padding.top,
> -                    mRect.width - (padding.left + padding.right),
> -                    mRect.height - (padding.top + padding.bottom));
> +                    mRect.Width() - (padding.left + padding.right),
> +                    mRect.Height() - (padding.top + padding.bottom));

While you're here, maybe simplify this to use padding.LeftRight() and padding.TopBottom() as well? Like so:

        nsRect dest(padding.left, padding.top,
                    mRect.Width() - padding.LeftRight(),
                    mRect.Height() - padding.TopBottom());

(I'll avoid ticking "open an issue" for this one, since technically it wouldn't be a BaseRect API usage -- it's a BaseMargin usage -- so maybe you'd prefer to do it separately.)

::: layout/generic/nsBulletFrame.cpp:831
(Diff revision 1)
> -                  mRect.width - (padding.left + padding.right),
> -                  mRect.height - (padding.top + padding.bottom));
> +                  mRect.Width() - (padding.left + padding.right),
> +                  mRect.Height() - (padding.top + padding.bottom));
>        auto devPxRect = LayoutDeviceRect::FromAppUnits(rect, appUnitsPerDevPixel);

Here, too, we could replace the parenthesized expressions with padding.LeftRight() and padding.TopBottom()

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review217638

Second wave of review feedback (I got up to the start of nsCSSRendering::ExpandPaintingRectForDecorationLine)

::: layout/generic/nsGfxScrollFrame.cpp:5008
(Diff revision 1)
> +     nscoord scY = mHelper.mScrollPort.Y();
> +     nscoord scWidth = mHelper.mScrollPort.Height();
>       bool fit = AddRemoveScrollbar(hasHorizontalScrollbar,
> -                                   mHelper.mScrollPort.y,
> +                                   scY, scWidth,
> -                                   mHelper.mScrollPort.height,
>                                     hSize.height, aOnRightOrBottom, aAdd);
> +     mHelper.mScrollPort.SetRectY(scY, scWidth);

Naming typo -- s/scWidth/scHeight/

(Probably copypaste error from the similar x-axis-oriented chunk further down)

::: layout/generic/nsGfxScrollFrame.cpp:5820
(Diff revision 1)
>      vRect = mScrollPort;
>      if (overlayScrollBarsWithZoom) {
> -      vRect.height = NSToCoordRound(res * scrollPortClampingSize.height);
> +      vRect.SetHeight(NSToCoordRound(res * scrollPortClampingSize.height));
>      }
> -    vRect.width = aContentArea.width - mScrollPort.width;
> -    vRect.x = scrollbarOnLeft ? aContentArea.x : mScrollPort.x + NSToCoordRound(res * scrollPortClampingSize.width);
> +    vRect.SetWidth(aContentArea.Width() - mScrollPort.Width());
> +    vRect.MoveToX(scrollbarOnLeft ? aContentArea.X() : mScrollPort.X() + NSToCoordRound(res * scrollPortClampingSize.width));

This line was already too long in mozilla-central, but it's getting significantly longer in this patch (9 chars longer, to 126 chars, which even full-screen MozReview can't show without arbitrary-midline-wrapping).

Let's rewrap it to get it under 80 chars. I think this works:

    vRect.MoveToX(scrollbarOnLeft
                  ? aContentArea.X()
                  : mScrollPort.X() +
                    NSToCoordRound(res * scrollPortClampingSize.width));

::: layout/generic/nsImageFrame.cpp:1648
(Diff revision 1)
>        // If the target size is pretty small, no point in using a layer.
> -      if (destLayerRect.width * destLayerRect.height < 64 * 64) {
> +      if (destLayerRect.Width() * destLayerRect.Height() < 64 * 64) {

This should probably use destLayerRect.Area() ?

::: layout/generic/nsImageMap.cpp:298
(Diff revision 1)
> -    r.width -= kOnePixel;
> -    r.height -= kOnePixel;
> +    r.SizeTo(r.Width() - kOnePixel,
> +             r.Height() - kOnePixel);

This can simplify to:
  r -= nsSize(kOnePixel, kOnePixel);

::: layout/generic/nsTextFrame.cpp:7805
(Diff revision 1)
>      nsPoint point = GetPointFromIterator(iter, properties);
>      nsRect rect;
> -    rect.x = point.x;
> +    rect.MoveTo(point.x, point.y);
> -    rect.y = point.y;

Simplify to:
  rect.MoveTo(point);

::: layout/generic/nsTextFrame.cpp:9916
(Diff revision 1)
>    gfxTextRun::Metrics textMetrics =
>      mTextRun->MeasureText(ComputeTransformedRange(provider),
>                            gfxFont::LOOSE_INK_EXTENTS, nullptr,
>                            &provider);
>    if (GetWritingMode().IsLineInverted()) {
> -    textMetrics.mBoundingBox.y = -textMetrics.mBoundingBox.YMost();
> +    textMetrics.mBoundingBox.MoveByY(-textMetrics.mBoundingBox.YMost());

This should be MoveToY ("To", not "By"), since it's replacing an assignment.

::: layout/mathml/nsMathMLChar.cpp:2299
(Diff revision 1)
> -        clipRect.y = dy;
> -        clipRect.height = std::min(bm.ascent + bm.descent, fillEnd - dy);
> +        clipRect.SetRectY(dy, 
> +                          std::min(bm.ascent + bm.descent, fillEnd - dy));

drop trailing whitespace after "dy," on first line here.

::: layout/mathml/nsMathMLChar.cpp:2464
(Diff revision 1)
>        while (dx < fillEnd) {
> -        clipRect.x = dx;
> +        clipRect.SetRectX(dx, std::min(bm.rightBearing - bm.leftBearing, fillEnd - dx));
> -        clipRect.width = std::min(bm.rightBearing - bm.leftBearing, fillEnd - dx);

This line (from combining two expressions into one) will end up too long -- 89 chars I think.

Maybe wrap after the second comma? Like so:

        clipRect.SetRectX(dx, std::min(bm.rightBearing - bm.leftBearing,
                                       fillEnd - dx));

::: layout/painting/MaskLayerImageCache.h:109
(Diff revision 1)
>      // Create a hash for this object.
>      PLDHashNumber Hash() const
>      {
> -      PLDHashNumber hash = HashBytes(&mRect.x, 4*sizeof(gfxFloat));
> +      auto mRectX = mRect.X();
> +      PLDHashNumber hash = HashBytes(&mRectX, 4*sizeof(gfxFloat));

With the current patch, this will hash uninitialized stack memory  (starting with the local variable that is sneakily called "mRectX", despite not being a member, and then 3 gfxFloats' worth of memory after that).

As discussed in IRC -- for the purposes of this bug, it's probably fine to just copy all the members into a local array and then hash that array... but longer-term it'd be nice to avoid the copying & use some BaseRect::Hash() function or equivalent.  Maybe file a followup and refer to it in a // XXX code-comment here?

::: layout/painting/nsCSSRendering.cpp:201
(Diff revision 1)
>      if (mVertical) {
> -      if (joinedBorderArea.y > mPIStartBorderData.mCoord) {
> -        joinedBorderArea.y =
> +      if (joinedBorderArea.Y() > mPIStartBorderData.mCoord) {
> +        joinedBorderArea.MoveByY(-(mUnbrokenMeasure - aBorderArea.Height()));
> -          -(mUnbrokenMeasure + joinedBorderArea.y - aBorderArea.height);

I'm pretty sure this change is wrong...

The old code is effectively doing the following assignment (which I've tweaked by pushing the negation through the parens):
  joinedBorderArea.y =
    -mUnbrokenMeasure +
    -joinedBorderArea.y +
    aBorderArea.height;

I don't think that can be expressed as MoveByY() (+=), since the "self" term (joinedBorderArea.y) is being negated.

So I think this needs to be a straight MoveToY(...) where "..." is just the right side of the assignment in the old code.

::: layout/painting/nsCSSRendering.cpp:208
(Diff revision 1)
> -      if (joinedBorderArea.x > mPIStartBorderData.mCoord) {
> -        joinedBorderArea.x =
> +      if (joinedBorderArea.X() > mPIStartBorderData.mCoord) {
> +        joinedBorderArea.MoveByX(-(mUnbrokenMeasure - aBorderArea.Width()));
> -          -(mUnbrokenMeasure + joinedBorderArea.x - aBorderArea.width);

Same here -- this can't be expressed as MoveByX since the "self" term is negated in the old code.

So I think this needs to be MoveToX.

::: layout/painting/nsCSSRendering.cpp:1732
(Diff revision 1)
>          if (skipSides.Left()) {
> -          nscoord xmost = fragmentClip.XMost();
> +          fragmentClip.SetBoxX(aFrameArea.X(), fragmentClip.XMost());
> -          fragmentClip.x = aFrameArea.x;
> -          fragmentClip.width = xmost - fragmentClip.x;
>          }

Instead of using SetBoxX, I think you can simplify to:
  fragmentClip.SetLeftEdge(aFrameArea.X());

(Pretty sure this is equivalent to [but simpler than] what you've got.  It looks to me like the "xmost" business in the old code here is just trying to keep the right edge unmoved, IIUC.)

::: layout/painting/nsCSSRendering.cpp:1735
(Diff revision 1)
>          if (skipSides.Right()) {
> -          nscoord xmost = fragmentClip.XMost();
> +          nscoord overflow = fragmentClip.XMost() - aFrameArea.XMost();
> -          nscoord overflow = xmost - aFrameArea.XMost();
>            if (overflow > 0) {
> -            fragmentClip.width -= overflow;
> +            fragmentClip.SetWidth(fragmentClip.Width() - overflow);
>            }

This would be more clearly expressed as:
  fragmentClip.SetRightEdge(std::min(fragmentClip.XMost(),
                                     aFrameArea.XMost()));

With this change & the others suggested here, this whole cascade ends up being
 if (skipSides.$SIDE()) {
   fragmentClip.Set[$SIDE]Edge(...)
 }
which is quite nice to grok & reason about.

::: layout/painting/nsCSSRendering.cpp:1741
(Diff revision 1)
>          if (skipSides.Top()) {
> -          nscoord ymost = fragmentClip.YMost();
> +          fragmentClip.SetBoxY(aFrameArea.Y(), fragmentClip.YMost());
> -          fragmentClip.y = aFrameArea.y;
> -          fragmentClip.height = ymost - fragmentClip.y;
>          }

As above, I think you can simplify this SetBoxY()  call to:
  fragmentClip.SetTopEdge(aFrameArea.Y());

::: layout/painting/nsCSSRendering.cpp:1744
(Diff revision 1)
>          if (skipSides.Bottom()) {
> -          nscoord ymost = fragmentClip.YMost();
> +          nscoord overflow = fragmentClip.YMost() - aFrameArea.YMost();
> -          nscoord overflow = ymost - aFrameArea.YMost();
>            if (overflow > 0) {
> -            fragmentClip.height -= overflow;
> +            fragmentClip.SetHeight(fragmentClip.Height() - overflow);
>            }

As above for the "right" case, I think this can be:
  fragmentClip.SetBottomEdge(std::min(fragmentClip.YMost(),
                                      aFrameArea.YMost()));

::: layout/painting/nsCSSRendering.cpp:3436
(Diff revision 1)
> -  poly[1].x = aRect.x + aRect.width;
> -  poly[1].y = aRect.y;
> -  poly[2].x = aRect.x + aRect.width;
> -  poly[2].y = aRect.y + aRect.height;
> -  poly[3].x = aRect.x;
> -  poly[3].y = aRect.y + aRect.height;
> +  poly[1].x = aRect.X() + aRect.Width();
> +  poly[1].y = aRect.Y();
> +  poly[2].x = aRect.X() + aRect.Width();
> +  poly[2].y = aRect.Y() + aRect.Height();
> +  poly[3].x = aRect.X();
> +  poly[3].y = aRect.Y() + aRect.Height();

These probably want to be simplified to XMost() and YMost() instead of X() + Width() etc.

::: layout/painting/nsCSSRendering.cpp:3687
(Diff revision 1)
> -          rect.x += startBevel;
> -          rect.width -= startBevel;
> +          rect.MoveByX(startBevel);
> +          rect.SetWidth(rect.Width() - startBevel);

These two lines can collapse to:
  rect.SetLeftEdge(rect.X() + startBevel);

::: layout/painting/nsCSSRendering.cpp:3689
(Diff revision 1)
>          }
>          if (eSideTop == aEndBevelSide) {
> -          rect.width -= endBevel;
> +          rect.SetWidth(rect.Width() - endBevel);

...and if you like, for symmetry, this could become:
  rect.SetRightEdge(rect.XMost() - startBevel);

(I think this'll be strictly simpler under the hood, once we change rects to store extents, too, since the right edge / "xmost" will be literally what we'll storing, IIUC.)

::: layout/painting/nsCSSRendering.cpp:3701
(Diff revision 1)
> -        rect.width = half;
> +        rect.SetWidth(half);
>          if (eSideLeft == aStartBevelSide) {
> -          rect.y += startBevel;
> -          rect.height -= startBevel;
> +          rect.MoveByY(startBevel);
> +          rect.SetHeight(rect.Height() - startBevel);
>          }
>          if (eSideLeft == aEndBevelSide) {
> -          rect.height -= endBevel;
> +          rect.SetHeight(rect.Height() - endBevel);

As above, this can become SetTopEdge / SetBottomEdge, I think.

::: layout/painting/nsCSSRendering.cpp:3724
(Diff revision 1)
>          if (eSideBottom == aStartBevelSide) {
> -          rect.x += startBevel;
> -          rect.width -= startBevel;
> +          rect.MoveByX(startBevel);
> +          rect.SetWidth(rect.Width() - startBevel);
>          }
>          if (eSideBottom == aEndBevelSide) {
> -          rect.width -= endBevel;
> +          rect.SetWidth(rect.Width() - endBevel);

As above, the first clause probably wants to be:
   rect.SetLeftEdge(rect.X() + startBevel);
...and the second clause:
   rect.SetRightEdge(rect.XMost() - endBevel);

::: layout/painting/nsCSSRendering.cpp:3740
(Diff revision 1)
> -          rect.y += aStartBevelOffset - startBevel;
> -          rect.height -= startBevel;
> +          rect.MoveByY(aStartBevelOffset - startBevel);
> +          rect.SetHeight(rect.Height() - startBevel);

(Unlike the clauses above, this one doesn't look like it can be simplified, though... we're doing something more complex than moving a single edge here. I tried to decompose this into a rect-movement & an edge movement but couldn't come up with something meaningful.)

::: layout/painting/nsCSSRendering.cpp:3775
(Diff revision 1)
> -          topRect.x += aStartBevelOffset - startBevel;
> -          topRect.width -= aStartBevelOffset - startBevel;
> +          topRect.MoveByX(aStartBevelOffset - startBevel);
> +          topRect.SetWidth(topRect.Width() - aStartBevelOffset - startBevel);

I don't think this is quite right. The old code is doing:
  a -= b - c;
...and you're replacing that with:
  a = a - b - c;
...which isn't a correct conversion.  You need parens around "b - c" to make it valid.

But really, since the old code adds the same quantity to "x" as it subtracts from "width", I think this wants to be a SetLeftEdge(...) operation? Something like:
  topRect.SetLeftEdge(topRect.X() + aStartBevelOffset - startBevel);

::: layout/painting/nsCSSRendering.cpp:3779
(Diff revision 1)
>          if (eSideTop == aStartBevelSide) {
> -          topRect.x += aStartBevelOffset - startBevel;
> -          topRect.width -= aStartBevelOffset - startBevel;
> +          topRect.MoveByX(aStartBevelOffset - startBevel);
> +          topRect.SetWidth(topRect.Width() - aStartBevelOffset - startBevel);
>          }
>          if (eSideTop == aEndBevelSide) {
> -          topRect.width -= aEndBevelOffset - endBevel;
> +          topRect.SetWidth(topRect.Width() - aEndBevelOffset - endBevel);

As above, this isn't a correct conversion. (Though it would be if you had parens around "aEndBevelOffset - endBevel")

Also, maybe this really wants to be a SetRightEdge(topRect.XMost() - (...)) expression, for symmetry with the clause directly above (after that clause's review feedback is addressed)?

::: layout/painting/nsCSSRendering.cpp:3790
(Diff revision 1)
> -          bottomRect.x += aStartBevelOffset - startBevel;
> -          bottomRect.width -= aStartBevelOffset - startBevel;
> +          bottomRect.MoveByX(aStartBevelOffset - startBevel);
> +          bottomRect.SetWidth(bottomRect.Width() - aStartBevelOffset - startBevel);
>          }
>          if (eSideBottom == aEndBevelSide) {
> -          bottomRect.width -= aEndBevelOffset - endBevel;
> +          bottomRect.SetWidth(bottomRect.Width() - aEndBevelOffset - endBevel);
>          }

As above, the subtraction conversions here are wrong, and these might really want to be using the Set***Edge() APIs.

::: layout/painting/nsCSSRendering.cpp:3807
(Diff revision 1)
> -          leftRect.y += aStartBevelOffset - startBevel;
> -          leftRect.height -= aStartBevelOffset - startBevel;
> +          leftRect.MoveByY(aStartBevelOffset - startBevel);
> +          leftRect.SetHeight(leftRect.Height() - aStartBevelOffset - startBevel);
>          }
>          if (eSideLeft == aEndBevelSide) {
> -          leftRect.height -= aEndBevelOffset - endBevel;
> +          leftRect.SetHeight(leftRect.Height() - aEndBevelOffset - endBevel);

As above, subtraction conversion looks wrong, & consider if you can use edge-setting APIs.

::: layout/painting/nsCSSRendering.cpp:3821
(Diff revision 1)
> -          rightRect.y += aStartBevelOffset - startBevel;
> -          rightRect.height -= aStartBevelOffset - startBevel;
> +          rightRect.MoveByY(aStartBevelOffset - startBevel);
> +          rightRect.SetHeight(rightRect.Height() - aStartBevelOffset - startBevel);
>          }
>          if (eSideRight == aEndBevelSide) {
> -          rightRect.height -= aEndBevelOffset - endBevel;
> +          rightRect.SetHeight(rightRect.Height() - aEndBevelOffset - endBevel);

As above, subtraction conversion looks wrong, & consider if you can use edge-setting APIs.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> 
> ::: layout/generic/nsGfxScrollFrame.cpp:5008
> ...
> 
> Naming typo -- s/scWidth/scHeight/

Ha!  Good point.  Logically correct, but bad name.


> 
> ::: layout/painting/MaskLayerImageCache.h:109
> (Diff revision 1)
> >      // Create a hash for this object.
> >      PLDHashNumber Hash() const
> >      {
> > -      PLDHashNumber hash = HashBytes(&mRect.x, 4*sizeof(gfxFloat));
> > +      auto mRectX = mRect.X();
> > +      PLDHashNumber hash = HashBytes(&mRectX, 4*sizeof(gfxFloat));
> 
> With the current patch, this will hash uninitialized stack memory  (starting
> with the local variable that is sneakily called "mRectX", despite not being
> a member, and then 3 gfxFloats' worth of memory after that).
> 
> As discussed in IRC -- for the purposes of this bug, it's probably fine to
> just copy all the members into a local array and then hash that array... but
> longer-term it'd be nice to avoid the copying & use some BaseRect::Hash()
> function or equivalent.  Maybe file a followup and refer to it in a // XXX
> code-comment here?

It will be handled in bug 1429792 and if that lands as proposed, we'll do the right thing right away.

> 
> With this change & the others suggested here, this whole cascade ends up
> being
>  if (skipSides.$SIDE()) {
>    fragmentClip.Set[$SIDE]Edge(...)
>  }
> which is quite nice to grok & reason about.

Nice!

> 
> ::: layout/painting/nsCSSRendering.cpp:3689
> (Diff revision 1)
> >          }
> >          if (eSideTop == aEndBevelSide) {
> > -          rect.width -= endBevel;
> > +          rect.SetWidth(rect.Width() - endBevel);
> 
> ...and if you like, for symmetry, this could become:
>   rect.SetRightEdge(rect.XMost() - startBevel);

endBevel, but, yes, I like it.

Thanks!

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1424382: Part 2. Use BaseRect access methods instead of member variables in layout/ (involved cases).

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

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

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

Comment 7

[Daniel Holbert [:dholbert]]

(More review feedback coming shortly, from the end of the patch, before the latest update. I'm pretty sure it'll all be applicable, as long as the end of the patch didn't change much.)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review217888

::: layout/painting/nsCSSRendering.cpp:4088
(Diff revision 1)
> -      Float& rectICoord = aParams.vertical ? rect.y : rect.x;
> +      const Float rectBSize = aParams.vertical ? rect.Width() : rect.Height();
> -      Float& rectISize = aParams.vertical ? rect.height : rect.width;
> -      const Float rectBSize = aParams.vertical ? rect.width : rect.height;
>  
>        const Float adv = rectBSize - lineThickness;
>        const Float flatLengthAtVertex =
>          std::max((lineThickness - 1.0) * 2.0, 1.0);
>  
>        // Align the start of wavy lines to the nearest ancestor block.
>        const Float cycleLength = 2 * (adv + flatLengthAtVertex);
>        rect = ExpandPaintingRectForDecorationLine(aFrame, aParams.style, rect,

I'm not sure your changes to this function are valid.

In particular, here's what I'm seeing at a high level here:
 - In the old code, we cache the *original* rectICoord/rectISize values early on, and then we potentially replace |rect| (when we call rect = ExpandPaintingRectForDecorationLine(...)), and then we do some arithmetic with our original cached rectICoord and rectISize variables.

 - In the new code, we *don't bother caching the original values of those variables*, and we do arithmetic on |rect| directly (using the values that have been provided by ExpandPaintingRectForDecorationLine())

So it looks like this might change behavior? (maybe for the better even, I'm not sure.  But even so: if this piece is potentially intentionally changing behavior, it'd be better to do that in its own patch rather than in this megapatch)

::: layout/painting/nsCSSRenderingBorders.cpp:1204
(Diff revision 1)
>    if ((aSides & (eSideBitsTop | eSideBitsLeft)) == (eSideBitsTop | eSideBitsLeft)) {
>      // adjust the left's top down a bit
> -    r[eSideLeft].y += aBorderSizes[eSideTop];
> -    r[eSideLeft].height -= aBorderSizes[eSideTop];
> +    r[eSideLeft].MoveByY(aBorderSizes[eSideTop]);
> +    r[eSideLeft].SetHeight(r[eSideLeft].Height() - aBorderSizes[eSideTop]);
>    }

These two calls should collapse to a single r[eSideLeft].SetTopEdge call, I think (which matches what the comment suggests we're doing).  Like so:
  r[eSideLeft].SetTopEdge(r[eSideLeft].Y() +
                          aBorderSizes[eSideTop]);

::: layout/painting/nsCSSRenderingBorders.cpp:1220
(Diff revision 1)
>    if ((aSides & (eSideBitsBottom | eSideBitsLeft)) == (eSideBitsBottom | eSideBitsLeft)) {
>      // adjust the bottom's left a bit
> -    r[eSideBottom].x += aBorderSizes[eSideLeft];
> -    r[eSideBottom].width -= aBorderSizes[eSideLeft];
> +    r[eSideBottom].MoveByX(aBorderSizes[eSideLeft]);
> +    r[eSideBottom].SetWidth(r[eSideBottom].Width() - aBorderSizes[eSideLeft]);

Similarly, I think this wants to collapse to "SetLeftEdge".

::: layout/painting/nsDisplayList.cpp:9229
(Diff revision 1)
>  nsDisplaySVGEffects::HitTest(nsDisplayListBuilder* aBuilder, const nsRect& aRect,
>                               HitTestState* aState, nsTArray<nsIFrame*> *aOutFrames)
>  {
> -  nsPoint rectCenter(aRect.x + aRect.width / 2, aRect.y + aRect.height / 2);
> +  nsPoint rectCenter(aRect.Center());
>    if (nsSVGIntegrationUtils::HitTestFrameForEffects(mFrame,
>        rectCenter - ToReferenceFrame())) {
>      mList.HitTest(aBuilder, aRect, aState, aOutFrames);
>    }
>  }

Probably better to just drop the local variable "rectCenter" altogether, & just use aRect.Center() directly instead of the local variable. (It's basically the same number of characters now, and we only use it once.)

::: layout/svg/nsSVGOuterSVGFrame.cpp:581
(Diff revision 1)
>    nsPoint pointRelativeToContentBox =
> -    nsPoint(aRect.x + aRect.width / 2, aRect.y + aRect.height / 2) -
> +    nsPoint(aRect.X() + aRect.Width() / 2, aRect.Y() + aRect.Height() / 2) -
>        refFrameToContentBox;

I'm pretty sure this "nsPoint(...)" expression that you're modifying here can just be further simplified to aRect.Center(), right?

::: layout/svg/nsSVGUtils.cpp:1036
(Diff revision 1)
>      if (NS_STYLE_CLIP_RIGHT_AUTO & effects->mClipFlags) {
> -      clipRect.width = aWidth - clipRect.X();
> +      clipRect.SetWidth(aWidth - clipRect.X());
>      }
>      if (NS_STYLE_CLIP_BOTTOM_AUTO & effects->mClipFlags) {
> -      clipRect.height = aHeight - clipRect.Y();
> +      clipRect.SetHeight(aHeight - clipRect.Y());
>      }

These SetWidth/SetHeight calls really want to be:
 clipRect.SetRightEdge(aWidth)
 clipRect.SetBottomEdge(aHeight)
...which are shorter & have the benefit of matching the "if (...$SIDE...) { Set$SIDEEdge(...) } pattern.

::: layout/xul/ScrollBoxObject.cpp:198
(Diff revision 1)
>         // In the left-to-right case we scroll so that the left edge of the
>         // selected child is scrolled to the left edge of the scrollbox.
>         // In the right-to-left case we scroll so that the right edge of the
>         // selected child is scrolled to the right edge of the scrollbox.
>  
> -       nsPoint pt(isLTR ? rect.x : rect.x + rect.width - frameWidth,
> +       nsPoint pt(isLTR ? rect.X() : rect.X() + rect.Width() - frameWidth,

s/rect.X() + rect.Width()/rect.XMost()/

::: layout/xul/ScrollBoxObject.cpp:209
(Diff revision 1)
>     } else {
>         // Use a destination range that ensures the top edge will be visible.
> -       nsRect range(cp.x, rect.y - csspixel, 0, csspixel);
> -       sf->ScrollTo(nsPoint(cp.x, rect.y), nsIScrollableFrame::INSTANT, &range);
> +     nsRect range(cp.x, rect.Y() - csspixel, 0, csspixel);
> +     sf->ScrollTo(nsPoint(cp.x, rect.Y()), nsIScrollableFrame::INSTANT, &range);

This tweak is inadevertantly de-indenting part of this clause (but only part of it).

Let's restore the original (4-space) indentation for these lines, to keep it consistent with the other code at the same logic-nesting level in this same function.

::: layout/xul/nsGroupBoxFrame.cpp:234
(Diff revision 1)
>      // draw right side
>      clipRect = rect;
> -    clipRect.x = groupRect.XMost();
> -    clipRect.width = rect.XMost() - groupRect.XMost();
> +    clipRect.MoveToX(groupRect.XMost());
> +    clipRect.SizeTo(rect.XMost() - groupRect.XMost(), border.top);
> -    clipRect.height = border.top;

I think this code could be rewritten a little more intuitively using the SetBoxX() method (probably more efficient after the internal-representation change, too). Specifically, this seems a bit simpler (no handwritten arithmetic needed) and I think is equivalent (though please sanity-check me):

  clipRect.SetBoxX(groupRect.XMost(), rect.XMost());
  clipRect.SetHeight(border.top);

::: layout/xul/nsGroupBoxFrame.cpp:250
(Diff revision 1)
>      clipRect = rect;
> -    clipRect.y += border.top;
> +    clipRect.SetRectY(border.top, mRect.Height() - (yoff + border.top));
> -    clipRect.height = mRect.height - (yoff + border.top);

The "SetRectY(border.top, ...)" is wrong here.  The old code is *adding* border.top to our "y" value, rather than directly setting it.

So I think this wants to be using MoveByY and SetHeight, or alternately it needs "clipRect.Y() +" inserted just inside the SetRectY(...) parens.

::: layout/xul/nsResizerFrame.cpp:117
(Diff revision 1)
> +          mMouseDownRect.SetRect( mouseDownRectX, mouseDownRectY,
> +                                  mouseDownRectW, mouseDownRectH);

Drop stray space after "SetRect( ", as well as on the next line to preserve alignment.

::: layout/xul/tree/nsTreeBodyFrame.cpp:2426
(Diff revision 1)
>      image->GetWidth(&coord);
> -    r.width = nsPresContext::CSSPixelsToAppUnits(coord);
>      image->GetHeight(&coord);
> -    r.height = nsPresContext::CSSPixelsToAppUnits(coord);
> +    r.SizeTo(nsPresContext::CSSPixelsToAppUnits(coord),
> +             nsPresContext::CSSPixelsToAppUnits(coord));

This patch is breaking the logic here by doing the code reordering.  The old code uses a single variable ("coord") to represent two possibly-different quantities.  And the new code is throwing away the first of those quantities without using it.

Maybe just replace the single local variable "coord" with two variables "width" and "height?  (Or you could restore this closer to the original code's ordering & just use interleaved SetWidth & SetHeight calls)

::: layout/xul/tree/nsTreeBodyFrame.cpp:3139
(Diff revision 1)
>      // Paint the right side (whole) separator.
>      nsRect separatorRect(rowRect);
> -    if (primaryX > rowRect.x) {
> -      separatorRect.width -= primaryX - rowRect.x;
> -      separatorRect.x += primaryX - rowRect.x;
> +    if (primaryX > rowRect.X()) {
> +      separatorRect.SetWidth(separatorRect.Width() - primaryX - rowRect.X());
> +      separatorRect.MoveByX(primaryX - rowRect.X());

I think you can collapse these SetWidth/MoveByX calls to a single SetLeftEdge() call.

(since we're adding the same quantity to the X-coord as we're subtracting from the width)

::: layout/xul/tree/nsTreeBodyFrame.cpp:3885
(Diff revision 1)
> -  if (imageSize.height > checkboxRect.height)
> -    imageSize.height = checkboxRect.height;
> +  imageSize.SizeTo(std::min(imageSize.Height(), checkboxRect.Height()),
> +                   std::min(imageSize.Width(), checkboxRect.Width()));
> -  if (imageSize.width > checkboxRect.width)

The SizeTo args need to be swapped here.

(SizeTo() expects Width first, and then height)

Comment 9

[Daniel Holbert [:dholbert]]

I made it all the way through the original patch now, and my comments are all above. I'll leave it to you to make the changes in whichever of the now-split patches are appropriate.

Let me know when that's done and I'll take a final look, probably by folding the patches together & using a merge tool to compare that against the patch that I reviewed before. For now, I'll mark both patches r-, since there's work to be done & since otherwise mozreview won't let you send updated "review request" notifications.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review217998

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review218000

::: commit-message-d5f42:1
(Diff revision 2)
> +Bug 1424382: Part 1. Simpler cases of BaseRect access methods instead of direct member variable access. r?dholbert

This commit message doesn't make sense, out of context.

I think you want it to say something like:
"Part 1. Use BaseRect access methods instead of member variables in layout/ (simpler cases)"

And then Part 2 would have the same wording, except "(files with complex cases)" -- since at first glance Part 2 includes some simple cases too, but it's maybe just covering *the files* that include any non-trivial stuff (?)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8941971 [details]
Bug 1424382: Part 2. Use BaseRect access methods instead of member variables in layout/ (involved cases).

https://reviewboard.mozilla.org/r/212158/#review218004

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, the part 1 vs. part 2 split was files.  I didn't want dependency between them.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

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

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8941971 [details]
Bug 1424382: Part 2. Use BaseRect access methods instead of member variables in layout/ (involved cases).

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

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b37d29c6603f6737e22fc69d9a1e3134cc4ea72

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review230338

This is really close! BTW, it looks like MozReview still (incorrectly) thinks all of my old review feedback is still relevant, so it shows 40+ open issues on this patch.  You can close out those (in theory you'd do that as you addressed them).  Though maybe if you've already addressed all of those issues, it's easier not to bother, and just ultimately land on inbound so that mozreview won't force you to click a button 40+ times. :)

Anyway, if it helps: my new feedback (the comments I'm leaving now) should all say "Diff revision 3" (they do in my UI, at least).

::: commit-message-d5f42:1
(Diff revision 3)
> +Bug 1424382: Part 1. Use BaseRect access methods instead of member variables in layout/ (simpler cases). r?dholbert
> +* * *
> +Bug 1424382
> +* * *
> +Bug 1424382 Part 3
> +* * *
> +Bug 1424382 More
> +* * *
> +Bug 1424382 More
> +* * *
> +Bug 1424382

Let's get rid of these bonus commit message lines. (typically these are artifacts from "hg qfold" operations)

::: layout/base/PresShell.cpp:8338
(Diff revision 3)
>    // caret coordinates are in app units, convert to pixels
>    aTargetPt.x =
> -    presContext->AppUnitsToDevPixels(caretCoords.x + caretCoords.width);
> +    presContext->AppUnitsToDevPixels(caretCoords.X() + caretCoords.Width());
>    aTargetPt.y =
> -    presContext->AppUnitsToDevPixels(caretCoords.y + caretCoords.height);
> +    presContext->AppUnitsToDevPixels(caretCoords.Y() + caretCoords.Height());
>  

This should take advantage of:
  caretCoords.XMost()
  caretCoords.YMost()

::: layout/generic/nsHTMLCanvasFrame.cpp:401
(Diff revision 3)
> -  nsRect r;
> -  r.x = bp.left;
> -  r.y = bp.top;
> +  return nsRect(bp.left, bp.top,
> +                mRect.Width() - bp.left - bp.right,
> +                mRect.Height() - bp.top - bp.bottom);

Instead of subtracting left and right, just subtract
  bp.LeftRight()

...and similarly:
  bp.TopBottom()

::: layout/mathml/nsMathMLChar.cpp:2171
(Diff revision 3)
>      else if (2 == i) { // bottom
> -      dy = aRect.y + aRect.height - bm.descent;
> +      dy = aRect.Y() + aRect.Height() - bm.descent;

Let's use aRect.YMost() here.

::: layout/painting/FrameLayerBuilder.cpp:2544
(Diff revision 3)
> -      layerContentsVisible = Rect(
> -          aLayerContentsVisibleRect->x, aLayerContentsVisibleRect->y,
> -          aLayerContentsVisibleRect->width, aLayerContentsVisibleRect->height);
> +      layerContentsVisible = Rect(aLayerContentsVisibleRect->X(),
> +                                  aLayerContentsVisibleRect->Y(),
> +                                  aLayerContentsVisibleRect->Width(),
> +                                  aLayerContentsVisibleRect->Height());
> +        ;

Drop the stray semicolon-on-its-own-line here.

::: layout/painting/nsCSSRendering.cpp:4087
(Diff revision 3)
> -      Float& rectICoord = aParams.vertical ? rect.y : rect.x;
> +      const Float rectBSize = aParams.vertical ? rect.Width() : rect.Height();
> -      Float& rectISize = aParams.vertical ? rect.height : rect.width;

tl;dr: can you revert the changes to this function, and punt them to a followup bug (or followup patch on this same bug, if you prefer), so that they don't hold up the rest of this patch?

Longer: I'm not sure I like how this chunk is ending up, readability-wise. I haven't reviewed it for correctness, but I can see in a quick skim that before this patch, we're doing the reasoning in terms of inline-axis/block-axis coordinates & sizes -- whereas after the patch, we're kind of doing a mixture of logical vs. physical coordinates (rectBSize, dirtyRectICoord, mixed with MoveByX / MoveByY / SetTopEdge / etc).  I recognize that this chunk can't be a straightforward x -> X() substitution, since the old code is using C++ references to internal components of the Rect. But I wonder if the best solution would really be to just promote these C++ references to legitimate local copies (with ICoord/ISize/BCoord/BSize in their naming), and then we should just do a SetRect() operation at the end to "commit" the final computed sizes back into the rect...?

I'm not sure that would work, but it seems feasible. But as noted in the tl;dr, I think this should happen separately from the rest of this patch, since it's significantly less trivial/uncontroversial.

::: layout/painting/nsCSSRenderingBorders.cpp:2140
(Diff revision 3)
>          } else {
>            from = tmp;
>          }
>        }
>      }
> -    Float right = mDirtyRect.x + mDirtyRect.width + radius + AA_MARGIN;
> +    Float right = mDirtyRect.X() + mDirtyRect.Width() + radius + AA_MARGIN;

Use mDirtyRect.XMost() here (and YMost() below this -- there are 4 places total in this function that want that treatment, 2 XMost and 2 YMost.)

::: layout/painting/nsCSSRenderingBorders.cpp:3531
(Diff revision 3)
>    const nscoord borderX[3] = {
> -    mArea.x + 0,
> -    mArea.x + mWidths.left,
> -    mArea.x + mArea.width - mWidths.right,
> +    mArea.X() + 0,
> +    mArea.X() + mWidths.left,
> +    mArea.X() + mArea.Width() - mWidths.right,
>    };
>    const nscoord borderY[3] = {
> -    mArea.y + 0,
> -    mArea.y + mWidths.top,
> -    mArea.y + mArea.height - mWidths.bottom,
> +    mArea.Y() + 0,
> +    mArea.Y() + mWidths.top,
> +    mArea.Y() + mArea.Height() - mWidths.bottom,
>    };

Final line of these arrays should use mArea.XMost() and mArea.YMost()

::: layout/painting/nsDisplayList.cpp:4102
(Diff revision 3)
>      // Calculate the scaling factor for the frame.
> -    const gfxSize scale = gfxSize(destLayerRect.width / imageWidth,
> -                                  destLayerRect.height / imageHeight);
> +    const gfxSize scale = gfxSize(destLayerRect.Width() / imageWidth,
> +                                  destLayerRect.Height() / imageHeight);
>  
>      if ((scale.width != 1.0f || scale.height != 1.0f) &&
> -        (destLayerRect.width * destLayerRect.height >= 64 * 64)) {
> +        (destLayerRect.Width() * destLayerRect.Height() >= 64 * 64)) {

.Area()

::: layout/svg/nsSVGOuterSVGFrame.cpp:582
(Diff revision 3)
>  
>    nsPoint refFrameToContentBox =
>      ToReferenceFrame() + outerSVGFrame->GetContentRectRelativeToSelf().TopLeft();
>  
>    nsPoint pointRelativeToContentBox =
> -    nsPoint(aRect.x + aRect.width / 2, aRect.y + aRect.height / 2) -
> +    nsPoint(aRect.X() + aRect.Width() / 2, aRect.Y() + aRect.Height() / 2) -

I think this nsPoint(....) expression can be simplified to "aRect.Center()"

::: layout/xul/ScrollBoxObject.cpp:198
(Diff revision 3)
>         // In the left-to-right case we scroll so that the left edge of the
>         // selected child is scrolled to the left edge of the scrollbox.
>         // In the right-to-left case we scroll so that the right edge of the
>         // selected child is scrolled to the right edge of the scrollbox.
>  
> -       nsPoint pt(isLTR ? rect.x : rect.x + rect.width - frameWidth,
> +       nsPoint pt(isLTR ? rect.X() : rect.X() + rect.Width() - frameWidth,

The right side of the ":" should simplify to
  rect.XMost() - frameWidth

::: layout/xul/tree/nsTreeBodyFrame.cpp:3124
(Diff revision 3)
>        if (level == 0)
>          currX += mIndentation;
>  
> -      if (currX > rowRect.x) {
> +      if (currX > rowRect.X()) {
>          nsRect separatorRect(rowRect);
> -        separatorRect.width -= rowRect.x + rowRect.width - currX;
> +        separatorRect.SetWidth(separatorRect.Width() - rowRect.XMost() - currX);

[Logic error]: I think you're accidentally changing the result of the arithmetic here.  You need to wrap the last two terms in parens -- so this should end up looking like:

  separatorRect.SetWidth(separatorRect.Width() - (rowRect.XMost() - currX));

(Or alternately/equivalently, you could change the final "-" to a "+" and not add parens. But I think the parens version is closer to what we have currently & probably closer to the original developer's mental formulation.)

::: layout/xul/tree/nsTreeBodyFrame.cpp:3133
(Diff revision 3)
>      }
>  
>      // Paint the right side (whole) separator.
>      nsRect separatorRect(rowRect);
> -    if (primaryX > rowRect.x) {
> -      separatorRect.width -= primaryX - rowRect.x;
> +    if (primaryX > rowRect.X()) {
> +      separatorRect.SetWidth(separatorRect.Width() - primaryX - rowRect.X());

[Logic error]: Here too, you need parens around the last two terms. Should be:
 separatorRect.SetWidth(separatorRect.Width() - (primaryX - rowRect.X()));

::: layout/xul/tree/nsTreeBodyFrame.cpp:3859
(Diff revision 3)
> -  if (imageSize.height > checkboxRect.height)
> -    imageSize.height = checkboxRect.height;
> +  imageSize.SizeTo(std::min(imageSize.Height(), checkboxRect.Height()),
> +                   std::min(imageSize.Width(), checkboxRect.Width()));

[Logic error]: The SizeTo() args here are in the wrong order. (The width line should come first, and then the height line.)

The original code happened to clamp height before it clamped width (which didn't make a difference).  But now that we'll be using SizeTo(), we have to pass in the width term before the height term, or else we'll accidentally swap dimensions!

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8941971 [details]
Bug 1424382: Part 2. Use BaseRect access methods instead of member variables in layout/ (involved cases).

https://reviewboard.mozilla.org/r/212158/#review230788

Here's my partial review for "part 2". (For my own reference when I pick this back up: I got up to the beginning of nsMenuPopupFrame.cpp.)

::: layout/base/PresShell.cpp:3428
(Diff revision 2)
> -      nscoord maxHeight;
> +      nscoord maxHeight, rangeY;
>        scrollPt.y = ComputeWhereToScroll(aVertical.mWhereToScroll,
>                                          scrollPt.y,
> -                                        aRect.y,
> +                                        aRect.Y(),
>                                          aRect.YMost(),
> -                                        visibleRect.y,
> +                                        visibleRect.Y(),
>                                          visibleRect.YMost(),
> -                                        &allowedRange.y, &maxHeight);
> -      allowedRange.height = maxHeight - allowedRange.y;
> +                                        &rangeY, &maxHeight);
> +      allowedRange.SetBoxY(rangeY, maxHeight);

Please rename the helper-variables to "rangeYMin" and "rangeYMax".

(That's basically what they're named inside the ComputeWhereToScroll function...
https://dxr.mozilla.org/mozilla-central/rev/df0e967f8e8fa30a69338008b6ee296bf5e9c545/layout/base/PresShell.cpp#3358,3365-3366
...and it makes the "SetBoxY" API-usage make more sense here.

Without that change, this "SetBoxY(fooY, fooHeight)" *smells* like it's a misusage of the API -- it seems like it wants to be using SetRectY() instead. But in fact the API is correct, and the problem is that the maxHeight variable is just confusingly named.)

::: layout/base/PresShell.cpp:3452
(Diff revision 2)
> -      nscoord maxWidth;
> +      nscoord maxWidth, rangeX;
>        scrollPt.x = ComputeWhereToScroll(aHorizontal.mWhereToScroll,
>                                          scrollPt.x,
> -                                        aRect.x,
> +                                        aRect.X(),
>                                          aRect.XMost(),
> -                                        visibleRect.x,
> +                                        visibleRect.X(),
>                                          visibleRect.XMost(),
> -                                        &allowedRange.x, &maxWidth);
> -      allowedRange.width = maxWidth - allowedRange.x;
> +                                        &rangeX, &maxWidth);
> +      allowedRange.SetBoxX(rangeX, maxWidth);

As above, please name the helper-vars "rangeXMin" and "rangeXMax".

::: layout/base/nsLayoutUtils.cpp:1135
(Diff revision 2)
> -      screenRect.x -= left;
> -      screenRect.width += left + right;
> +      screenRect.SetBoxX(screenRect.X() - left,
> +                          screenRect.XMost() + right);

Fix indentation. (screenRect.XMost() is over-indented here.)

::: layout/generic/nsTextFrame.cpp:8890
(Diff revision 2)
>    nsRect r;
> -  r.x = NSToCoordFloor(aRect.X());
> -  r.y = NSToCoordFloor(aRect.Y());
> -  r.width = NSToCoordCeil(aRect.XMost()) - r.x;
> +  r.MoveTo(NSToCoordFloor(aRect.X()), NSToCoordFloor(aRect.Y()));
> +  r.SizeTo(NSToCoordCeil(aRect.XMost()) - r.X(),
> +           NSToCoordCeil(aRect.YMost()) - r.Y());
> -  r.height = NSToCoordCeil(aRect.YMost()) - r.y;
>    return r;

I think this can be simplified to the following (for a single function call & less explicit arithmetic):

  r.SetBox(NSToCoordFloor(aRect.X()),
           NSToCoordFloor(aRect.Y()),
           NSToCoordCeil(aRect.XMost()),
           NSToCoordCeil(aRect.YMost()));

(Please sanity-check that & let me know if I'm missing something though...)

::: layout/painting/nsCSSRendering.cpp:3615
(Diff revision 2)
> -        nsRect rect(aBorder.x, aBorder.y, startDashLength, aBorder.height);
> +        nsRect rect(aBorder.X(), aBorder.Y(), startDashLength, aBorder.Height());
>          DrawSolidBorderSegment(aDrawTarget, rect, aBorderColor,
>                                 aAppUnitsPerDevPixel);
>  
> -        rect.x += startDashLength + dashLength;
> +        rect.SetRectX(rect.X() + startDashLength + dashLength,

Please replace "rect.X() + startDashLength" with "rect.XMost()".

(It just so happens that startDashLength == rect.width, so this replacement is valid.  See the declaration of "rect" a few lines up, which has startDashLength as its 3rd arg, which sets rect.width.)

This makes this a bit more brief & readable, and it ends up more consistent with your rewrite of the "Y-axis" version of this code, ~20 lines further down (which uses YMost() in its spot like this).

::: layout/painting/nsCSSRenderingBorders.cpp:1196
(Diff revision 2)
>    if ((aSides & (eSideBitsTop | eSideBitsLeft)) == (eSideBitsTop | eSideBitsLeft)) {
>      // adjust the left's top down a bit
> -    r[eSideLeft].y += aBorderSizes[eSideTop];
> -    r[eSideLeft].height -= aBorderSizes[eSideTop];
> +    r[eSideLeft].MoveByY(aBorderSizes[eSideTop]);
> +    r[eSideLeft].SetHeight(r[eSideLeft].Height() - aBorderSizes[eSideTop]);
>    }

Let's use SetTopEdge/SetLeftEdge/SetBottomEdge/SetLeftEdge in this section (starting here & the 3 clauses below this)

All of the comments here are of the form "Adjust the $RECT's $EDGE", which seems like they want to be using the Set***Edge()-setting functions. Plus, the first and last clause (the ones that adjust x/y position) are particularly good candidates, since they're adding the same thing to x (or y) as they're subtracting from width (or height).

(Also: this clued me into the fact that there's a preexisting comment typo in this section -- see my next review-nit on that.)

::: layout/painting/nsCSSRenderingBorders.cpp:1202
(Diff revision 2)
>    if ((aSides & (eSideBitsTop | eSideBitsRight)) == (eSideBitsTop | eSideBitsRight)) {
>      // adjust the top's left a bit
> -    r[eSideTop].width -= aBorderSizes[eSideRight];
> +    r[eSideTop].SetWidth(r[eSideTop].Width() - aBorderSizes[eSideRight]);

This preexisting comment is almost certainly a typo. I think it needs s/left/right/ -- it should say:
 "adjust the top's right a bit"

Please make that fix (s/left/right/) -- that'll avoid causing confusion when you address my previous nit and make us call "SetRightEdge(...)" here.


(Justification for why I think this comment is a typo:
 - we're subtracting from this rect's width, which means we're adjusting its right side, not its left side.
 - Plus, the bitmask in the "if" condition is about "top" and "right", not "left".  And in the 3 related clauses alongside this one, the bitmask's sides match the comment's sides.)

::: layout/svg/nsSVGImageFrame.cpp:376
(Diff revision 2)
>    Rect rect;
>    SVGImageElement *element = static_cast<SVGImageElement*>(GetContent());
> -  element->GetAnimatedLengthValues(&rect.x, &rect.y,
> -                                   &rect.width, &rect.height, nullptr);
> +  auto rectX = rect.X();
> +  auto rectY = rect.Y();
> +  auto rectW = rect.Width();
> +  auto rectH = rect.Height();
> +  element->GetAnimatedLengthValues(&rectX, &rectY, &rectW, &rectH, nullptr);
> +  rect.SetRect(rectX, rectY, rectW, rectH);

It's a bit silly of us to be reading out the values of all of "rect's" default-initialized members here, copying those default values into local variables that we're just going to stomp on anyway (in GetAnimatedLengthValues).

Let's just follow the example from elsewhere in the file, e.g. here: https://dxr.mozilla.org/mozilla-central/rev/df0e967f8e8fa30a69338008b6ee296bf5e9c545/layout/svg/nsSVGImageFrame.cpp#260-262

...and just rewrite this like so:
  float x, y, width, height;
  element->GetAnimatedLengthValues(&x, &y, &width, &height, nullptr);
  Rect rect(x, y, width, height);


(Notably: moving the decl of "rect" further down, after we've determined its components via our GetAnimatedLengthValues call.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8941971 [details]
Bug 1424382: Part 2. Use BaseRect access methods instead of member variables in layout/ (involved cases).

https://reviewboard.mozilla.org/r/212158/#review231034

OK, I finished getting through "part 2".  Just a few more nits to add to those in my previous bug-comment:

::: layout/xul/nsMenuPopupFrame.cpp:1754
(Diff revision 2)
> -      if (!dontOverlapOSBar && mMenuCanOverlapOSBar && !mInContentShell)
> -        screen->GetRect(&screenRectPixels.x, &screenRectPixels.y,
> -          &screenRectPixels.width, &screenRectPixels.height);
> -      else
> +      auto screenRectPixelsX = screenRectPixels.X();
> +      auto screenRectPixelsY = screenRectPixels.Y();
> +      auto screenRectPixelsW = screenRectPixels.Width();
> +      auto screenRectPixelsH = screenRectPixels.Height();

It's a bit misleading that we're copying screenRectPixels.X() etc. here -- that implies that the "screenRectPixels" rect might already contain useful values at this point (it doesn't), & that we *care* about the values we're reading out (we don't - we stomp on them right below this).

So: let's just declare these as
 int32_t screenRectPixelsX = 0;
 int32_t screenRectPixelsY = 0;
 int32_t screenRectPixelsW = 0;
 int32_t screenRectPixelsH = 0;
(default-initializing to 0 so that we've got reasonable fallback in case the nsIScreen APIs fail)

(This is equivalent to what you've got -- since the rect is zero-initialized -- it's just a bit more direct this way & avoids giving the impression that the rect has useful data inside it.)

::: layout/xul/nsResizerFrame.cpp:116
(Diff revision 2)
> +          mMouseDownRect.SetRect( mouseDownRectX, mouseDownRectY,
> +                                  mouseDownRectW, mouseDownRectH);

Drop the leading space character inside of "SetRect( "

(and deindent following line to keep alignment after that changed)

::: layout/xul/nsSprocketLayout.cpp:1029
(Diff revision 2)
> -
> -      //nscoord childLayoutHeight = GET_HEIGHT(aChildLayoutRect,isHorizontal);
> +      // Actual and containing rectangles need to be updated
> +      // from these cached values, as they may be modified.

Might be worth mentioning "outparams" in this comment to make it clearer why these updates matter.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211754/diff/3-4/

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

https://reviewboard.mozilla.org/r/211754/#review231444

r=me, with the unrelated-looking gfx/2d/DrawTargetD2D1.cpp changes reverted.

::: gfx/2d/DrawTargetD2D1.cpp:1546
(Diff revision 4)
> +#if 0
>    PopAllClips();
> +#endif

Looks like this file's changes are from a random local change that you qref'ed in by accident -- this looks unrelated to BaseRect usage, and it's a change in gfx rather than layout.

Comment 22

[Daniel Holbert [:dholbert]]

(I think this can land, after the unrelated thing in comment 21 is reverted?)

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8941472 [details]
Bug 1424382: Use BaseRect access methods instead of member variables in layout/ (simpler cases).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211754/diff/4-5/

Comment 24

[Daniel Holbert [:dholbert]]

I'd like to see if I can get this landed before it bitrots too much (assuming bug 1386277 is still something we want to do.) Setting ni=me to hopefully avoid losing track of this.

Comment 25

[Daniel Holbert [:dholbert]]

(Canceling ni; I think this has likely bitrotted and I don't foresee having cycles to spend untangling that in the near future.)