Bug 61897

Summary: Flash of broken page when exiting full screen at jerryseinfeld.com
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: WebCore Misc.Assignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61911    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Jer Noble 2011-06-01 17:42:16 PDT
Flash of broken page when exiting full screen at jerryseinfeld.com
Comment 1 Jer Noble 2011-06-01 22:44:17 PDT
Created attachment 95724 [details]
Patch
Comment 2 Jer Noble 2011-06-01 22:44:40 PDT
<rdar://problem/9522985>
Comment 3 Jer Noble 2011-06-02 00:45:25 PDT
Created attachment 95732 [details]
Patch

Added radar URL; moved some code between the two patches to allow them to apply cleanly.
Comment 4 Simon Fraser (smfr) 2011-06-02 12:04:20 PDT
Comment on attachment 95724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95724&action=review

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=61897

There should be a brief summary of the fix here.

> Source/WebCore/dom/Document.cpp:4892
> +    bool shouldCreatePlaceholder = m_fullScreenElement->renderer()->isBox();

Why does being a box require a placeholder? Needs an explanatory comment.

> Source/WebCore/dom/Document.cpp:4894
> +    IntRect frameRect = renderer->isBox() ? toRenderBox(renderer)->frameRect() : IntRect();

Why test renderer->isBox() rather than shouldCreatePlaceholder? Seems like an if () clause here would be easier to read.

> Source/WebCore/dom/Document.cpp:4976
> +        renderer->createPlaceholder(RenderStyle::clone(placeholder->style()), placeholder->frameRect());

So the style has been cloned twice?

> Source/WebCore/rendering/RenderFullScreen.cpp:102
> +    if (!m_placeholder) {
> +        m_placeholder = new (document()->renderArena()) RenderBlock(document());
> +        m_placeholder->setStyle(style);
> +        m_placeholder->setIsAnonymous(false);
> +        if (parent()) {
> +            parent()->addChild(m_placeholder, this);
> +            remove();
> +            m_placeholder->addChild(this);
> +        }
> +    } else
> +        m_placeholder->setStyle(style);
> +    m_placeholder->setFrameRect(frameRect);

It's weird and a bit scarey that you're manipulating the render tree outside of layout, and expecting that the frameRect that you set will not be changed by layout.
Comment 5 Jer Noble 2011-06-02 14:27:49 PDT
Comment on attachment 95724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95724&action=review

>> Source/WebCore/ChangeLog:6
>> +        https://bugs.webkit.org/show_bug.cgi?id=61897
> 
> There should be a brief summary of the fix here.

Sure.

>> Source/WebCore/dom/Document.cpp:4892
>> +    bool shouldCreatePlaceholder = m_fullScreenElement->renderer()->isBox();
> 
> Why does being a box require a placeholder? Needs an explanatory comment.

Only a RenderBox and its subclasses has a frameRect().  I'll add a comment.

>> Source/WebCore/dom/Document.cpp:4894
>> +    IntRect frameRect = renderer->isBox() ? toRenderBox(renderer)->frameRect() : IntRect();
> 
> Why test renderer->isBox() rather than shouldCreatePlaceholder? Seems like an if () clause here would be easier to read.

This could be shouldCreatePlaceholder.  I'll change it.

>> Source/WebCore/dom/Document.cpp:4976
>> +        renderer->createPlaceholder(RenderStyle::clone(placeholder->style()), placeholder->frameRect());
> 
> So the style has been cloned twice?

Or many more times, depending on how many times this function is called while within full-screen mode.

I guess that, since RenderStyles are RefCounted, we don't have to clone them, but I didn't like the idea that two renderers might point to the same style, no matter how briefly.

>> Source/WebCore/rendering/RenderFullScreen.cpp:102
>> +    m_placeholder->setFrameRect(frameRect);
> 
> It's weird and a bit scarey that you're manipulating the render tree outside of layout, and expecting that the frameRect that you set will not be changed by layout.

It very well might be changed by layout.  If the style says, e.g., "width: 50%" I would absolutely expect that the frameRect will change during layout.  But it should change in a similar way to the full-screen element.  

