deCOMtaminate parts of nsIFrame that come from nsIBox
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(7 files, 10 obsolete files)
92.33 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
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+
roc
:
superreview+
|
Details | Diff | Splinter Review |
96.86 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
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()?
Comment 1•20 years ago
|
||
I'm taking this. Going to be my first steps into mozilla code (as suggested by Robert O'Callahan).
Updated•20 years ago
|
Comment 2•20 years ago
|
||
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•20 years ago
|
||
Setting optimistic milestone
Updated•20 years ago
|
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•20 years ago
|
||
> +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
This bug is somewhat obsolete because bryner is merging nsIBox into nsIFrame. We'll still need this deCOMtamination in nsIFrame though.
Comment 9•20 years ago
|
||
Henrik is inactive per roc, assign to "nobody".
Comment 10•20 years ago
|
||
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•20 years ago
|
||
bug 258513 merged nsIBox into nsIFrame, adding dependency
Comment 12•19 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.
Updated•19 years ago
|
Comment 13•19 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.
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.
Comment 18•19 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?
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•19 years ago
|
||
Ah, that makes more sense. :-)
Updated•19 years ago
|
Comment 21•19 years ago
|
||
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 ?
All of them.
Comment 23•18 years ago
|
||
screw your mom
Comment 24•18 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.
Comment 25•18 years ago
|
||
Yes, I'll continue on this. Parts of the v2 patch have already landed in bug 363879.
Comment 26•18 years ago
|
||
Only a few parts of the old patch still applied.
Updated•18 years ago
|
Comment 27•18 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•18 years ago
|
Reporter | ||
Comment 28•18 years ago
|
||
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.
Reporter | ||
Comment 31•18 years ago
|
||
Except BoundsCheck needs to return two separate things, since it changes the pref _and_ max size/coord as I said.
Reporter | ||
Comment 32•18 years ago
|
||
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&)".
Reporter | ||
Comment 34•18 years ago
|
||
Comment 35•18 years ago
|
||
Sorry for the bustages, seems that I should test on gcc as well.
Comment 36•18 years ago
|
||
Should this be resolved now that the patch was checked in, or is there more work to do?
There's more work to do.
Comment 38•18 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.
Comment 39•18 years ago
|
||
Comment 40•18 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.
Comment 42•18 years ago
|
||
> > 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.
Comment 44•18 years ago
|
||
Comment 45•18 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.
Comment 47•18 years ago
|
||
I had missed to add a MoveTo(0,0) there, thanks.
// ----- 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 :-)
Comment 50•18 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().
Updated•18 years ago
|
Comment 51•18 years ago
|
||
Comment on attachment 253459 [details] [diff] [review] Part 2, v2.2 - final improvements made? Checked in
Updated•18 years ago
|
Comment 52•18 years ago
|
||
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?
Comment 54•18 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. :-)
Reporter | ||
Comment 55•18 years ago
|
||
That last patch missed a GetCollapsed() implementation. See bug 369127.
Comment 56•18 years ago
|
||
- 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•18 years ago
|
||
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.
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.
Comment 65•17 years ago
|
||
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 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•17 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.
Updated•17 years ago
|
Comment 68•17 years ago
|
||
bz checked in the last patch at 2007-02-22 10:05. Should this bug be marked as FIXED?
Reporter | ||
Comment 69•17 years ago
|
||
There's more deCOM to do. e.g. GetBorder and company.
Comment 70•15 years ago
|
||
Updated•15 years ago
|
Comment 71•15 years ago
|
||
+ 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•14 years ago
|
||
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?
Comment 75•14 years ago
|
||
I suppose I can do that, yeah.
Comment 76•13 years ago
|
||
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•6 years ago
|
Assignee | ||
Updated•6 years ago
|
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•2 years ago
|
||
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•2 years ago
•
|
||
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.
Description
•