243370 - deCOMtaminate parts of nsIFrame that come from nsIBox

Status: RESOLVED INACTIVE

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It'd be nice to document into the bargain, but.... ;)

Some things of note here:

1) Can GetFrame() ever return null?  If not, it should be Frame().
2) Should GetMouseThrough be HasMouseThrough()?

Comment 1

[Henrik Lynggaard Hansen]

I'm taking this.

Going to be my first steps into mozilla code (as suggested by Robert O'Callahan). 

Comment 2

[Henrik Lynggaard Hansen]

Attachment: GetPreferredSize,GetMinimumSize,GetMaximumSize

Patch for 
-  NS_IMETHOD GetPrefSize(nsBoxLayoutState& aBoxLayoutState, nsSize& aSize)=0;
-  NS_IMETHOD GetMinSize(nsBoxLayoutState& aBoxLayoutState, nsSize& aSize)=0;
-  NS_IMETHOD GetMaxSize(nsBoxLayoutState& aBoxLayoutState, nsSize& aSize)=0;

Comment 3

[Henrik Lynggaard Hansen]

Setting optimistic milestone

Comment 4

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

Comment on attachment 149215 [details] [diff] [review]
GetPreferredSize,GetMinimumSize,GetMaximumSize

It mostly looks good. A few minor comments:

+nsSize nsGfxScrollFrame::GetMaximumSize(nsBoxLayoutState& aState)

This should just return nsSize(NS_MAXSIZE, NS_MAXSIZE). You don't want to do
arithmetic on NS_MAXSIZE.

+  NS_ENSURE_SUCCESS(rv, nsSize(0,0));

You should write this out instead of using the macro; I think NS_ENSURE_SUCCESS
is only intended for use in functions that return nsresult.

+			  
+			  
+  // nsIbox

Just one blank line please.

   NS_IMETHOD GetAscent(nsBoxLayoutState& aBoxLayoutState, nscoord& aAscent);
+  
+
+  
 #ifdef ACCESSIBILITY

ditto ... everywhere you do this...

+  mMaxSize.width = NS_MAXSIZE;
+  mMaxSize.height = NS_MAXSIZE;
+  mMaxSize = nsBox::GetMaximumSize(aState);

The first two assignments are guaranteed to be overwritten, remove them.

+nsSize nsContainerBox::GetPreferredSize(nsBoxLayoutState& aState)
...
+    result.width = 0;
+    result.height = 0;
they should already be zero, right?

+    result.width = 0;
+    result.height = 0;
same thing

+    result.width = NS_MAXSIZE;
+    result.height = NS_MAXSIZE;
same thing

 #define NS_IBOX_IID { 0x162f6b5a, 0xf926, 0x11d3, { 0xba, 0x6, 0x0, 0x10,
0x83, 0x2, 0x3c, 0x1e } }
Please change this to a new IID, since you're changing the interface.

+
 #include "nsIServiceManager.h"

Please, no unnecessary blank lines :-)

   nsSize size(0,0);
-  GetPrefSize(aState, size);
+  size = GetPreferredSize(aState);
   aCoord = size.height;

Probably just write this as "aCoord = GetPreferredSize(aState).height;"

-     nsSize minSize(0,0);
-     GetMinSize(state,  minSize);
+     nsSize minSize = GetMinimumSize(state);
+
unnecessary newline

+  virtual nsresult GetPreferredSize(nsBoxLayoutState& aBoxLayoutState, nsSize&
aSize);
forgot to change result type to nsSize ... followed by unnecessary newlines

In xul/base/src/nsListItemFrame.h
+  virtual nsSize GetPreferredSize(nsBoxLayoutState& aBoxLayoutState);
+  
stray newline

-    ibox->GetMaxSize(aState, maxSize);
+ 
+    nsSize prefSize = ibox->GetPreferredSize(aState);
stray newline

+nsMenuFrame::GetPreferredSize(nsBoxLayoutState& aState)
...
+      maxSize = GetMaximumSize(aState);
might as well declare maxSize here rather than up above.

