RESOLVED FIXED 124216
Add a new mode to extend the tile cache beyond the page
https://bugs.webkit.org/show_bug.cgi?id=124216
Summary Add a new mode to extend the tile cache beyond the page
Beth Dakin
Reported 2013-11-12 11:56:44 PST
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.
Attachments
Patch (44.87 KB, patch)
2013-11-12 12:20 PST, Beth Dakin
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (2.80 MB, application/zip)
2013-11-12 13:55 PST, Build Bot
no flags
Patch (21.05 KB, patch)
2013-11-12 16:37 PST, Beth Dakin
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (2.76 MB, application/zip)
2013-11-12 19:31 PST, Build Bot
no flags
Patch (20.85 KB, patch)
2013-11-12 20:52 PST, Beth Dakin
no flags
Patch (20.39 KB, patch)
2013-11-13 11:36 PST, Beth Dakin
no flags
Patch (20.16 KB, patch)
2013-11-15 16:49 PST, Beth Dakin
no flags
Patch (21.16 KB, patch)
2013-11-18 14:13 PST, Beth Dakin
simon.fraser: review-
Patch (20.13 KB, patch)
2013-11-18 14:21 PST, Beth Dakin
simon.fraser: review-
Patch to make smfr happy, allow margins to be any size rather than m_tileSize-sized (22.53 KB, patch)
2013-11-20 18:25 PST, Beth Dakin
simon.fraser: review+
Beth Dakin
Comment 1 2013-11-12 12:20:50 PST
Build Bot
Comment 2 2013-11-12 13:10:17 PST
Build Bot
Comment 3 2013-11-12 13:55:15 PST
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
Comment 4 2013-11-12 13:55:17 PST
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 5 2013-11-12 14:03:51 PST
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
Comment 6 2013-11-12 16:16:54 PST
(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
Comment 7 2013-11-12 16:37:28 PST
Build Bot
Comment 8 2013-11-12 19:31:12 PST
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
Comment 9 2013-11-12 19:31:15 PST
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
Comment 10 2013-11-12 20:52:50 PST
Created attachment 216767 [details] Patch This should fix most of the test failures. Still investigating the problem with headers ad footers.
Tim Horton
Comment 11 2013-11-13 03:37:38 PST
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
Comment 12 2013-11-13 11:20:44 PST
(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
Comment 13 2013-11-13 11:36:36 PST
Tim Horton
Comment 14 2013-11-14 22:58:19 PST
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
Comment 15 2013-11-15 16:47:18 PST
(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
Comment 16 2013-11-15 16:49:30 PST
Tim Horton
Comment 17 2013-11-18 14:01:12 PST
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
Comment 18 2013-11-18 14:09:27 PST
(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
Comment 19 2013-11-18 14:11:59 PST
(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
Comment 20 2013-11-18 14:13:45 PST
Tim Horton
Comment 21 2013-11-18 14:15:15 PST
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
Comment 22 2013-11-18 14:17:01 PST
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
Comment 23 2013-11-18 14:21:17 PST
Simon Fraser (smfr)
Comment 24 2013-11-18 14:23:14 PST
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 25 2013-11-18 14:34:26 PST
Comment on attachment 217234 [details] Patch See previous comments
Beth Dakin
Comment 26 2013-11-18 15:10:16 PST
(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)
Comment 27 2013-11-18 15:22:57 PST
(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
Comment 28 2013-11-20 18:25:37 PST
Created attachment 217511 [details] Patch to make smfr happy, allow margins to be any size rather than m_tileSize-sized
Simon Fraser (smfr)
Comment 29 2013-11-21 11:28:01 PST
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
Comment 30 2013-11-21 12:40:33 PST
(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
Comment 31 2013-11-21 13:56:09 PST
Note You need to log in before you can comment on or make changes to this bug.