Bug 124216 – Add a new mode to extend the tile cache beyond the page

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

We should add a new mode (off by default) to extend the tile cache beyond the page and keep fixed elements stable during rubber-band.

Patch forthcoming.

[Beth Dakin]

Created attachment 216702 [details]
Patch

[Build Bot]

Comment on attachment 216702 [details]
Patch

Attachment 216702 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/22749598

[Build Bot]

Comment on attachment 216702 [details]
Patch

Attachment 216702 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22898799

New failing tests:
fast/css/sticky/sticky-margins.html
platform/mac-wk2/tiled-drawing/tile-coverage-after-scroll.html
fast/css/sticky/sticky-top-margins.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-html-background.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-body-layer.html
platform/mac-wk2/tiled-drawing/tile-coverage-after-scroll-speculative.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-transformed-html.html
fast/css/sticky/sticky-as-positioning-container.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-opacity-html.html
fast/css/sticky/sticky-top.html
fast/css/sticky/inflow-sticky.html
platform/mac-wk2/tiled-drawing/tiled-drawing-zoom-scrolled.html
fast/css/sticky/replaced-sticky.html
platform/mac-wk2/tiled-drawing/tile-coverage-scroll-to-bottom.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-background-no-image.html
platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-non-propagated-body-background.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-zoomed.html
fast/css/sticky/sticky-writing-mode-horizontal-bt.html
fast/css/sticky/sticky-overflowing.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-positioned.html

[Build Bot]

Created attachment 216719 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Comment on attachment 216702 [details]
Patch

Is it possible to break this patch down into several? Maybe the TileController expansion, adding the pref, then the fixed constraining?

[Beth Dakin]

(In reply to comment #5)
> (From update of attachment 216702 [details])
> Is it possible to break this patch down into several? Maybe the TileController expansion, adding the pref, then the fixed constraining?

Sure. Let's make this bug just about the tile cache. Will attach a patch with just that stuff momentarily.

[Beth Dakin]

Created attachment 216741 [details]
Patch

[Build Bot]

Comment on attachment 216741 [details]
Patch

Attachment 216741 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22369847

New failing tests:
fast/css/sticky/sticky-margins.html
platform/mac-wk2/tiled-drawing/tile-coverage-after-scroll.html
fast/css/sticky/sticky-top-margins.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-html-background.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-body-layer.html
platform/mac-wk2/tiled-drawing/tile-coverage-after-scroll-speculative.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-transformed-html.html
fast/css/sticky/sticky-as-positioning-container.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-opacity-html.html
fast/css/sticky/sticky-top.html
fast/css/sticky/inflow-sticky.html
platform/mac-wk2/tiled-drawing/tiled-drawing-zoom-scrolled.html
fast/css/sticky/replaced-sticky.html
platform/mac-wk2/tiled-drawing/tile-coverage-scroll-to-bottom.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-background-no-image.html
platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-non-propagated-body-background.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-zoomed.html
fast/css/sticky/sticky-writing-mode-horizontal-bt.html
fast/css/sticky/sticky-overflowing.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background.html
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-positioned.html

[Build Bot]

Created attachment 216765 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Beth Dakin]

Created attachment 216767 [details]
Patch

This should fix most of the test failures. Still investigating the problem with headers ad footers.

[Tim Horton]

Comment on attachment 216767 [details]
Patch

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

> Source/WebCore/ChangeLog:30
> +        Donât use the tileSizeForCoverageRect() does not make sense in a world where the 

something wacky with the grammar here

> Source/WebCore/platform/graphics/ca/mac/TileController.h:126
> +    virtual void setTileMargins(unsigned marginTop, unsigned marginBottom , unsigned marginLeft, unsigned marginRight) OVERRIDE;

extra space after marginBottom

> Source/WebCore/platform/graphics/ca/mac/TileController.h:230
> +    // These represent the number of tiles that should be used as a margin on each side.

