Bug 61897 – Flash of broken page when exiting full screen at jerryseinfeld.com

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jer Noble]

Flash of broken page when exiting full screen at jerryseinfeld.com

[Jer Noble]

Created attachment 95724 [details]
Patch

[Jer Noble]

<rdar://problem/9522985>

[Jer Noble]

Created attachment 95732 [details]
Patch

Added radar URL; moved some code between the two patches to allow them to apply cleanly.

[Simon Fraser (smfr)]

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.

[Jer Noble]

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.

[Beth Dakin]

I lived on this patch for a little while and did lot of video watching. Seems good!

[Maciej Stachowiak]

Comment on attachment 95732 [details]
Patch

Unflagging - Jer will be revising to address comments.

[Jer Noble]

Created attachment 95928 [details]
Patch

Addressed Simon's comments by ensuring the creation of the placeholder renderer occurs entirely within layout.

[Jer Noble]

Comment on attachment 95928 [details]
Patch

Whoops, somehow webkit-patch picked up two patches. Obseleting.

[Jer Noble]

Created attachment 95930 [details]
Patch

Addressed Simon's comments by ensuring the creation of the placeholder renderer occurs entirely within layout.

[Simon Fraser (smfr)]

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?

[Jer Noble]

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.)

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

Committed r88034: <http://trac.webkit.org/changeset/88034>