Bug 93040 - [BlackBerry] Upstream GraphicsLayerClient::contentsVisible()
Summary: [BlackBerry] Upstream GraphicsLayerClient::contentsVisible()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arvid Nilsson
URL:
Keywords:
Depends on:
Blocks: 93036
  Show dependency treegraph
 
Reported: 2012-08-02 16:16 PDT by Arvid Nilsson
Modified: 2012-08-03 06:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2012-08-02 16:36 PDT, Arvid Nilsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Nilsson 2012-08-02 16:16:10 PDT
Our LayerTiler uses both proactive and reactive rendering to populate tiles. If contentsVisible() is accurate, it will cause the right tiles to be rendered. Failing that, when a dirty tile is found to be visible on the compositing thread, a render job is scheduled.
Comment 1 Arvid Nilsson 2012-08-02 16:36:10 PDT
Created attachment 156199 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-02 18:15:15 PDT
Comment on attachment 156199 [details]
Patch

Clearing flags on attachment: 156199

Committed r124544: <http://trac.webkit.org/changeset/124544>
Comment 3 WebKit Review Bot 2012-08-02 18:15:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Antonio Gomes 2012-08-03 05:48:34 PDT
Comment on attachment 156199 [details]
Patch

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

Post-land comments:

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:89
> +    virtual bool contentsVisible(const GraphicsLayer*, const IntRect& contentRect) const = 0;

but it would be preferable to add a default blank implementation here, instead of a pure virtual method, and get rid of the #if platform(bb)?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1632
> +    IntRect visibleContentRect(view->visibleContentRect());
> +    FloatQuad absoluteContentQuad = renderer()->localToAbsoluteQuad(FloatRect(localContentRect));
> +    return absoluteContentQuad.enclosingBoundingBox().intersects(visibleContentRect);

does this work for subframes?
Comment 5 Arvid Nilsson 2012-08-03 05:57:36 PDT
Gnnh, I forgot to add the declaration to RenderLayerBacking.h
Comment 6 Arvid Nilsson 2012-08-03 06:04:09 PDT
Comment on attachment 156199 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsLayerClient.h:89
>> +    virtual bool contentsVisible(const GraphicsLayer*, const IntRect& contentRect) const = 0;
> 
> but it would be preferable to add a default blank implementation here, instead of a pure virtual method, and get rid of the #if platform(bb)?

There's a default implementation coming in https://bugs.webkit.org/show_bug.cgi?id=93036, however it's still not enabling it for all platforms.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1632
>> +    return absoluteContentQuad.enclosingBoundingBox().intersects(visibleContentRect);
> 
> does this work for subframes?

I think this is actually returns true too often, which is better than too seldom - if my thinking is correct, this will return true if the layer is visible in the subframe's viewport, even if the whole viewport is scrolled off screen.
Comment 7 Antonio Gomes 2012-08-03 06:09:24 PDT
> > does this work for subframes?
> 
> I think this is actually returns true too often, which is better than too seldom - if my thinking is correct, this will return true if the layer is visible in the subframe's viewport, even if the whole viewport is scrolled off screen.

I do not think it clips against its parent viewport.
Comment 8 Arvid Nilsson 2012-08-03 06:21:24 PDT
(In reply to comment #7)
> > > does this work for subframes?
> > 
> > I think this is actually returns true too often, which is better than too seldom - if my thinking is correct, this will return true if the layer is visible in the subframe's viewport, even if the whole viewport is scrolled off screen.
> 
> I do not think it clips against its parent viewport.

Exactly, I guess that was what I was trying to say. So it will return true too often (false positive) but that's ok considering the use case - when to populate tiles. ,maybe there is benefit from making it more accurate, though.