It seems vaguely odd to me that margin is measured in tiles and not pixels, especially since we have a variable tile size (in some rare cases).

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:397
> +    IntSize boundsSizeIncludingMargin = expandedIntSize(m_tileCacheLayer->bounds().size());
> +    boundsSizeIncludingMargin.setWidth(boundsSizeIncludingMargin.width() + leftMarginWidth() + rightMarginWidth());
> +    boundsSizeIncludingMargin.setHeight(boundsSizeIncludingMargin.height() + topMarginHeight() + bottomMarginHeight());

IntSize boundsSizeIncludingMargin = expandedIntSize(m_tileCacheLayer->bounds().size());
boundsSizeIncludingMargin.expand(leftMarginWidth() + rightMarginWidth(), topMarginHeight() + bottomMarginHeight());

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:401
> +    IntPoint boundsOriginIncludingMargin;
> +    boundsOriginIncludingMargin.setX(-leftMarginWidth());
> +    boundsOriginIncludingMargin.setY(-topMarginHeight());

IntPoint boundsOriginIncludingMargin(-leftMarginWidth(), -topMarginHeight()); ?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:-411
> -    topLeft.setX(std::max(clampedRect.x() / m_tileSize.width(), 0));

What was going on here?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:450
> +        float tileWidth = visibleRect.width();
> +        float tileHeight = visibleRect.height();

I think we should always get these from computeTileSize even if it's OK to assume now, in case we change the logic there. Or maybe from m_tileSize? I'm not sure when that gets set, though.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:469
> +        // We're within one tile from the right edge, so we should make sure we have a rights-margin tile.

rights?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:494
> +    // Don't extend coverage beyond the coverage bounds.

This sentence reads like a pointless tautology now. Or maybe it's just too early in the morning.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:510
> -        FloatSize tileSize = coverageRect.size();
> +        FloatSize tileSize = m_visibleRect.size();

Very surprised we don't have a 2k-or-so max here.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:983
> +    return m_marginTop != 0 || m_marginBottom != 0 || m_marginLeft != 0 || m_marginRight != 0;

I don't think you need any of the != 0s.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2381
> +    boundsIncludingMargin.setX(boundsIncludingMargin.x() - leftMarginWidth);
> +    boundsIncludingMargin.setY(boundsIncludingMargin.y() - topMarginHeight);
> +    boundsIncludingMargin.setWidth(boundsIncludingMargin.width() + leftMarginWidth + (LayoutUnit)tiledBacking->rightMarginWidth());
> +    boundsIncludingMargin.setHeight(boundsIncludingMargin.height() + topMarginHeight + (LayoutUnit)tiledBacking->bottomMarginHeight());

Please use e.g. moveBy() and expand()

> Source/WebCore/rendering/RenderLayerBacking.h:153
> +    bool tiledCacheLayerHasMargin() const;

tiledBackingHasMargin, maybe?

[Beth Dakin]

