deCOMtaminate parts of nsIFrame that come from nsIBox

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
15 years ago
9 months ago

People

(Reporter: bzbarsky, Assigned: anlan)

Tracking

(Blocks 1 bug)

Trunk
mozilla1.9alpha3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 10 obsolete attachments)

92.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
866 bytes, patch
Details | Diff | Splinter Review
1.73 KB, patch
Details | Diff | Splinter Review
2.12 KB, patch
Details | Diff | Splinter Review
93.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
96.86 KB, patch
roc
: review+
Details | Diff | Splinter Review
90.51 KB, patch
Details | Diff | Splinter Review
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()?
I'm taking this.

Going to be my first steps into mozilla code (as suggested by Robert O'Callahan). 
Status: NEW → ASSIGNED
Assignee: nobody → admin
Status: ASSIGNED → NEW
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;
Setting optimistic milestone
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
Attachment #149215 - Flags: superreview?
Attachment #149215 - Flags: review?(roc)
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
> +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..
> 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 on attachment 149215 [details] [diff] [review]
GetPreferredSize,GetMinimumSize,GetMaximumSize

minusing in anticipation of new patch
Attachment #149215 - Flags: superreview?
Attachment #149215 - Flags: superreview-
Attachment #149215 - Flags: review?(roc)
Attachment #149215 - Flags: review-
This bug is somewhat obsolete because bryner is merging nsIBox into nsIFrame.
We'll still need this deCOMtamination in nsIFrame though.
Henrik is inactive per roc, assign to "nobody".
Assignee: henrik → nobody
Status: ASSIGNED → NEW
Assignee: nobody → nobody
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.
Assignee: nobody → bugmail
Target Milestone: mozilla1.8beta → mozilla1.9alpha
bug 258513 merged nsIBox into nsIFrame, adding dependency
Depends on: 258513
Assignee

Comment 12

14 years ago
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.
Attachment #149215 - Attachment is obsolete: true
Attachment #190017 - Flags: review?(roc)
Assignee

Updated

14 years ago
OS: Linux → All
Hardware: PC → All
Summary: deCOMtaminate nsIBox → deCOMtaminate nsIFrame
Assignee

Comment 13

14 years ago
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.
OS: All → Linux
Hardware: All → PC
Summary: deCOMtaminate nsIFrame → deCOMtaminate parts of nsIFrame that come from nsIBox
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 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".
But yeah we should not land this until the reflow branch lands.
Andreas, please attach a new patch. I'll finish the review of it and then land
it after the reflow branch has landed.
Assignee

Comment 18

14 years ago
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?
Assignee: vhaarr+bmo → mozilla
Attachment #190017 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #193829 - Flags: review?(roc)
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.
Assignee

Comment 20

14 years ago
Ah, that makes more sense. :-)
Assignee

Updated

14 years ago
Attachment #193829 - Flags: review?(roc)
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 23

13 years ago
screw your mom

Comment 24

13 years ago
(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.
Assignee

Comment 25

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

Comment 26

13 years ago
Only a few parts of the old patch still applied.
Attachment #193829 - Attachment is obsolete: true
Assignee

Updated

13 years ago
Attachment #250521 - Flags: review?(roc)
Assignee

Comment 27

13 years ago
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!

Updated

13 years ago
Whiteboard: [checkin needed]
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).
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).
Though I suppose it would better to apply your fix everywhere and fix BoundsCheck separately.
Except BoundsCheck needs to return two separate things, since it changes the pref _and_ max size/coord as I said.
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&)".
Assignee

Comment 35

13 years ago
Sorry for the bustages, seems that I should test on gcc as well.  
Should this be resolved now that the patch was checked in, or is there more work to do?
Whiteboard: [checkin needed]
Assignee

Comment 38

13 years ago
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.
Assignee

Comment 39

13 years ago
Posted patch Part 2, v1 (obsolete) — Splinter Review
Assignee

Comment 40

13 years ago
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);
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.
Assignee

Comment 42

13 years ago
Posted patch BoxMetrics reduction, diff -b (obsolete) — Splinter Review
> > 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?
Roll those changes into the current patch and we'll do this patch now before we move on to any new functions.
Assignee

Comment 44

13 years ago
Posted patch Part 2, v2 (obsolete) — Splinter Review
Attachment #252309 - Attachment is obsolete: true
Attachment #253182 - Attachment is obsolete: true
Attachment #253306 - Flags: review?(roc)
Assignee

Comment 45

13 years ago
(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?
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.
Assignee

Comment 47

13 years ago
Posted patch Part 2, v2.1 - comments fixed (obsolete) — Splinter Review
I had missed to add a MoveTo(0,0) there, thanks.
Attachment #253306 - Attachment is obsolete: true
Attachment #253315 - Flags: review?(roc)
Attachment #253306 - Flags: review?(roc)
   // ----- 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 :-)
   // ----- 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 :-)
Assignee

Comment 50

13 years ago
|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().
Attachment #253315 - Attachment is obsolete: true
Attachment #253459 - Flags: review?(roc)
Attachment #253315 - Flags: review?(roc)
Assignee

Updated

13 years ago
OS: Linux → All
Hardware: PC → All
Whiteboard: [checkin needed]
Comment on attachment 253459 [details] [diff] [review]
Part 2, v2.2 - final improvements made?

Checked in

Updated

13 years ago
Whiteboard: [checkin needed]
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 ;-)
(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?
Assignee

Comment 54

13 years ago
(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. :-)

Depends on: 369127
That last patch missed a GetCollapsed() implementation.  See bug 369127.
Assignee

Comment 56

13 years ago
Posted patch Part 3, v1 (obsolete) — Splinter Review
-  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...
Attachment #253786 - Flags: review?(roc)
Assignee

Comment 57

13 years ago
Posted patch Part 3, v1 - updated to tree (obsolete) — Splinter Review
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.
Attachment #253786 - Attachment is obsolete: true
Attachment #253786 - Flags: review?(roc)
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?
(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.
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.
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.
Maybe we should sort out that GetBorderAndPadding issue first and save ourselves an extra mass rename?
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.
Or perhaps there are few enough callers using GetBorder or GetPadding that we could just adjust one of those the same way as GetBorderAndPadding.
Assignee

Comment 65

12 years ago
Posted patch Part 3, v2Splinter Review
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.
Attachment #253815 - Attachment is obsolete: true
Attachment #255881 - Flags: review?(roc)
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...
Attachment #255881 - Flags: superreview+
Attachment #255881 - Flags: review?(roc)
Attachment #255881 - Flags: review+
Assignee

Comment 67

12 years ago
(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. 

Target Milestone: mozilla1.9alpha1 → M1
Target Milestone: M1 → mozilla1.9alpha3

Comment 68

12 years ago
bz checked in the last patch at 2007-02-22 10:05.  Should this bug be marked as FIXED?
There's more deCOM to do. e.g. GetBorder and company.
Attachment #394299 - Attachment is obsolete: true
Attachment #394299 - Flags: review?(roc)
+  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!!
Blocks: deCOM
David, do you want someone to take over the revisions on this?
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?
Attachment #396449 - Flags: review?(roc)
I suppose I can do that, yeah.
Was the bug resolved? Were the patches checked in?
Only the last patch isn't applied yet?
Review comments need to be addressed. I think all patches still need landing.

Updated

9 months ago
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.