Bug 126536 – TileController can fail to receive exposedRect from the drawing area if set at the wrong time

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tim Horton]

If something tries to update the view exposed rect before we have a TileController, the TileController won't get the exposed rect even when it is created.

I'm going to flip this around to pull the exposed rect from FrameView, so it will always be valid, and do some cleanup.

[Tim Horton]

Created attachment 220478 [details]
patch

[Anders Carlsson]

Comment on attachment 220478 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +        TileController can fail to receive exposedRect from the drawing area if if set at the wrong time

if if.

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:100
> +    virtual FloatRect exposedRect() const { return FloatRect(); }

I think this should return the infinite rect?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:308
> +void TileController::exposedRectChanged()

DidChange?

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:378
> +    FloatRect visibleRect = m_rootLayer.get().frame;

[m_rootLayer frame];

[Tim Horton]

http://trac.webkit.org/changeset/161447

[Simon Fraser (smfr)]

Comment on attachment 220478 [details]
patch

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

> Source/WebCore/page/FrameView.cpp:4384
> +    m_exposedRect = exposedRect;
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +    // FIXME: We should support clipping to the exposed rect for subframes as well.
> +    if (m_frame->isMainFrame()) {
> +        if (TiledBacking* tiledBacking = this->tiledBacking())
> +            tiledBacking->exposedRectChanged();
> +    }

This doesn't check if exposedRectChanged.

> Source/WebCore/page/FrameView.h:636
> +    FloatRect m_exposedRect;

Maybe a comment to say which flavor of "exposed", and what coordinate system this is in.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2608
> +FloatRect RenderLayerBacking::exposedRect() const
> +{
> +    // FIXME: We should support clipping to the exposed rect for subframes as well.
> +    if (m_isMainFrameRenderViewLayer)
> +        return owningLayer().renderer().view().frameView().exposedRect();
> +
> +    return FloatRect::infiniteRect();
> +}

This makes me sad. Currently flushing graphics layers computes a visible rect ('exposedness")  for each layer based on the rect passed into flushCompositingState().

You've added an alternate way to get at the exposed rect, and one which is broken for all layers other than the main frame's layer.

Do we even need this, given that GraphicsLayerCA has FloatRect m_visibleRect; ?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126592

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 220478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220478&action=review
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:2608
> > +FloatRect RenderLayerBacking::exposedRect() const
> > +{
> > +    // FIXME: We should support clipping to the exposed rect for subframes as well.
> > +    if (m_isMainFrameRenderViewLayer)
> > +        return owningLayer().renderer().view().frameView().exposedRect();
> > +
> > +    return FloatRect::infiniteRect();
> > +}
> 
> This makes me sad. Currently flushing graphics layers computes a visible rect ('exposedness")  for each layer based on the rect passed into flushCompositingState().
> 
> You've added an alternate way to get at the exposed rect, and one which is broken for all layers other than the main frame's layer.
> 
> Do we even need this, given that GraphicsLayerCA has FloatRect m_visibleRect; ?

After re-discussing http://trac.webkit.org/changeset/139822 and this patch, smfr sez "so why not store it, as you are doing, then push it onto the TiledBacking when the TiledBacking is created? (and revert the confusing GraphicsLayerClient part)". Which I like better than this mess, so I'm going to do that.

[Tim Horton]

Created attachment 220568 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 220568 [details]
patch

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

> Source/WebCore/page/FrameView.h:449
> +    void setExposedRect(FloatRect);
> +    FloatRect exposedRect() const { return m_exposedRect; }

Please add comments to say what "exposed" means and why it's different from visibleContentRect etc.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:55
> +    , m_exposedRect(FloatRect::infiniteRect())

Sad that this exposedRect only makes sense for the main TileController.

[EFL EWS Bot]

Comment on attachment 220568 [details]
patch

Attachment 220568 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6233419985977344

[Tim Horton]

(In reply to comment #9)
> (From update of attachment 220568 [details])
> Attachment 220568 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/6233419985977344

c++: internal compiler error: Killed (program cc1plus)

[Tim Horton]

http://trac.webkit.org/changeset/161530