Bug 49027 - video controls RenderReplaced::layout() calls setNeedsLayout(false) while its children still need layout
Summary: video controls RenderReplaced::layout() calls setNeedsLayout(false) while its...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 53020
Blocks: 49019
  Show dependency tree / graph
 
Reported: 2010-11-04 14:30 PDT by James Robinson
Modified: 2011-11-28 08:56 PST (History)
6 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 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.
Comment 1 Simon Fraser (smfr) 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.
Comment 2 James Robinson 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.
Comment 3 James Robinson 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.
Comment 4 Antti Koivisto 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.
Comment 5 Dimitri Glazkov (Google) 2011-02-13 08:42:18 PST 
I think I can fix this while I am in here.
Comment 6 Dimitri Glazkov (Google) 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.
Comment 7 Dimitri Glazkov (Google) 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" :(