Closed
Bug 51767
Opened 24 years ago
Closed 19 years ago
[HLP][HBTN]HTMLButtonControlFrame should process its children more efficiently
Categories
(Core :: Layout: Form Controls, defect, P4)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: attinasi, Assigned: bernd_mozilla)
References
Details
Attachments
(3 files, 3 obsolete files)
23.89 KB,
patch
|
MatsPalmgren_bugz
:
review-
|
Details | Diff | Splinter Review |
27.85 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
3.78 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
The ButtonControlFrame is allowing the CSSFrameConstructor to process its
children, and it then has to go through those children and reparent them in
nsHTMLButtonControlFrame::SetInitialChildList. Maybe it should do his the way
the select does it?
Updated•24 years ago
|
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: --- → Future
Updated•24 years ago
|
Summary: HTMLButtonControlFrame should process its children more efficiently → [HBTN]HTMLButtonControlFrame should process its children more efficiently
Updated•24 years ago
|
Priority: P1 → P2
Target Milestone: Future → ---
Comment 2•24 years ago
|
||
This is a cleanup task
Summary: [HBTN]HTMLButtonControlFrame should process its children more efficiently → [HLP][HBTN]HTMLButtonControlFrame should process its children more efficiently
Target Milestone: --- → Future
Updated•23 years ago
|
Priority: P2 → P4
Oh man this code is broken,
1.) the button does create a area frame in the init function but does not tell
the frame constructor about it so it needs to reframe on every frame access.
2.) I believe that it would be more correct to create below every button a area
frame and make the functions AppendFrames, InsertFrames, and RemoveFrame
similiar to the nsTableCell variants, e.g. you can't alter the area frame but
only area frame children.
Mats are you able to review this or should I ask somebody else?
Comment 7•19 years ago
|
||
(In reply to comment #6)
> Mats are you able to review this or should I ask somebody else?
>
Sure, I can have a look at it tomorrow.
Comment on attachment 204212 [details] [diff] [review]
patch
Don't hurry, the code is wrong since years, I need this code to fix bug 316026
Attachment #204212 -
Flags: review?(mats.palmgren)
Comment 9•19 years ago
|
||
Comment on attachment 204212 [details] [diff] [review]
patch
Index: base/nsCSSFrameConstructor.cpp
+ ConstructButtonFrame(aState, aContent, aParentFrame,
+ aTag, aStyleContext, aFrame,
+ aStyleDisplay, aFrameHasBeenInitialized,
+ aFrameItems);
+ aAddedToFrameList = PR_TRUE;
return NS_UNLIKELY(!*aFrame) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;
I would prefer:
rv = ConstructButtonFrame(...); ...; return rv;
+nsCSSFrameConstructor::ConstructButtonFrame(nsFrameConstructorState& aState,
+ nsIContent* aContent,
+ nsIFrame* aParentFrame,
+ nsIAtom* aTag,
+ nsStyleContext* aStyleContext,
+ nsIFrame** aNewFrame,
+ const nsStyleDisplay* aStyleDisplay,
+ PRBool& aFrameHasBeenInitialized,
+ nsFrameItems& aFrameItems)
Return value NS_OK implies aFrameHasBeenInitialized is true so that parameter
is redundant.
+ // Initialize the button frame
+ InitAndRestoreFrame(aState, aContent,
+ aState.GetGeometricParent(aStyleDisplay, aParentFrame),
+ aStyleContext, nsnull, buttonFrame);
InitAndRestoreFrame can fail.
+ nsIFrame* areaFrame = NS_NewAreaFrame(mPresShell, NS_BLOCK_SPACE_MGR | NS_BLOCK_SHRINK_WRAP);
Add "NS_ENSURE_TRUE(areaFrame, NS_ERROR_OUT_OF_MEMORY);" here.
+ InitAndRestoreFrame(aState, aContent, buttonFrame, styleContext, nsnull,
+ areaFrame);
InitAndRestoreFrame can fail.
+ nsresult rv = aState.AddChild(buttonFrame, aFrameItems, aStyleDisplay, aContent,
+ aStyleContext, aParentFrame);
+ if (NS_FAILED(rv)) {
+ return rv;
+ }
Do we leak buttonFrame (and areaFrame) if AddChild() fails?
(this also applies to nsCSSFrameConstructor::ConstructHTMLFrame)
+ ProcessChildren(aState, aContent, areaFrame, PR_FALSE,
+ childItems, PR_TRUE);
ProcessChildren can fail.
You changed the 'aCanHaveGeneratedContent' argument from true to false, so
button::before does not work with this patch - is this intentional?
Changing 'aParentIsBlock' from false to true fixes bug 229425, but now it
also applies ::first-letter/line to <button> when it's inline.
CSS 2.1 says:
"The :first-letter pseudo-element applies to block, list-item, table-cell,
table-caption and inline-block elements."
Maybe "buttonFrame->GetStyleDisplay()->IsBlockLevel()" will do?
I don't see any call to CreateViewForFrame() in ConstructButtonFrame(),
we need that in case it's abs.pos. etc.
Index: forms/nsHTMLButtonControlFrame.cpp
- AddComputedBorderPaddingToDesiredSize(aDesiredSize, aReflowState);
+
You are leaving three empty lines here, which is two to many ;-)
Attachment #204212 -
Flags: review?(mats.palmgren) → review-
Assignee | ||
Comment 10•19 years ago
|
||
Assignee | ||
Comment 11•19 years ago
|
||
Comment on attachment 205074 [details] [diff] [review]
revised patch
this is rtest'ed
Attachment #205074 -
Flags: review?(mats.palmgren)
Comment 12•19 years ago
|
||
Comment on attachment 205074 [details] [diff] [review]
revised patch
nsCSSFrameConstructor.h:
>+ // ConstructButtonFrame puts the new frame in aFrameItems and
>+ // handles the kids of the button.
indent
nsHTMLButtonControlFrame.cpp:
>+ NS_PRECONDITION(PR_FALSE, "unsupported operation");
>+ return NS_ERROR_NOT_IMPLEMENTED;
Maybe s/NS_PRECONDITION/NS_NOTREACHED and
s/NS_ERROR_NOT_IMPLEMENTED/NS_ERROR_UNEXPECTED would
be more accurate if your intention is that these
methods should never be called?
r=mats
Attachment #205074 -
Flags: review?(mats.palmgren) → review+
Assignee | ||
Comment 13•19 years ago
|
||
Comment on attachment 205074 [details] [diff] [review]
revised patch
Robert,
the code that Mats proposes to change is a copy of the cellframe stuff http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableCellFrame.cpp#238
An other way would be to remove these functions at all and assert at the nsFrame level.
Attachment #205074 -
Flags: superreview?(roc)
+ // our new frame retured is the top frame which is the list frame.
"button frame"
> InitAndRestoreFrame can fail.
> ProcessChildren can fail.
These comments haven't been addressed yet.
Assignee | ||
Comment 15•19 years ago
|
||
Assignee: rods → bernd_mozilla
Attachment #207985 -
Flags: superreview?(roc)
Assignee | ||
Comment 16•19 years ago
|
||
Attachment #207985 -
Attachment is obsolete: true
Attachment #207986 -
Flags: superreview?(roc)
Attachment #207985 -
Flags: superreview?(roc)
+ ProcessChildren(aState, aContent, areaFrame, PR_TRUE,
+ childItems, buttonFrame->GetStyleDisplay()->IsBlockLevel());
Still need to check rv here.
Assignee | ||
Comment 18•19 years ago
|
||
Attachment #208111 -
Flags: superreview+
Attachment #208111 -
Flags: review+
Assignee | ||
Comment 19•19 years ago
|
||
fix checked in
whoa, a bug filed by my cvs sr 5 years ago
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 20•19 years ago
|
||
Er... why are we calling ProcessChildren() on an <input type="button">? Doesn't that assert in ProcessChildren (since IsLeaf() is true)? Not to mention just generally behaving wrong?
Also,
> + // the anonmyous content is allready parented to the area frame
should say "anonymous" and "already". But that's minor compared to the other thing.
Assignee | ||
Comment 21•19 years ago
|
||
Why should it assert? The area frame is not a leaf frame. ;-)
I will fix it soon.
Assignee | ||
Comment 22•19 years ago
|
||
Attachment #208486 -
Flags: superreview?(bzbarsky)
Attachment #208486 -
Flags: review?(bzbarsky)
Comment 23•19 years ago
|
||
Er.. you're passing the areaFrame to ProcessChildren? Doesn't that give kids the wrong style context (just like kids of table cells get the wrong style context)?
Comment 24•19 years ago
|
||
Comment on attachment 208486 [details] [diff] [review]
patch to address Boris' comments
r+sr=bzbarsky, I guess, but the style context parentage still needs to be fixed. I guess we could just file a separate bug on that and make it depend on the table cell one and hope we solve the general problem in the 1.9 timeframe...
Attachment #208486 -
Flags: superreview?(bzbarsky)
Attachment #208486 -
Flags: superreview+
Attachment #208486 -
Flags: review?(bzbarsky)
Attachment #208486 -
Flags: review+
Comment 25•19 years ago
|
||
I guess this code was already screwing up the style context parentage, though... It's still a bug, but we might have it on file already. Check?
Assignee | ||
Comment 26•19 years ago
|
||
fix checked in, I had a discussion with Boris on IRC, I will file the necessary followup bugs.
Assignee | ||
Comment 27•19 years ago
|
||
The broken inheritance is filed under bug 323656
Attachment #205074 -
Attachment is obsolete: true
Attachment #205074 -
Flags: superreview?(roc)
Attachment #207986 -
Attachment is obsolete: true
Attachment #207986 -
Flags: superreview?(roc)
Comment 28•19 years ago
|
||
Hmm... So how come this removed the |triedFrame = PR_TRUE;| line for <button>? Why was that no longer needed?
Comment 29•19 years ago
|
||
Please ignore comment 28 -- I figured it out....
You need to log in
before you can comment on or make changes to this bug.
Description
•