[webkit-reviews] review denied: [Bug 124216] Add a new mode to extend the tile cache beyond the page : [Attachment 217232] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 124216: Add a new mode to extend the tile cache beyond the page
https://bugs.webkit.org/show_bug.cgi?id=124216

Attachment 217232: Patch
https://bugs.webkit.org/attachment.cgi?id=217232&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217232&action=review


I think we need to measure memory increases from this code, and probably allow
margins to be < 1 sized.

> Source/WebCore/platform/graphics/TiledBacking.h:92
> +    virtual void setTileMargins(unsigned marginTop, unsigned marginBottom,
unsigned marginLeft, unsigned marginRight) = 0;
> +    virtual bool hasMargins() const = 0;
> +
> +    virtual unsigned topMarginHeight() const = 0;
> +    virtual unsigned bottomMarginHeight() const = 0;
> +    virtual unsigned leftMarginWidth() const = 0;
> +    virtual unsigned rightMarginWidth() const = 0;

It's unclear from these names whether the units here are pixels or tiles?

> Source/WebCore/platform/graphics/ca/mac/TileController.h:234
> +    unsigned m_marginTop;
> +    unsigned m_marginBottom;
> +    unsigned m_marginRight;
> +    unsigned m_marginLeft;

ints pls.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:442
> +    // If our tile coverage is just for slow-scrolling, then we want to
limit the tile coverage to the visible rect, but
> +    // we should include the margin tiles if we're close to an edge.
> +    if (m_tileCoverage & CoverageForSlowScrolling) {

I think we should nuke this mode.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:700
> +		   if (mainFrameBackingIsTiledWithMargin())
> +		       m_rootContentLayer->setMasksToBounds(false);

What does this do for composited elements that should be clipped by the
viewport?