superreview requested: [Bug 863972] deCOM box border/padding/margin methods : [Attachment 739924] patch

[bugzill...@mozilla.org]

Daniel Holbert [:dholbert] <dhol...@mozilla.com> has asked for superreview:
Bug 863972: deCOM box border/padding/margin methods
https://bugzilla.mozilla.org/show_bug.cgi?id=863972

Attachment 739924: patch
https://bugzilla.mozilla.org/attachment.cgi?id=739924&action=edit


------- Additional Comments from Daniel Holbert [:dholbert]
<dhol...@mozilla.com>
>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.c
pp?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

[bugzill...@mozilla.org]

Cameron McCormack (:heycam) <c...@mcc.id.au> has reassigned Daniel Holbert
[:dholbert] <dhol...@mozilla.com>'s request for superreview to David Baron
[:dbaron] (don't cc:, use needinfo? instead) <dba...@dbaron.org>:

Bug 863972: deCOM box border/padding/margin methods
https://bugzilla.mozilla.org/show_bug.cgi?id=863972

Attachment 739924: patch
https://bugzilla.mozilla.org/attachment.cgi?id=739924&action=edit

------- Additional Comments from Cameron McCormack (:heycam) <c...@mcc.id.au>
Yes, seems like a similar case to bug 842065.

[bugzill...@mozilla.org]

David Baron [:dbaron] (don't cc:, use needinfo? instead) <dba...@dbaron.org>

has granted Daniel Holbert [:dholbert] <dhol...@mozilla.com>'s request for
superreview:

Bug 863972: deCOM box border/padding/margin methods
https://bugzilla.mozilla.org/show_bug.cgi?id=863972

Attachment 739924: patch
https://bugzilla.mozilla.org/attachment.cgi?id=739924&action=edit

------- Additional Comments from David Baron [:dbaron] (don't cc:, use
needinfo? instead) <dba...@dbaron.org>
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?