(In reply to comment #11)
> (From update of attachment 216767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216767&action=review
> 
> > Source/WebCore/ChangeLog:30
> > +        Donât use the tileSizeForCoverageRect() does not make sense in a world where the 
> 
> something wacky with the grammar here
> 

Fixed.

> > Source/WebCore/platform/graphics/ca/mac/TileController.h:126
> > +    virtual void setTileMargins(unsigned marginTop, unsigned marginBottom , unsigned marginLeft, unsigned marginRight) OVERRIDE;
> 
> extra space after marginBottom
> 

Fixed.

> > Source/WebCore/platform/graphics/ca/mac/TileController.h:230
> > +    // These represent the number of tiles that should be used as a margin on each side.
> 
> It seems vaguely odd to me that margin is measured in tiles and not pixels, especially since we have a variable tile size (in some rare cases).
> 

Yeah, we can change this to pixels if you want! That was something that Simon requested a while ago, but I put it off because there were other more pressing livability problems with the patch. Definitely something to consider.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:397
> > +    IntSize boundsSizeIncludingMargin = expandedIntSize(m_tileCacheLayer->bounds().size());
> > +    boundsSizeIncludingMargin.setWidth(boundsSizeIncludingMargin.width() + leftMarginWidth() + rightMarginWidth());
> > +    boundsSizeIncludingMargin.setHeight(boundsSizeIncludingMargin.height() + topMarginHeight() + bottomMarginHeight());
> 
> IntSize boundsSizeIncludingMargin = expandedIntSize(m_tileCacheLayer->bounds().size());
> boundsSizeIncludingMargin.expand(leftMarginWidth() + rightMarginWidth(), topMarginHeight() + bottomMarginHeight());
> 

Will fix.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:401
> > +    IntPoint boundsOriginIncludingMargin;
> > +    boundsOriginIncludingMargin.setX(-leftMarginWidth());
> > +    boundsOriginIncludingMargin.setY(-topMarginHeight());
> 
> IntPoint boundsOriginIncludingMargin(-leftMarginWidth(), -topMarginHeight()); ?
> 

Heh, good call.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:-411
> > -    topLeft.setX(std::max(clampedRect.x() / m_tileSize.width(), 0));
> 
> What was going on here?
> 

TileIndex could never be negative before. Not sure why the max with 0 was ever actually necessary though…I can't imagine we ever would have computed a negative before.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:450
> > +        float tileWidth = visibleRect.width();
> > +        float tileHeight = visibleRect.height();
> 
> I think we should always get these from computeTileSize even if it's OK to assume now, in case we change the logic there. Or maybe from m_tileSize? I'm not sure when that gets set, though.
> 

Yeah, it does feel like a bit of a mess right now, between computeTileSize, m_tileSize, and using visibleRect here. However, I think we might actually need to use visibleRect here? One of the callers of this function is tilesWouldChangeForVisibleRect(), which was added with http://trac.webkit.org/changeset/147058 I think that calling computeTileSize() could return the wrong answer for a slow-scrolling page that calls tilesWouldChangeForVisibleRect(). This would all be much simpler if we got rid of the big tiles, which Simon thinks we should consider soon. We could make all of this code so much simpler.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:469
> > +        // We're within one tile from the right edge, so we should make sure we have a rights-margin tile.
> 
> rights?
> 

Fixed.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:494
> > +    // Don't extend coverage beyond the coverage bounds.
> 
> This sentence reads like a pointless tautology now. Or maybe it's just too early in the morning.
> 

Haha, no you're right. Will remove.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:510
> > -        FloatSize tileSize = coverageRect.size();
> > +        FloatSize tileSize = m_visibleRect.size();
> 
> Very surprised we don't have a 2k-or-so max here.
> 

Yeah, big tiles. We really should re-think those.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:983
> > +    return m_marginTop != 0 || m_marginBottom != 0 || m_marginLeft != 0 || m_marginRight != 0;
> 
> I don't think you need any of the != 0s.
> 

Good call. 

> > Source/WebCore/rendering/RenderLayerBacking.cpp:2381
> > +    boundsIncludingMargin.setX(boundsIncludingMargin.x() - leftMarginWidth);
> > +    boundsIncludingMargin.setY(boundsIncludingMargin.y() - topMarginHeight);
> > +    boundsIncludingMargin.setWidth(boundsIncludingMargin.width() + leftMarginWidth + (LayoutUnit)tiledBacking->rightMarginWidth());
> > +    boundsIncludingMargin.setHeight(boundsIncludingMargin.height() + topMarginHeight + (LayoutUnit)tiledBacking->bottomMarginHeight());
> 
> Please use e.g. moveBy() and expand()
> 

Fixed.

> > Source/WebCore/rendering/RenderLayerBacking.h:153
> > +    bool tiledCacheLayerHasMargin() const;
> 
> tiledBackingHasMargin, maybe?

Yes, that's better.

Thanks, Tim! New patch coming soon.

[Beth Dakin]

Created attachment 216826 [details]
Patch

[Tim Horton]

Comment on attachment 216826 [details]
Patch

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

> Source/WebCore/ChangeLog:30
> +        Don't use the tileSizeForCoverageRect() does not make sense in a world where the 

This didn't get fixed from my previous review.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2863
>          // Need to clip to prevent transformed content showing outside this frame

Will this not be a problem still in this new mode? Should we adjust the bounds instead of turning off masksToBounds?

[Beth Dakin]

(In reply to comment #14)
> (From update of attachment 216826 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216826&action=review
> 
> > Source/WebCore/ChangeLog:30
> > +        Don't use the tileSizeForCoverageRect() does not make sense in a world where the 
> 
> This didn't get fixed from my previous review.
> 

Oops, actually fixed this time.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2863
> >          // Need to clip to prevent transformed content showing outside this frame
> 
> Will this not be a problem still in this new mode? Should we adjust the bounds instead of turning off masksToBounds?

Oh yeah, I think you are right and this one should still be allowed to mask. I did consider changing the bounds, but ultimately that seemed like a much more intrusive change.

[Beth Dakin]

Created attachment 217101 [details]
Patch

[Tim Horton]

Comment on attachment 217101 [details]
Patch

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

> Source/WebCore/platform/graphics/TiledBacking.h:86
> +    virtual void setTileMargins(unsigned marginTop, unsigned marginBottom , unsigned marginLeft, unsigned marginRight) = 0;

extra space before a comma

> Source/WebCore/platform/graphics/TiledBacking.h:93
> +    virtual unsigned topMarginHeight() const = 0;
> +    virtual unsigned bottomMarginHeight() const = 0;
> +    virtual unsigned leftMarginWidth() const = 0;
> +    virtual unsigned rightMarginWidth() const = 0;
> +

smfr said something about Edges before. don't know if it's a thing (and probably shouldn't be in this patch if it's not) but it would be nice to have, especially if it can do math with our rect types, etc.

Of course, that would depend on this measuring pixels not tiles.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:440
> +    // If our tile coverage is just for slow-scrolling, then we want to limit the tile coverage the the visible rect, but

the the

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:488
> +    coverageVerticalSize += (topMarginHeight() + bottomMarginHeight());
> +    coverageHorizontalSize += (leftMarginWidth() + rightMarginWidth());

no enclosing parens

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:968
> +void TileController::setTileMargins(unsigned marginTop, unsigned marginBottom , unsigned marginLeft, unsigned marginRight)

another extra space before a comma

[Beth Dakin]

(In reply to comment #17)
> (From update of attachment 217101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217101&action=review
> 
> > Source/WebCore/platform/graphics/TiledBacking.h:86
> > +    virtual void setTileMargins(unsigned marginTop, unsigned marginBottom , unsigned marginLeft, unsigned marginRight) = 0;
> 
> extra space before a comma
> 

Fixed.

> > Source/WebCore/platform/graphics/TiledBacking.h:93
> > +    virtual unsigned topMarginHeight() const = 0;
> > +    virtual unsigned bottomMarginHeight() const = 0;
> > +    virtual unsigned leftMarginWidth() const = 0;
> > +    virtual unsigned rightMarginWidth() const = 0;
> > +
> 
> smfr said something about Edges before. don't know if it's a thing (and probably shouldn't be in this patch if it's not) but it would be nice to have, especially if it can do math with our rect types, etc.
> 
> Of course, that would depend on this measuring pixels not tiles.
> 

I can't seem to find what smfr is referring to. There is no file in the WebCore Xcode project that contains "edge" in its title. I global search within the project for "class Edge" did not return any results. I also did a global search for "edge," which returns tons of results as you may expect. I scanned through the tons, and didn't see anything that looked like a genetical edge class or struct.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:440
> > +    // If our tile coverage is just for slow-scrolling, then we want to limit the tile coverage the the visible rect, but
> 
> the the
> 

Fixed.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:488
> > +    coverageVerticalSize += (topMarginHeight() + bottomMarginHeight());
> > +    coverageHorizontalSize += (leftMarginWidth() + rightMarginWidth());
> 
> no enclosing parens
> 

Fixed.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:968
> > +void TileController::setTileMargins(unsigned marginTop, unsigned marginBottom , unsigned marginLeft, unsigned marginRight)
> 
> another extra space before a comma

Fixed.

[Beth Dakin]

(In reply to comment #18)
> (In reply to comment #17)
> I can't seem to find what smfr is referring to. There is no file in the WebCore Xcode project that contains "edge" in its title. I global search within the project for "class Edge" did not return any results. I also did a global search for "edge," which returns tons of results as you may expect. I scanned through the tons, and didn't see anything that looked like a genetical edge class or struct.
> 

Ugh, typos. Should say GENERIC edge class. Not genetical. There are other typos, of course, but that one seemed in most need of clarity. :-P

[Beth Dakin]

Created attachment 217232 [details]
Patch

[Tim Horton]

Comment on attachment 217232 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:141
> +            tiledBacking->setTileMargins(1,1,1,1);

spaces after commas

[WebKit Commit Bot]

Attachment 217232 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/platform/graphics/TiledBacking.h', u'Source/WebCore/platform/graphics/ca/mac/TileController.h', u'Source/WebCore/platform/graphics/ca/mac/TileController.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', u'Source/WebCore/rendering/RenderView.cpp']" exit_code: 1
Source/WebCore/rendering/RenderLayerBacking.cpp:141:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 217234 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 217232 [details]
Patch

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?

[Simon Fraser (smfr)]

Comment on attachment 217234 [details]
Patch

See previous comments

[Beth Dakin]

(In reply to comment #24)
> (From update of attachment 217232 [details])
> 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.
> 

Yes, I plan to change the tiles to be pixel sized in a follow-up patch. If you think that needs to be done in the initial implementation, then let me know.

> > 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?
> 

I will think of a more accurate name.

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

We talked about this on IRC, and you agreed that they should be unsigned as long as they represent #s of tiles.

> > 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.
> 

Me too. But not in tis patch.

> > 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?

Right now those elements will not end up getting clipped. Personally I think it will be interesting to live on it like this, and see how that feels and if we encounter obviously bad problems. Simple test cases that show things not being clipped tend to look fine. But if there are sites that have surprising/ugly elements hanging out right outside the viewport, then that is obviously a problem.

[Simon Fraser (smfr)]

(In reply to comment #26)
> Right now those elements will not end up getting clipped. Personally I think it will be interesting to live on it like this, and see how that feels and if we encounter obviously bad problems. Simple test cases that show things not being clipped tend to look fine. But if there are sites that have surprising/ugly elements hanging out right outside the viewport, then that is obviously a problem.

I do think we have to clip. There are many sites that "hide" things just outside the viewport.

Also, it would interfere with our "don't make compositing layers for fixed things outside the viewport" code.

[Beth Dakin]

Created attachment 217511 [details]
Patch to make smfr happy, allow margins to be any size rather than m_tileSize-sized

[Simon Fraser (smfr)]

Comment on attachment 217511 [details]
Patch to make smfr happy, allow margins to be any size rather than m_tileSize-sized

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

> Source/WebCore/platform/graphics/ca/mac/TileController.h:238
> +    int m_marginRight;
> +    int m_marginLeft;

left, right

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:444
> +    // These rect computations assume m_tileSize is the correct size to use. However, a tile in the margin area
> +    // might be a different size depending on the size of the margins. So adjustRectAtTileIndexForMargin() will
> +    // fix the rect we've computed to match the margin sizes if this tile is in the margins.
> +    adjustRectAtTileIndexForMargin(tileIndex, rect);

Can't we just intersect with the scaledBounds inflated by the margin?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:460
> +    if (clampedRect.x() >= 0)

clampedRect.y()?

This bug suggests that there aren't any tests. Can we make some?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:514
> +    // 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) {
> +        FloatSize coverageSize = visibleRect.size();
> +        FloatPoint coverageOrigin = visibleRect.location();
> +        float tileWidth = visibleRect.width();
> +        float tileHeight = visibleRect.height();
> +
> +        // We're within one tile from the top, so we should make sure we have a top-margin tile.
> +        if (visibleRect.y() < tileHeight) {
> +            coverageSize.setHeight(coverageSize.height() + topMarginHeight());
> +            coverageOrigin.setY(coverageOrigin.y() - topMarginHeight());
> +        }
> +
> +        // We're within one tile from the left edge, so we should make sure we have a left-margin tile.
> +        if (visibleRect.x() < tileWidth) {
> +            coverageSize.setWidth(coverageSize.width() + leftMarginWidth());
> +            coverageOrigin.setX(coverageOrigin.x() - leftMarginWidth());
> +        }
> +
> +        IntSize layerSize = expandedIntSize(m_tileCacheLayer->bounds().size());
> +        // We're within one tile from the bottom edge, so we should make sure we have a bottom-margin tile.
> +        if (visibleRect.y() + tileHeight > layerSize.height() - tileHeight)
> +            coverageSize.setHeight(coverageSize.height() + bottomMarginHeight());
> +
> +        // We're within one tile from the right edge, so we should make sure we have a right-margin tile.
> +        if (visibleRect.x() + tileWidth > layerSize.width() - tileWidth)
> +            coverageSize.setWidth(coverageSize.width() + rightMarginWidth());
> +
> +        return FloatRect(coverageOrigin, coverageSize);
> +    }
> +

Nuuuuuke it!

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

We need to track the issue of not clipping compositing layers in a bug.

[Beth Dakin]

(In reply to comment #29)
> (From update of attachment 217511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217511&action=review
> 
> > Source/WebCore/platform/graphics/ca/mac/TileController.h:238
> > +    int m_marginRight;
> > +    int m_marginLeft;
> 
> left, right
> 

Will fix.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:444
> > +    // These rect computations assume m_tileSize is the correct size to use. However, a tile in the margin area
> > +    // might be a different size depending on the size of the margins. So adjustRectAtTileIndexForMargin() will
> > +    // fix the rect we've computed to match the margin sizes if this tile is in the margins.
> > +    adjustRectAtTileIndexForMargin(tileIndex, rect);
> 
> Can't we just intersect with the scaledBounds inflated by the margin?
> 

We can't. The rect is calculate like this:

IntRect rect(tileIndex.x() * m_tileSize.width(), tileIndex.y() * m_tileSize.height(), m_tileSize.width(), m_tileSize.height());

and if the margins are not equivalent to m_tileSize, then the these calculations come out wrong.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:460
> > +    if (clampedRect.x() >= 0)
> 
> clampedRect.y()?
> 

Good catch!!

> This bug suggests that there aren't any tests. Can we make some?
> 

Let me think about that. I will file a follow-up bug to add tests. I think we need a way to enable this mode first (which I have a patch for) then after that we can add tests.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:514
> > +    // 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) {
> > +        FloatSize coverageSize = visibleRect.size();
> > +        FloatPoint coverageOrigin = visibleRect.location();
> > +        float tileWidth = visibleRect.width();
> > +        float tileHeight = visibleRect.height();
> > +
> > +        // We're within one tile from the top, so we should make sure we have a top-margin tile.
> > +        if (visibleRect.y() < tileHeight) {
> > +            coverageSize.setHeight(coverageSize.height() + topMarginHeight());
> > +            coverageOrigin.setY(coverageOrigin.y() - topMarginHeight());
> > +        }
> > +
> > +        // We're within one tile from the left edge, so we should make sure we have a left-margin tile.
> > +        if (visibleRect.x() < tileWidth) {
> > +            coverageSize.setWidth(coverageSize.width() + leftMarginWidth());
> > +            coverageOrigin.setX(coverageOrigin.x() - leftMarginWidth());
> > +        }
> > +
> > +        IntSize layerSize = expandedIntSize(m_tileCacheLayer->bounds().size());
> > +        // We're within one tile from the bottom edge, so we should make sure we have a bottom-margin tile.
> > +        if (visibleRect.y() + tileHeight > layerSize.height() - tileHeight)
> > +            coverageSize.setHeight(coverageSize.height() + bottomMarginHeight());
> > +
> > +        // We're within one tile from the right edge, so we should make sure we have a right-margin tile.
> > +        if (visibleRect.x() + tileWidth > layerSize.width() - tileWidth)
> > +            coverageSize.setWidth(coverageSize.width() + rightMarginWidth());
> > +
> > +        return FloatRect(coverageOrigin, coverageSize);
> > +    }
> > +
> 
> Nuuuuuke it!
> 

Haha, all in good time!

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:700
> > +                if (mainFrameBackingIsTiledWithMargin())
> > +                    m_rootContentLayer->setMasksToBounds(false);
> 
> We need to track the issue of not clipping compositing layers in a bug.

Yes, I have a bunch of bugs to file. This will definitely be one of them.

[Beth Dakin]

Committed change: http://trac.webkit.org/changeset/159645