In xul/base/src/nsMenuFrame.h
+  
+  // nsIbox
Argh! Stray newline! :-)

+ * @copydoc nsIBox::GetPreferredSize
Not sure what this does. I don't think we use it anywhere else. Maybe use @see
if we use that anywhere else in Mozilla?

In xul/base/src/nsNativeScrollbarFrame.h
+		     
+
+  // nsIBox
+  virtual nsSize GetPreferredSize(nsBoxLayoutState& aBoxLayoutState);
+
newwwwwwliiiiiines

+nsSize nsScrollBoxFrame::GetPreferredSize(nsBoxLayoutState& aBoxLayoutState)
 {
+  nsSize result(0,0);
declare it below, where it gets assigned.

+nsSize nsScrollBoxFrame::GetMaximumSize(nsBoxLayoutState& aBoxLayoutState)
 {
+  nsSize result(NS_MAXSIZE,NS_MAXSIZE);
ditto

-  // nsIBox methods
+
extra blank lines!!!!

+  nsSize thumbSize = thumbBox->GetPreferredSize(aState);
+  
another one

@@ -798,24 +798,22 @@ nsSplitterFrameInner::MouseDown(nsIDOMEv
another one in here

+	     nsSize max =aChild->GetMaximumSize(aState);
fix space

@@ -1433,18 +1431,18 @@ nsSprocketLayout::GetMaxSize(nsIBox* aBo
stray newline in here

     nsSize pref(0,0);
-    child->GetPrefSize(aState, pref);
+    pref = child->GetPreferredSize(aState);
put def and assignment on the same line

	     nsSize pref(0,0);
-	     child->GetPrefSize(aState, pref);
+	     pref = child->GetPreferredSize(aState);
also here

Comment 5

[Henrik Lynggaard Hansen]

> +nsSize nsGfxScrollFrame::GetMaximumSize(nsBoxLayoutState& aState)
> 
> This should just return nsSize(NS_MAXSIZE, NS_MAXSIZE). You don't want to do
> arithmetic on NS_MAXSIZE.

Done, but should this not be for most of the GetMaximum methods:

nsBox::GetMaximumSize
nsBoxToBlockAdaptor::GetMaximumSize (not really sure here)
nsContainerBox::GetMaximumSize (properly not)
nsTextControlFrame::GetMaximumSize (if nsBox:... is then this should too)
nsBoxFrame::GetMaximumSize (properly not - containerbox dependency)
nsScrollBoxFrame::GetMaximumSize (properly not)
nsSliderFrame::GetMaximumSize (properly not)

> 
> +  NS_ENSURE_SUCCESS(rv, nsSize(0,0));
> 
> You should write this out instead of using the macro; I think NS_ENSURE_SUCCESS
> is only intended for use in functions that return nsresult.

Hmm, think this might go against the style guide that says:
" Use the NS_ENSURE_SUCCESS(rv, rv)  and NS_ENSURE_TRUE(expr, rv) macros in
place of if (NS_FAILED(rv)) { return rv; } and if (!expr) { return rv; }, unless
the failure is a normal condition (i.e. you don't want it to assert)."

I'm guessing that since the previous version also used NS_ENSURE_SUCCES it is
not a "normal condition"

Looking at the Marco it simply does "return <ret>;" with the 2. param, thus
becomming "return nsSize(0,0);". This should be good as long as the return types
match, which they do.

You still think it should be re-written ?

> +nsSize nsContainerBox::GetPreferredSize(nsBoxLayoutState& aState)
> ...
> +    result.width = 0;
> +    result.height = 0;
> they should already be zero, right?
> 
I was unsure if the AddCssPref/AddCssMin/AddCssMax method might have changed the
result value as a sideeffect, but re-reading those calls it would seem not so,
They will be removed


> 
>  #define NS_IBOX_IID { 0x162f6b5a, 0xf926, 0x11d3, { 0xba, 0x6, 0x0, 0x10,
> 0x83, 0x2, 0x3c, 0x1e } }
> Please change this to a new IID, since you're changing the interface.

Silly newbie question: how ?

> + * @copydoc nsIBox::GetPreferredSize
> Not sure what this does. I don't think we use it anywhere else. Maybe use @see
> if we use that anywhere else in Mozilla?

It copies the doc from the given place, unlike @see that only creates a link. In
my opinion copydoc is better in this instance because we really want to copy the
parameter and return information. 

If I was to not use copydoc I would manually copy the @param and @return.Those
informations should not be hidden behind a "see"."See" is very valid if we are
refering to other methods or bodies of text, but not for paramters and returns

LXR finds 6 uses, in XPCOM classes, but even if this is a small amount I think
it is better to go in that direction..

Comment 6

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

> Done, but should this not be for most of the GetMaximum methods:

Correct, please fix them all :-)

> Hmm, think this might go against the style guide that says:

OK, leave it as you have it.

> Please change this to a new IID, since you're changing the interface.

Get hold of a UUID generator that generates random unique UUIDs. On Linux I have
one called 'uuidgen'. There are other similar things for Windows. Run it. Paste
the UUID into the code in a format that matches the format in the code.

I stil don't like copydoc because I don't want to unilaterally start extending
our use of Javadoc comments. If you want to use more Javadoc features, please
start a thread on .seamonkey to argue for it :-). For now, just copy the comment
please. Thanks!

Comment 7

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

Comment on attachment 149215 [details] [diff] [review]
GetPreferredSize,GetMinimumSize,GetMaximumSize

minusing in anticipation of new patch

Comment 8

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

This bug is somewhat obsolete because bryner is merging nsIBox into nsIFrame.
We'll still need this deCOMtamination in nsIFrame though.

Comment 9

[Justin Wood (:Callek)]

Henrik is inactive per roc, assign to "nobody".

Comment 10

[Vidar Haarr (not reading bugmail)]

I'll take a look at this once I'm done with nsPresContext cleaning in layout.
Targetting for when I think we should do this; if anyone thinks we should get it
between 1.8 beta and release, please feel free to retarget.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 258513 merged nsIBox into nsIFrame, adding dependency

Comment 12

[Andreas Lange]

Attachment: GetPrefSize, GetMinSize, GetMaxSize

The former patch was mostly useless since the reorganization. Here is a new one
dealing with the same functions. Still a lot of work to do, but I think this
chunk is enough to start with.

Comment 13

[Andreas Lange]

Hm, I updated the summary from nsIBox to nsIFrame as that is where the functions
are now. However, there is already bug 190735 with that summary - untouched
since nov 2003 though.

Comment 14

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

Any chance you could hold off on doing further work on at least the
layout-related parts of this until after the reflow branch lands?

Comment 15

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

Comment on attachment 190017 [details] [diff] [review]
GetPrefSize, GetMinSize, GetMaxSize

This is great stuff!

-#define FIX_FOR_BUG_40596
-#ifdef FIX_FOR_BUG_40596
-  aSize = mMinSize;
-  return NS_OK;
-#else
-  return nsBox::GetMinSize(aState, aSize);
-#endif
+  return nsBox::GetMinSize(aState);

You should just be returning mMinSize here, right?

+nsSize nsFrame::GetMaxSize(nsBoxLayoutState& aState)
+{
+  return nsSize(NS_MAXSIZE, NS_MAXSIZE);

Are you completely sure this is correct?

Ditto for +nsSize nsBoxFrame::GetMaxSize(nsBoxLayoutState& aBoxLayoutState).
nsIBox::AddCSSMaxSize can change the maxsize even when the initial size is
NS_INTRINSICSIZE --- it's not simply "add", it's sometimes "replace".

Comment 16

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

But yeah we should not land this until the reflow branch lands.

Comment 17

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

Andreas, please attach a new patch. I'll finish the review of it and then land
it after the reflow branch has landed.

Comment 18

[Andreas Lange]

Attachment: GetPrefSize, GetMinSize, GetMaxSize (v2)

Patch updated to tree and the first comments. A tad more reorg while I were at
it.

Noted but not fixed: the comment first in nsFrame::Get[Min|Max]Size is probably
stale, the check below does not care about reflow constraints. Should it?

Comment 19

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

Sorry, that was meant to be parsed

Andreas, please attach a new patch. (I'll finish the review of it and then land
it) after the reflow branch has landed.

So you'll need to update it after the reflow branch lands.

Comment 20

[Andreas Lange]

Ah, that makes more sense. :-)

Comment 21

[Justin Wood (:Callek)]

roc, that "wait for reflow branch", will that affect all parts of this listed on http://wiki.mozilla.org/Gecko:DeCOMtamination or just Get(?:Pref|Min|Max)Size ?

Comment 22

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

All of them.

Comment 23

[dan]

screw your mom

Comment 24

[James Napolitano]

(In reply to comment #19)
> 
> Andreas, please attach a new patch. (I'll finish the review of it and then land
> it) after the reflow branch has landed.
> 
> So you'll need to update it after the reflow branch lands.

Said reflow branch has now landed.

Comment 25

[Andreas Lange]

Yes, I'll continue on this. Parts of the v2 patch have already landed in bug 363879. 

Comment 26

[Andreas Lange]

Attachment: GetPrefSize, GetMinSize, GetMaxSize (v3)

Only a few parts of the old patch still applied.

Comment 27

[Andreas Lange]

Comment on attachment 250521 [details] [diff] [review]
GetPrefSize, GetMinSize, GetMaxSize (v3)

If anyone on CC has CVS access and some time, feel free to land this!

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Build bustage fix

I have no idea how this compiled as it was -- BoundsCheck takes non-const references for all three arguments and actually modifies the max-size (so I have _no_ idea what this code ended up doing with compilers where it did happen to compile).

Comment 29

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

There's actually several occurrences of that.

IMHO Andreas should fix this by deCOMtaminating BoundsCheck.
void BoundsCheck(nsSize&, nsSize&, nsSize&) should become
nsSize BoundsCheck(const nsSize&, const nsSize&, const nsSize&)
void BoundsCheck(nscoord&, nscoord&, nscoord&) should become
nscoord BoundsCheck(nscoord, nscoord, nscoord).

Comment 30

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

Though I suppose it would better to apply your fix everywhere and fix BoundsCheck separately.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Except BoundsCheck needs to return two separate things, since it changes the pref _and_ max size/coord as I said.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: More bustage fixes

Comment 33

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

Hardly any of the callers need the updated max; the ones that do should call BoundsCheckMinMax instead (which itself should be fixed to be "nsSize BoundsCheckMinMax(const nsSize&, const nsSize&)".

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: And more bustage fixes

Comment 35

[Andreas Lange]

Sorry for the bustages, seems that I should test on gcc as well.  

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Should this be resolved now that the patch was checked in, or is there more work to do?

Comment 37

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

There's more work to do.

Comment 38

[Andreas Lange]

There are still some ~25 functions that should be fixed in some way. The number of patches to do is mostly a questions of how large the patches should be...

Filed bug 366531 on the BoundsCheck() issue and bug 366534 on the similar functions in nsIBoxLayout.

Comment 39

[Andreas Lange]

Attachment: Part 2, v1

Comment 40

[Andreas Lange]

Comment on attachment 252309 [details] [diff] [review]
Part 2, v1

Checkpoint, time to get comment on these changes before continuing.

Summary of nsIFrame changes this time - 6 functions updated and 6 removed:

-  NS_IMETHOD GetFlex(nsBoxLayoutState& aBoxLayoutState, nscoord& aFlex)=0;
+  virtual nscoord GetFlex(nsBoxLayoutState& aBoxLayoutState) = 0;
-  NS_IMETHOD GetAscent(nsBoxLayoutState& aBoxLayoutState, nscoord& aAscent)=0;
+  virtual nscoord GetAscent(nsBoxLayoutState& aBoxLayoutState) = 0;
-  NS_IMETHOD IsCollapsed(nsBoxLayoutState& aBoxLayoutState, PRBool& aCollapsed)=0;
+  virtual PRBool IsCollapsed(nsBoxLayoutState& aBoxLayoutState) = 0;
-  NS_IMETHOD GetMouseThrough(PRBool& aMouseThrough)=0;
+  virtual PRBool GetMouseThrough() const = 0;

Reduced to only one override:
-  NS_IMETHOD ChildrenMustHaveWidgets(PRBool& aMust) const=0;
+  // Only nsDeckFrame requires that all its children have widgets
+  virtual PRBool ChildrenMustHaveWidgets() const { return PR_FALSE; }

Updated users with alternatives:
-  NS_HIDDEN_(nsresult) GetContentRect(nsRect& aContentRect); 
   PRBool IsHorizontal() const { return (mState & NS_STATE_IS_HORIZONTAL) != 0; }
-  nsresult GetOrientation(PRBool& aIsHorizontal)  /// XXX to be removed
-  { aIsHorizontal = IsHorizontal(); return NS_OK; } 
   PRBool IsNormalDirection() const { return (mState & NS_STATE_IS_DIRECTION_NORMAL) != 0; }
-  nsresult GetDirection(PRBool& aIsNormal)  /// XXX to be removed
-  { aIsNormal = IsNormalDirection(); return NS_OK; }
 
No users: (Possibly BoxMetrics::mOverflow is unused after this)
-  NS_IMETHOD SetIncludeOverflow(PRBool aInclude) = 0;
-  NS_IMETHOD GetOverflow(nsSize& aOverflow) = 0;
-  NS_IMETHOD GetIndexOf(nsIBox* aChild, PRInt32* aIndex)=0;

Non-virtual:
-  NS_HIDDEN_(nsresult)
-    GetOrdinal(nsBoxLayoutState& aBoxLayoutState, PRUint32& aOrdinal);
+  // Implemented in nsBox, used in nsBoxFrame
+  PRUint32 GetOrdinal(nsBoxLayoutState& aBoxLayoutState);

Comment 41

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

Good job!

> No users: (Possibly BoxMetrics::mOverflow is unused after this)

Can you check on that?

Could you change the name of GetAscent to GetBoxAscent? We have some warnings where other frames declare methods called GetAscent with different signatures.

Comment 42

[Andreas Lange]

Attachment: BoxMetrics reduction, diff -b

> > No users: (Possibly BoxMetrics::mOverflow is unused after this)
> Can you check on that?

Yepp, that was correct. And mIncludeOverflow is also unused. Attached a diff -b of the removal. Should I fold it into my ongoing patch here or spin of another bug? Are all comments in the code still relevant?

> Could you change the name of GetAscent to GetBoxAscent? We have some warnings
> where other frames declare methods called GetAscent with different signatures.

Sure, 25 occurrances replaced. Should I attach a patch for review (~100k now), or do you prefer a larger patch with a couple of more functions fixed?

Comment 43

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

Roll those changes into the current patch and we'll do this patch now before we move on to any new functions.

Comment 44

[Andreas Lange]

Attachment: Part 2, v2

Comment 45

[Andreas Lange]

(In reply to comment #0)

> 2) Should GetMouseThrough be HasMouseThrough()?

Re-read bz's comment now. The attached patch deals with that function, should it be renamed?

Comment 46

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

I think GetMouseThrough is OK.

-    nsRect insideBorder;
-    aBox->GetContentRect(insideBorder);
+    nsRect insideBorder(aBox->mRect);

Is this correct? Seems like you should set insideBorder's origin to (0,0).

   PRBool c1 = PR_FALSE, c2 = PR_FALSE;
 
   if (mBoxInColumn) 
-    mBoxInColumn->IsCollapsed(aState, c1);
+    c1 = mBoxInColumn->IsCollapsed(aState);
 
   if (mBoxInRow) 
-    mBoxInRow->IsCollapsed(aState, c2);
+    c2 = mBoxInRow->IsCollapsed(aState);
 
   return (c1 || c2);

This can be rewritten into a single boolean expression.

Comment 47

[Andreas Lange]

Attachment: Part 2, v2.1 - comments fixed

I had missed to add a MoveTo(0,0) there, thanks.

Comment 48

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

   // ----- figure out our size ----------
-  nsRect contentRect;
-  aBox->GetContentRect(contentRect);
+  nsRect contentRect(aBox->GetRect());

For this, where you're only using the size, it would be clearer to rename contentRect to just "nsSize size;".

Other than that it's all good :-)

Comment 49

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

   // ----- figure out our size ----------
-  nsRect contentRect;
-  aBox->GetContentRect(contentRect);
+  nsRect contentRect(aBox->GetRect());

For this, where you're only using the size, it would be clearer to rename contentRect to just "nsSize size;" (call aBox->GetSize()).

Other than that it's all good :-)

Comment 50

[Andreas Lange]

Attachment: Part 2, v2.2 - final improvements made?

|size| was already used a lot in that function, so I went with |originalSize|.

Also reduced nsGridRow::IsCollapsed() in the same way as nsGridCell::IsCollapsed().

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 253459 [details] [diff] [review]
Part 2, v2.2 - final improvements made?

Checked in

Comment 52

[[e-mail]]

Comment on attachment 253459 [details] [diff] [review]
Part 2, v2.2 - final improvements made?

>    while (child) 
>    {  
>        // ignore collapsed children
>-      PRBool isCollapsed = PR_FALSE;
>-      aBox->IsCollapsed(aState, isCollapsed);
>-
>-      if (!isCollapsed)
>+      if (!aBox->IsCollapsed(aState))

>    while (child) 
>    {  
>       // ignore collapsed children
>-      PRBool isCollapsed = PR_FALSE;
>-      aBox->IsCollapsed(aState, isCollapsed);
>-
>-      if (!isCollapsed)
>+      if (!aBox->IsCollapsed(aState))
These were wrong before but it's more obvious now ;-)

Comment 53

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

(In reply to comment #52)
> These were wrong before but it's more obvious now ;-)

Are you pointing out what's being fixed in bug 368902, or another occurence?

Comment 54

[Andreas Lange]

(In reply to comment #53)

> > These were wrong before but it's more obvious now ;-)
> 
> Are you pointing out what's being fixed in bug 368902, or another occurence?

Those are fixed in bug 368902. Hey, didn't think you kept track of that Neil. :-)

Comment 55

[Boris Zbarsky [:bzbarsky]]

That last patch missed a GetCollapsed() implementation.  See bug 369127.

Comment 56

[Andreas Lange]

Attachment: Part 3, v1

-  NS_IMETHOD SetBounds(nsBoxLayoutState& aBoxLayoutState, const nsRect& aRect,
-                       PRBool aRemoveOverflowArea = PR_FALSE)=0;
+  virtual void SetBounds(nsBoxLayoutState& aBoxLayoutState, const nsRect& aRect,
+                         PRBool aRemoveOverflowArea = PR_FALSE) = 0;
-  nsresult GetChildBox(nsIBox** aBox)
+  nsIBox* GetChildBox() const
-  nsresult GetNextBox(nsIBox** aBox)
+  nsIBox* GetNextBox() const
-  NS_HIDDEN_(nsresult) GetParentBox(nsIBox** aParent);
+  nsIBox* GetParentBox() const

-  NS_IMETHOD GetBorderAndPadding(nsMargin& aBorderAndPadding);
-  NS_IMETHOD GetBorder(nsMargin& aBorder)=0;
-  NS_IMETHOD GetPadding(nsMargin& aBorderAndPadding)=0;
-  NS_IMETHOD GetMargin(nsMargin& aMargin)=0;
-  NS_IMETHOD GetInset(nsMargin& aInset)=0;
+  virtual nsMargin GetBorderAndPadding();
+  virtual nsMargin GetBorder() = 0;
+  virtual nsMargin GetPadding() = 0;
+  virtual nsMargin GetMargin() = 0;
+  virtual nsMargin GetInset() = 0;

There is one GetBorder() call in accessible...

Comment 57

[Andreas Lange]

Attachment: Part 3, v1 - updated to tree

A missing piece from part 2 that was in part 3 got landed in bug 369127. Sorry that I didn't notice it myself a bit earlier.

Comment 58

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

I think we should consider just removing GetInset. It's used only for some arcane XUL debugging feature.

The GetBorder/GetPadding/GetMargin/GetBorderAndPadding can perhaps be just dropped in favour of GetUsedBorder etc?

Comment 59

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

(In reply to comment #58)
> The GetBorder/GetPadding/GetMargin/GetBorderAndPadding can perhaps be just
> dropped in favour of GetUsedBorder etc?

Not trivially, and even then it might cause changes in behavior.

Comment 60

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

Hrm, actually, maybe it is pretty trivial if we override them on nsBoxFrame/nsLeafBoxFrame -- except that we might need to be careful with GetUsedMargin around block/box boundaries.

Comment 61

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

I'm not sure what the issue is here ... GetBorder/GetPadding/GetMargin/GetBorderAndPadding in nsBox.cpp seem to have the same implementation as the nsIFrame::GetUsed* methods (except that the nsBox.cpp versions don't handle percentage margins or padding). The only behaviour changes I can see would involve percentage margins or padding, where the current code is completely broken, or XUL code calling these methods on (box-wrapped?) table frames, where the current code is also likely to be completely broken.

There is one problem, actually, which is that GetBorderAndPadding doesn't just sum up GetBorder + GetPadding in all cases --- nsGroupBoxFrame and nsGridRowLeafBoxFrame override it and return special values (which is pretty bogus if you ask me). So we should rename that to a special weird (but deCOMtaminated) GetBoxBorderAndPadding method, for now.

Comment 62

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

Maybe we should sort out that GetBorderAndPadding issue first and save ourselves an extra mass rename?

Comment 63

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

Sure. Andreas, can you figure out what's going on there? :-) In particular, the key is to figure out who's calling GetBorderAndPadding and expecting that magic behaviour from nsGroupBoxFrame and nsGridRowLeafBoxFrame.

Comment 64

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

Or perhaps there are few enough callers using GetBorder or GetPadding that we could just adjust one of those the same way as GetBorderAndPadding.

Comment 65

[Andreas Lange]

Attachment: Part 3, v2

Ok, I'll have a look at the GetBorder-thing when I get a chance. 

Meanwhile, would you mind killing of the rest of the patch? GetInset() and helpers are now removed instead. Tested with reftests.

Comment 66

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

Comment on attachment 255881 [details] [diff] [review]
Part 3, v2

-  NS_IMETHOD GetAscent(nsBoxLayoutState& aBoxLayoutState, nscoord& aAscent);
+  virtual nscoord GetBoxAscent(nsBoxLayoutState& aBoxLayoutState);

This is fixing a regression from the earlier patch? You should mention that...

Comment 67

[Andreas Lange]

(In reply to comment #66)

> This is fixing a regression from the earlier patch? You should mention that...

Hm, thought I did that, seems not. No regression, it does the same as the overridden default but in a more straightforward manner. 

Comment 68

[Jesse Ruderman]

bz checked in the last patch at 2007-02-22 10:05.  Should this bug be marked as FIXED?

Comment 69

[Boris Zbarsky [:bzbarsky]]

There's more deCOM to do. e.g. GetBorder and company.

Comment 70

[David Zbarsky (:dzbarsky)]

Attachment: GetBorder, GetPadding, GetLayoutManager, etc.

Comment 71

[David Zbarsky (:dzbarsky)]

Attachment: GetBorder, GetPadding, GetLayoutManager, etc. v2

Comment 72

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

+  virtual void SetParent(const nsIFrame* aParent) { mParent = (nsIFrame*)aParent; }

Get rid of that case, it's useless...

+  virtual nsMargin GetPadding()=0;
+  virtual nsMargin GetMargin()=0;
+  virtual void SetLayoutManager(nsIBoxLayout* aLayout)=0;
+  virtual nsIBoxLayout* GetLayoutManager()=0;
+  virtual void RelayoutChildAtOrdinal(nsBoxLayoutState& aState, nsIBox* aChild)=0;

We generally write "= 0"

+	// XXX what was going on here before?
+    hRect.y = aScrollArea.YMost();

Probably just remove this comment.

+  return returnFrame->GetChildFrameContainingOffset(*aReturnOffset, aHint,
+											&aOffset);

Fix indent

 #if 0 //XXXrbs disable due to bug 310227
   if (mState & NS_FRAME_IS_DIRTY)
-    return NS_ERROR_UNEXPECTED;
-#endif
-
-  NS_ASSERTION(aOutOffset && aOutFrame, "Bad out parameters");
+	  return nsnull;

Fix indent, or just remove this code

+    nsCOMPtr<nsIBoxLayout> layout = child->GetLayoutManager();

Just make this a regular nsIBoxLayout* pointer. Lots of examples of this.

+      nsBox::gTheme->GetWidgetBorder(context->DeviceContext(), this,
+                                     disp->mAppearance, &margin);
+	  return nsMargin(context->DevPixelsToAppUnits(margin.left),
+		              context->DevPixelsToAppUnits(margin.top),
+	                  context->DevPixelsToAppUnits(margin.right),
+                      context->DevPixelsToAppUnits(margin.bottom));

The indenting here is a bit crazy...

     if (gTheme->ThemeSupportsWidget(context, this, disp->mAppearance)) {
+

Remove stray blank line

+		  return nsMargin(context->DevPixelsToAppUnits(margin.left),
+		                  context->DevPixelsToAppUnits(margin.top),
+	                      context->DevPixelsToAppUnits(margin.right),
+                          context->DevPixelsToAppUnits(margin.bottom));

More strange indenting

+virtual PRBool
+nsBoxFrame::GetDebug()
 {
-  aDebug = (mState & NS_STATE_CURRENTLY_IN_DEBUG);
-  return NS_OK;
+  return mState & NS_STATE_CURRENTLY_IN_DEBUG;

There's actually a bug here, this should be (mState & NS_STATE_CURRENTLY_IN_DEBUG) != 0 to guarantee this should be 0 or 1 (I know this already existed, but may as well fix it)

+	    nsMargin margin = child->GetMargin();
       childRect.Deflate(margin);

Fix indent

Thanks!!

Comment 73

[Zack Weinberg (:zwol)]

David, do you want someone to take over the revisions on this?

Comment 74

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

Comment on attachment 396449 [details] [diff] [review]
GetBorder, GetPadding, GetLayoutManager, etc. v2

Needs updating.

Zack, do you want to finish this off and get it in?

Comment 75

[Zack Weinberg (:zwol)]

I suppose I can do that, yeah.

Comment 76

[Marco Castelluccio [:marco]]

Was the bug resolved? Were the patches checked in?
Only the last patch isn't applied yet?

Comment 77

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

Review comments need to be addressed. I think all patches still need landing.

Comment 78

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Comment 79

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 80

[Daniel Holbert [:dholbert]]

Some of this has been addressed elsewhere (e.g. nsIFrame::GetOffsets in bug 1713491).

Some of it hasn't; e.g. we still have virtual nsresult GetChildFrameContainingOffset( whose nsresult return value could presumably be removed (that was one of the APIs that the patch here cleans up).

In any case: the patch here is unfortunately huge and would be prohibitively difficult to rebase, given the code changes over the last 12 years. So we can't use this bug/patch for much at this point other than for inspiration when rewriting these cleanups from scratch (which probably isn't high-priority).

Given we're already at 79 comments here, this isn't a great place to pick that work back up; let's just close this bug as INACTIVE and we can file new bugs as-needed for these cleanups.