[Webkit-unassigned] [Bug 72686] [chromium] Need to prepaint tiles in TiledLayerChromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72686





--- Comment #12 from Adrienne Walker <enne at google.com>  2011-11-28 15:27:31 PST ---
(From update of attachment 116562)
View in context: https://bugs.webkit.org/attachment.cgi?id=116562&action=review

This is looking a lot better.  Thanks for all the changes.

I'm still worried about the TextureManager-related concerns that I brought up above.  For example, ManagedTexture::isValid (called via needsIdlePaint) touches the LRU queue, which will put offscreen textures behind onscreen textures regarding eviction.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

I'd love to see some unit tests for this.  If you need any pointers about where to look, what to mock out, or where to start, I'm happy to help.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:164
> +    virtual void paintContentsIfDirty(bool idlePaint) { }

I know you're trying to reuse the layer iterating code in CCLayerTreeHost, but it's awkward to pass a boolean for custom behavior here.  reveman has a patch out that makes iterating through layers less painful, and I think it'll be cleaner to just have separate paintContentsIfDirty(void) and idlePaintContentsIfDirty(void) functions.  See: https://bugs.webkit.org/attachment.cgi?id=116293

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:42
> +size_t TextureManager::reclaimLimitBytes(const IntSize& viewportSize)
>  {
> +    // FIXME: Choose a limit based on viewport size.

I think any adjusting of texture limits based on viewport size should be done in a separate patch.  jamesr did something similar here: https://bugs.webkit.org/show_bug.cgi?id=72202

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:426
> +    idlePaintContentRect.inflate(idlePaintPaddingWidth);

I don't think idlePaintPaddingWidth should explicitly be the viewport size, because that won't make any sense for tiled non-root layers.

The contentRect here is the part of the layer that's visible.  For the root layer, this will always be the clip layer, which is sized to the viewport.  For iframes, the contentRect will (most often, unless it's clipped by some other masking layer or is partially offscreen) be the size of its clip rect as well, which is the portion of it that will be visible.  I think it'd be sufficient to use contentRect as an approximation for the viewport size.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:439
> +    int l1, t1, r1, b1, l2, t2, r2, b2;

style nit: Abbreviations are frowned upon in WebKit.  Maybe visibleLeft or contentLeft vs. offscreenLeft or prepaintLeft?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:447
> +    while ((l1 > l2 || t1 > t2 || r1 < r2 || b1 < b2) && !m_skipsIdlePaint) {
> +        if (b1 < b2) {
> +            ++b1;
> +            prepareToUpdateTiles(true, l1, b1, r1, b1);
> +            if (!m_paintRect.isEmpty() || m_skipsIdlePaint)
> +                break;

m_skipsIdlePaint seems kind of misnamed.  It's more "idle painting ran out of memory", and it's possible that we still painted a few tiles and that it wasn't skipped at all.  Could you just return a boolean from prepareToUpdateTiles about whether painting occurred or not, rather than setting a flag?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:477
> +    IntRect idlePaintContentRect = contentRect;
> +    idlePaintContentRect.inflate(idlePaintPaddingWidth);
> +    idlePaintContentRect.intersect(IntRect(IntPoint::zero(), contentBounds()));

If you're going to duplicate this logic, it should probably be its own function.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.