And actually this function is called from within layout, in the case of wrapWithRenderFullScreen() above.  I could modify webkitWillEnterFullScreenForElement() so that the placeholder is added during layout instead of directly after, but that would require adding two new ivars to Document.
Comment 6 Beth Dakin 2011-06-02 16:02:50 PDT
I lived on this patch for a little while and did lot of video watching. Seems good!
Comment 7 Maciej Stachowiak 2011-06-03 10:38:09 PDT
Comment on attachment 95732 [details]
Patch

Unflagging - Jer will be revising to address comments.
Comment 8 Jer Noble 2011-06-03 11:05:20 PDT
Created attachment 95928 [details]
Patch

Addressed Simon's comments by ensuring the creation of the placeholder renderer occurs entirely within layout.
Comment 9 Jer Noble 2011-06-03 11:06:05 PDT
Comment on attachment 95928 [details]
Patch

Whoops, somehow webkit-patch picked up two patches. Obseleting.
Comment 10 Jer Noble 2011-06-03 11:06:39 PDT
Created attachment 95930 [details]
Patch

Addressed Simon's comments by ensuring the creation of the placeholder renderer occurs entirely within layout.
Comment 11 Simon Fraser (smfr) 2011-06-03 11:13:34 PDT
Comment on attachment 95930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95930&action=review

> Source/WebCore/dom/Document.cpp:4788
> +    // Only create a placeholder block for a RenderBox, as only a box will have a frameRect.

I'd like this comment to say what you say in the changelog about the fullscreen element being taken out of normal flow. Otherwise the role of the placeholder is unclear.

> Source/WebCore/rendering/RenderFullScreen.cpp:107
> +    m_placeholder->setFrameRect(frameRect);

Is this necessary? Won't layout happen and set the frame?
Comment 12 Jer Noble 2011-06-03 11:27:43 PDT
Comment on attachment 95930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95930&action=review

>> Source/WebCore/dom/Document.cpp:4788
>> +    // Only create a placeholder block for a RenderBox, as only a box will have a frameRect.
> 
> I'd like this comment to say what you say in the changelog about the fullscreen element being taken out of normal flow. Otherwise the role of the placeholder is unclear.

Sure thing.

>> Source/WebCore/rendering/RenderFullScreen.cpp:107
>> +    m_placeholder->setFrameRect(frameRect);
> 
> Is this necessary? Won't layout happen and set the frame?

Not for the case of an element whose width and height are dependent on their contents.  Since the placeholder doesn't have any contents in the normal flow, it's width and height will remain zero. (In the case that the style's width or height are auto.)
Comment 13 Jer Noble 2011-06-03 11:35:54 PDT
(In reply to comment #12)
> (From update of attachment 95930 [details])
> >> Source/WebCore/rendering/RenderFullScreen.cpp:107
> >> +    m_placeholder->setFrameRect(frameRect);
> > 
> > Is this necessary? Won't layout happen and set the frame?
> 
> Not for the case of an element whose width and height are dependent on their contents.  Since the placeholder doesn't have any contents in the normal flow, it's width and height will remain zero. (In the case that the style's width or height are auto.)

Oh, I see what you're saying; we reset the style's width and height if they're auto, so these values should get replaced during layout.  Let me try that out.
Comment 14 Jer Noble 2011-06-03 11:44:47 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 95930 [details] [details])
> > >> Source/WebCore/rendering/RenderFullScreen.cpp:107
> > >> +    m_placeholder->setFrameRect(frameRect);
> > > 
> > > Is this necessary? Won't layout happen and set the frame?
> > 
> > Not for the case of an element whose width and height are dependent on their contents.  Since the placeholder doesn't have any contents in the normal flow, it's width and height will remain zero. (In the case that the style's width or height are auto.)
> 
> Oh, I see what you're saying; we reset the style's width and height if they're auto, so these values should get replaced during layout.  Let me try that out.

Yes, it looks like this is unnecessary.  I'll remove the frameRect stuff from the patch.
Comment 15 Jer Noble 2011-06-03 11:48:25 PDT
Committed r88034: <http://trac.webkit.org/changeset/88034>