366534 - deCOMtaminate nsIBoxLayout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andreas Lange]

Some of the functions should be handled like the the corresponding nsIBox methods in nsIFrame, see bug 243370.

Comment 1

[Sudheer]

I am working on this as my first mozilla bug. 

Comment 2

[Sudheer]

Attachment: deCOM fixes for nsIBoxLayout

deCOMtaminated all functions, except Layout () as it seems to return success and failure and the return value is being checked.

I compiled and did some testing, but I am not sure if there is a test suite. Please let me know and I will test.

Comment 3

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

You should add the line:

diff -pNu12

to your ~/.cvsrc and diff again.  You also have a bunch of unrelated changes at the end of the diff.  When you attach a new patch, set the review flag to "?" and enter the email address "[e-mail]".

Comment 4

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

Regarding testing:  reftests and mochitests probably exercise this code a bit.  You may want to try running them.  See http://developer.mozilla.org/en/docs/Mozilla_automated_testing .  The Firefox UI probably exercises it even more, though.

Comment 5

[Sudheer]

Attachment: deCOMtamination nsIBoxLayout

Corrected the mistakes pointed out by David Baron. Thanks David. I did play with firefox UI, but I will try other tests suggested.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsBoxLayout::GetFlex and nsBoxLayout::IsCollapsed should be removed (from nsSprocketLayout too). The implementations just call to the underlying aBox method in all situations, so we might as well eliminate these methods and have the callers call the methods on aBox directly.

+  nsSize mnSize (0,0);

minSize

+nsSize
+nsBoxLayout::GetMaxSize(nsIBox* aBox, nsBoxLayoutState& aBoxLayoutState)
 {
-  aSize.width = NS_INTRINSICSIZE;
-  aSize.height = NS_INTRINSICSIZE;
-  AddBorderAndPadding(aBox, aSize);
-  return NS_OK;
+  nsSize mxSize (NS_INTRINSICSIZE,NS_INTRINSICSIZE);
+  AddBorderAndPadding(aBox, mxSize);
+  return mxSize;
 }

maxSize would be a better name, but actually you can return nsSize(NS_INTRINSICSIZE, NS_INTRINSICSIZE) here. AddBorderAndPadding never changes a size that's already NS_INTRINSICSIZE.

+  nsSize pref;
+  pref = nsGridRowGroupLayout::GetPrefSize(aBox, aBoxLayoutState);

Put these on the same line, or if the line is too long, write
  nsSize pref =
    nsGridRowGroupLayout::...

Comment 7

[Sudheer]

Attachment: deCOM fixes for nsIBoxLayout

Fixed per ROC's comments. I tried mochitest, but the window seem to hang (I did not knew that the window should be always focused while testing, if that matters).
The latest trunk seem to have some problem with the menus (file, edit, etc..) When I click on them, the menu's are long as the window. This happens without my changes (I updated yesterday evening. I guess somebody filed a bug). I tested my fixes with little bit older build and everything seems fine.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-  return NS_OK;
+  return;
 }

You don't need these "return" statements at the end of void functions.

Other than that it looks. Just post a new patch with that fixed and you should be done.

Comment 9

[Sudheer]

Attachment: patch - deCOMtaminate nsIBoxLayout

Fixed per ROC's comments.

Comment 10

[Andreas Lange]

NS_IBOX_LAYOUT_IID needs update, right?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

yes

Comment 12

[Sudheer]

Should I generate new IID and update nsIBoxLayout.h?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes please

Comment 14

[Sudheer]

Attachment: patch - deCOMtamination of nsIBoxLayout

Generated new IID for nsIBoxLayout

Comment 15

[Reed Loden [:reed]]

Comment on attachment 296056 [details] [diff] [review]
patch - deCOMtamination of nsIBoxLayout

Continue with deCOM of more stuff to make code cleaner and better overall.

Comment 16

[Mike Schroepfer]

Comment on attachment 296056 [details] [diff] [review]
patch - deCOMtamination of nsIBoxLayout

deCOM is great - thanks for the patch!

Comment 17

[Reed Loden [:reed]]

Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.347; previous revision: 1.346
done
Checking in layout/xul/base/src/nsBoxLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxLayout.cpp,v  <--  nsBoxLayout.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in layout/xul/base/src/nsBoxLayout.h;
/cvsroot/mozilla/layout/xul/base/src/nsBoxLayout.h,v  <--  nsBoxLayout.h
new revision: 1.11; previous revision: 1.10
done
Checking in layout/xul/base/src/nsIBoxLayout.h;
/cvsroot/mozilla/layout/xul/base/src/nsIBoxLayout.h,v  <--  nsIBoxLayout.h
new revision: 1.11; previous revision: 1.10
done
Checking in layout/xul/base/src/nsListBoxLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsListBoxLayout.cpp,v  <--  nsListBoxLayout.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in layout/xul/base/src/nsListBoxLayout.h;
/cvsroot/mozilla/layout/xul/base/src/nsListBoxLayout.h,v  <--  nsListBoxLayout.h
new revision: 1.4; previous revision: 1.3
done
Checking in layout/xul/base/src/nsSprocketLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,v  <--  nsSprocketLayout.cpp
new revision: 1.65; previous revision: 1.64
done
Checking in layout/xul/base/src/nsSprocketLayout.h;
/cvsroot/mozilla/layout/xul/base/src/nsSprocketLayout.h,v  <--  nsSprocketLayout.h
new revision: 1.19; previous revision: 1.18
done
Checking in layout/xul/base/src/nsStackLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsStackLayout.cpp,v  <--  nsStackLayout.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in layout/xul/base/src/nsStackLayout.h;
/cvsroot/mozilla/layout/xul/base/src/nsStackLayout.h,v  <--  nsStackLayout.h
new revision: 1.10; previous revision: 1.9
done
Checking in layout/xul/base/src/grid/nsGridLayout2.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridLayout2.cpp,v  <--  nsGridLayout2.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in layout/xul/base/src/grid/nsGridLayout2.h;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridLayout2.h,v  <--  nsGridLayout2.h
new revision: 1.16; previous revision: 1.15
done
Checking in layout/xul/base/src/grid/nsGridRowGroupLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowGroupLayout.cpp,v  <--  nsGridRowGroupLayout.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in layout/xul/base/src/grid/nsGridRowGroupLayout.h;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowGroupLayout.h,v  <--  nsGridRowGroupLayout.h
new revision: 1.8; previous revision: 1.7
done
Checking in layout/xul/base/src/grid/nsGridRowLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLayout.cpp,v  <--  nsGridRowLayout.cpp
new revision: 1.15; previous revision: 1.14
done
Checking in layout/xul/base/src/grid/nsGridRowLayout.h;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLayout.h,v  <--  nsGridRowLayout.h
new revision: 1.8; previous revision: 1.7
done
Checking in layout/xul/base/src/grid/nsGridRowLeafLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp,v  <--  nsGridRowLeafLayout.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in layout/xul/base/src/grid/nsGridRowLeafLayout.h;
/cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.h,v  <--  nsGridRowLeafLayout.h
new revision: 1.9; previous revision: 1.8
done