863972 - deCOM box border/padding/margin methods

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

We never get non-NS_OK return values from these methods, so let's change them to return nsMargins directly.

Comment 1

[Daniel Holbert [:dholbert]]

Comment on attachment 739924 [details] [diff] [review]
patch

>diff --git a/layout/xul/base/src/nsBox.cpp b/layout/xul/base/src/nsBox.cpp
>-NS_IMETHODIMP
>-nsIFrame::GetBorderAndPadding(nsMargin& aBorderAndPadding)
>+nsMargin
>+nsIFrame::GetBorderAndPadding()
> {
[...]
>+  return GetBorder() + GetPadding();
> }

Nice, so this is just a one-liner now.

Might be worth moving the impl into nsIFrame.h (merging the function definition into its decl), but it's fine as-is too.

>+++ b/layout/xul/base/src/nsMenuFrame.cpp
>-      nsMargin borderPadding;
>-      GetBorderAndPadding(borderPadding);
>+      nsMargin borderPadding = GetBorderAndPadding();
> 
>       // if there is a scroll frame, add the desired width of the scrollbar as well
>       nsIScrollableFrame* scrollFrame = do_QueryFrame(popupFrame->GetFirstPrincipalChild());
>       nscoord scrollbarWidth = 0;
>       if (scrollFrame) {
>         scrollbarWidth =
>           scrollFrame->GetDesiredScrollbarSizes(&aState).LeftRight();
>       }

This |borderPadding| is only used once, a few lines below this:
  aSize.width =
    tmpSize.width + std::max(borderPadding.LeftRight(), scrollbarWidth);
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuFrame.cpp?rev=d1f113402481#1375

Just drop |borderPadding| and replace that one usage with GetBorderAndPadding().

>diff --git a/layout/xul/grid/nsGrid.cpp b/layout/xul/grid/nsGrid.cpp
>--- a/layout/xul/grid/nsGrid.cpp
>+++ b/layout/xul/grid/nsGrid.cpp
>@@ -675,18 +675,18 @@ nsGrid::GetRowOffsets(nsBoxLayoutState& 
>     if (!box->IsCollapsed())
>     {
>        // get real border and padding. GetBorderAndPadding
>        // is redefined on nsGridRowLeafFrame. If we called it here
>        // we would be in finite recurson.
>-       box->GetBorder(border);
>-       box->GetPadding(padding);
>+       border = box->GetBorder();
>+       padding = box->GetPadding();
> 
>        totalBorderPadding += border;
>        totalBorderPadding += padding;
>      }
[...]
>-           box->GetBorder(border);
>-           box->GetPadding(padding);
>+           border = box->GetBorder();
>+           padding = box->GetPadding();
>            totalChildBorderPadding += border;
>            totalChildBorderPadding += padding;
>            totalChildBorderPadding += margin;
>         }

Drop the variables |border|, |padding|, |margin| here entirely, and just directly add box->GetBorder() etc.  (AFAICT, these are declared at the beginning of this function & are there purely for memory-reuse purposes. Now that we don't need outparams, they're no longer needed.)

>+++ b/layout/xul/grid/nsGridRowLeafLayout.cpp
>@@ -152,24 +152,19 @@ nsGridRowLeafLayout::PopulateBoxSizes(ns
>       if (i == firstIndex || i == lastIndex) {
>-        nsMargin offset = GetTotalMargin(aBox, isHorizontal);
>-
>-        nsMargin border(0,0,0,0);
>-        // can't call GetBorderPadding we will get into recursion
>-        aBox->GetBorder(border);
>-        offset += border;
>-        aBox->GetPadding(border);
>-        offset += border;
>+        nsMargin offset = GetTotalMargin(aBox, isHorizontal) +
>+                          aBox->GetBorder() +
>+                          aBox->GetPadding();

Keep the comment about avoiding recursion.  (with typofix s/GetBorderPadding/GetBorderAndPadding/)

r=me with the above.

This probably needs super-review as well, though, since it's an interface change, and it's possible[1] (though probably unlikely? [2]) that binary extensions might be using these APIs.

Flagging for blank super-review right now -- I'll let you pick your super-reviewer. (maybe bz or dbaron?)

[1] per bug 842065 comment 5
[1] per bug 842065 comment 7

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 739924 [details] [diff] [review]
patch

Yes, seems like a similar case to bug 842065.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 739924 [details] [diff] [review]
patch

Sorry for dropping this for so long.

I'm a little worried here that we might start confusing these methods for the non-Box methods.  I wonder if we should call them GetBoxMargin() / GetBoxPadding() etc., instead?  Does that seem reasonable?

Comment 4

[Cameron McCormack (:heycam)]

Yes, that sounds fine.

Comment 5

[Cameron McCormack (:heycam)]

Some of this patch may still be relevant, but it'll be easier to write from scratch.