Closed
Bug 313181
Opened 19 years ago
Closed 19 years ago
[FIX]use GetChildListNameFor more
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: roc, Assigned: bzbarsky)
Details
Attachments
(3 files)
17.00 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
14.28 KB,
patch
|
roc
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
20.07 KB,
patch
|
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
In ContentRemoved and probably other places, we analyse a frame to figure out which child list it should be removed from. This could just call GetChildListNameFor.
Reporter | ||
Comment 1•19 years ago
|
||
this is in nsCSSFrameConstructor
Assignee | ||
Comment 2•19 years ago
|
||
Assignee | ||
Comment 3•19 years ago
|
||
Summary of changes: 1) deCOMify GetChildListNameFor 2) Replace several places where we're looking at display/float values with a OUT_OF_FLOW bit check instead, since it's possible to be in-flow no matter what the CSS says if we didn't have the right containing blocks around. 3) In ContentRemoved, merge the popup/float/positioned codepaths as much as possible. All the placeholder handling is shared; the only thing special about out-of-flow popups is where they need to be removed from. Use GetChildListNameFor here to remove the float/positioned frames from the right list. Note that the ContentRemoved code was actually mis-indented, but happened to work because there was no '{' after the check for NS_STYLE_DISPLAY_POPUP... I've tested that the frame model looks correct both after loading a page with floats and abs pos elements and after modifying their style to put them in-flow.
Attachment #200567 -
Flags: superreview?(dbaron)
Attachment #200567 -
Flags: review?(roc)
Assignee | ||
Updated•19 years ago
|
Assignee: nobody → bzbarsky
Priority: -- → P2
Summary: use GetChildListNameFor more → [FIX]use GetChildListNameFor more
Target Milestone: --- → mozilla1.9alpha
Reporter | ||
Updated•19 years ago
|
Attachment #200566 -
Flags: review+
Reporter | ||
Updated•19 years ago
|
Attachment #200567 -
Flags: review?(roc) → review+
Comment on attachment 200567 [details] [diff] [review] Same as diff -w -- much easier to review Removing the null-check for placeholderFrame *could* be a little dangerous given that nsAbsoluteList::AddChild also sets the NS_FRAME_OUT_OF_FLOW bit, independently of whether NS_NewPlaceholderFrame failed. Fix that if you think you should, and sr=dbaron.
Attachment #200567 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 5•19 years ago
|
||
Yeah, that should be fixed. Luckily, I think I can assert in nsAbsoluteItems::AddChild that there is a placeholder for the child once I fix nsCSSFrameConstructor::CreateFloatingLetterFrame to check the rv from creating the placeholder. Other than that one, the only place that adds frames to an nsAbsoluteItems is nsFrameConstructorState::AddChild which constructs placeholders and checks the rv. Since a frame going into an nsAbsoluteItems can't be an ib special frame (they're all forced to be blocks), things should be good. I'll try to post an updated patch for this tonight.
Assignee | ||
Comment 6•19 years ago
|
||
The only differences from the previous patch are: 1) Assert added in nsAbsoluteItems::AddChild 2) The last two hunks in the patch -- changes to CreateFloatingLetterFrame to make sure it doesn't put things without placeholders into nsAbsoluteItems. I've tested that positioned and floated content still works and that floating letter frames work....
Attachment #201722 -
Flags: superreview?(dbaron)
(In reply to comment #6) > 2) The last two hunks in the patch -- changes to CreateFloatingLetterFrame to > make sure it doesn't put things without placeholders into nsAbsoluteItems. Any chance you could explain these changes? The diff just doesn't make much sense.
Assignee | ||
Comment 8•19 years ago
|
||
> Any chance you could explain these changes?
Sure thing. The old code did the following:
1) Create a placeholder frame. Ignore whether this actually succeeds.
2) Create a continuation frame for the text the first-letter frame did not grab
(so if the block starts with "Text", make the "ext" be in the continuation
frame). This part doesn't use the placeholder.
3) Add the floating letter frame to the mFloatList member of the state (an
nsAbsoluteItems)
4) Set aResult to contain nothing but the placeholder and the continuation
created in step 2, if any.
The new code does:
1) Create a continuation frame for the text as in step 2) before.
2) Assert that aResult is an empty frame list (verified to always be true by
code inspection).
3) Call AddChild on the state. This method will create the placeholder and add
the floating letter frame to the state's mFloatList (since it's display
IsFloating()). If creating a placeholder fails, it will destroy the letter
frame and return an error. If this method succeeds, it will add the
placeholder to aResult.
4) If step 1 created a continuation then:
a) If step 3 succeeded, add the continuation to aResult.
b) If step 3 failed, destroy the continuation
So the new code gives the exact same output as the old code if creating the placeholder succeeds and the assert in the new step 2 does not fire. If the assert fires, we will leave whatever used to be in aResult in it, instead of just forgetting about those frames. If creating a placeholder fails, we'll destroy all the newly-created stuff and return instead of putting a placeholder-less float in the float list.
Attachment #201722 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 9•19 years ago
|
||
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•