Bug 126536 - TileController can fail to receive exposedRect from the drawing area if set at the wrong time
Summary: TileController can fail to receive exposedRect from the drawing area if set a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on: 126537 126592
Blocks: 126531
  Show dependency treegraph
 
Reported: 2014-01-06 12:51 PST by Tim Horton
Modified: 2014-01-08 17:34 PST (History)
9 users (show)

See Also:


Attachments
patch (43.19 KB, patch)
2014-01-06 18:07 PST, Tim Horton
andersca: review+
Details | Formatted Diff | Diff
patch (39.87 KB, patch)
2014-01-07 16:36 PST, Tim Horton
simon.fraser: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-01-06 12:51:44 PST
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.
Comment 1 Tim Horton 2014-01-06 18:07:32 PST
Created attachment 220478 [details]
patch
Comment 2 Anders Carlsson 2014-01-06 18:13:26 PST
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];
Comment 3 Tim Horton 2014-01-07 12:48:43 PST
http://trac.webkit.org/changeset/161447
Comment 4 Simon Fraser (smfr) 2014-01-07 12:57:58 PST
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; ?
Comment 5 WebKit Commit Bot 2014-01-07 13:20:17 PST
Re-opened since this is blocked by bug 126592
Comment 6 Tim Horton 2014-01-07 13:42:32 PST
(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.
Comment 7 Tim Horton 2014-01-07 16:36:04 PST
Created attachment 220568 [details]
patch
Comment 8 Simon Fraser (smfr) 2014-01-07 16:49:50 PST
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.
Comment 9 EFL EWS Bot 2014-01-07 17:52:43 PST
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
Comment 10 Tim Horton 2014-01-08 03:07:10 PST
(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)
Comment 11 Tim Horton 2014-01-08 17:34:42 PST
http://trac.webkit.org/changeset/161530