If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FIX]use GetChildListNameFor more

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: roc, Assigned: bz)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
this is in nsCSSFrameConstructor
Created attachment 200566 [details] [diff] [review]
Proposed patch
Created attachment 200567 [details] [diff] [review]
Same as diff -w -- much easier to review

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: nobody → bzbarsky
Priority: -- → P2
Summary: use GetChildListNameFor more → [FIX]use GetChildListNameFor more
Target Milestone: --- → mozilla1.9alpha
Attachment #200566 - Flags: review+
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+
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.
Created attachment 201722 [details] [diff] [review]
Patch with that fixed

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.
> 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+
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.