video controls RenderReplaced::layout() calls setNeedsLayout(false) while its children still need layout 
https://bugs.webkit.org/show_bug.cgi?id=49027
Summary video controls RenderReplaced::layout() calls setNeedsLayout(false) while its...
James Robinson
Reported 2010-11-04 14:30:57 PDT
The ASSERT()s in https://bugs.webkit.org/show_bug.cgi?id=49019 trigger on the following HTML: <!DOCTYPE html> <video controls></video> as well as most of the media/ tests. The render tree looks something like this: RenderView 0x3dc3cd8 #document 0x3dfed20 RenderBlock 0x3e3c968 HTML 0x3e3bf40 RenderBody 0x3e37968 BODY 0x3e3cd90 RenderVideo 0x3e4aa88 VIDEO 0x3e37a30 * RenderBlock (relative positioned) 0x3e4ae48 DIV 0x3e38950 RenderFlexibleBox (positioned) 0x3e48308 DIV 0x3e4b050 RenderButton 0x3e48808 INPUT 0x3e4c610 RenderFlexibleBox (positioned) 0x3e48ea8 DIV 0x3e4cb90 RenderBlock (positioned) 0x3e49438 DIV 0x3e4cc30 RenderText 0x3db0ed8 #text 0x3db0e50 "00:00" RenderSlider 0x3e499a8 INPUT 0x3e4ccd0 RenderBlock 0x3db0a08 DIV 0x3db01f0 RenderButton 0x3dafb88 INPUT 0x3e476d0 The RenderBlock root of the controls is marked as m_needsLayout=true, m_normalChildNeedsLayout=true, m_posChildNeedsLayout=true at the end of RenderReplaced::layout() on its parent RenderVideo.
Attachments
Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2010-11-04 14:39:28 PDT
Oooh, maybe RenderTreeAsText should optionally mark renderers that still need layout. If we did that in DRT, it might catch lots of bugs.
James Robinson
Comment 2 2010-11-04 15:13:58 PDT
(In reply to comment #1) > Oooh, maybe RenderTreeAsText should optionally mark renderers that still need layout. If we did that in DRT, it might catch lots of bugs. I like it. I also think that DRT should ASSERT that if any renderers are marked as needing layout at the end of the test that the appropriate layout timer is set and the layout root (if set) is an ancestor of all dirty objects.
James Robinson
Comment 3 2010-11-04 16:06:26 PDT
RenderReplaced::layout() isn't written to handle children and in fact RenderReplaced overrides canHaveChildren() to return false, however RenderMedia goes ahead and inserts a shadow RenderObject tree underneath itself.
Antti Koivisto
Comment 4 2010-11-05 09:11:10 PDT
(In reply to comment #3) > RenderReplaced::layout() isn't written to handle children and in fact RenderReplaced overrides canHaveChildren() to return false, however RenderMedia goes ahead and inserts a shadow RenderObject tree underneath itself. RenderMedia also overrides layout() which is supposed to handle this.
Dimitri Glazkov (Google)
Comment 5 2011-02-13 08:42:18 PST
I think I can fix this while I am in here.
Dimitri Glazkov (Google)
Comment 6 2011-04-11 14:17:53 PDT
(In reply to comment #5) > I think I can fix this while I am in here. One fix write RenderMedia::layout as full replacement for RenderReplaced::layout.
Dimitri Glazkov (Google)
Comment 7 2011-11-28 08:56:57 PST
(In reply to comment #5) > I think I can fix this while I am in here. I am no longer "in here" :(
Note You need to log in before you can comment on or make changes to this bug.