[chromium] Avoid pushing dirty tiles to the impl layer
Created attachment 115889 [details] Patch
Comment on attachment 115889 [details] Patch I <3 the tests. Enne probably wants to look through the meat of the change, its a little beyond my comfort zone.
BTW the meat of the change is this in TiledLayerChromium::pushPropertiesTo: 248 if (tile->isDirty()) 249 continue; The rest is test harness hookup
Comment on attachment 115889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115889&action=review Nice test. Making LayerChromium more testable is fantastic. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:160 > +void TiledLayerChromium::createTiler(CCLayerTilingData::BorderTexelOption borderTexelOption) > +{ > + m_tiler = CCLayerTilingData::create(IntSize(defaultTileSize, defaultTileSize), borderTexelOption); > } It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size. Maybe this function should take both tile size and texel option? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:64 > + // Exposed to subclasses for testing. > + void setTileSize(const IntSize&); Maybe not just for testing. This'd probably be useful if we wanted to have image layers not tile unless they were really large, for instance. Just sayin'. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:117 > + // The tile size is 100x100, so this invalidates and then paints three tiles. 3? 100 x 200 pixels = 1 x 2 tiles?
(In reply to comment #4) > (From update of attachment 115889 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115889&action=review > > Nice test. Making LayerChromium more testable is fantastic. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:160 > > +void TiledLayerChromium::createTiler(CCLayerTilingData::BorderTexelOption borderTexelOption) > > +{ > > + m_tiler = CCLayerTilingData::create(IntSize(defaultTileSize, defaultTileSize), borderTexelOption); > > } > > It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size. Maybe this function should take both tile size and texel option? Hmmm....currently the tile size is calculated and set much later than setLayerTreeHost() which is where the tiler is created. Could probably defer creating the tiler until we know the tile size since we just invalidate everything when changing the tile size anyway. Would require a bit of careful thought, though, to not get it wrong. How would you feel about doing this later? > > > > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:117 > > + // The tile size is 100x100, so this invalidates and then paints three tiles. > > 3? 100 x 200 pixels = 1 x 2 tiles? Bad comment, when I was fighting border texels I made this test use 3 tiles instead of two. Will fix the comment.
Created attachment 115925 [details] fix comment
Comment on attachment 115925 [details] fix comment Attachment 115925 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10516337
(In reply to comment #5) > (In reply to comment #4) > > It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size. Maybe this function should take both tile size and texel option? > > Hmmm....currently the tile size is calculated and set much later than setLayerTreeHost() which is where the tiler is created. Could probably defer creating the tiler until we know the tile size since we just invalidate everything when changing the tile size anyway. Would require a bit of careful thought, though, to not get it wrong. How would you feel about doing this later? If it's not too complicated, deferring the creation of the tiler until the tile size is known seems like it'd make this cleaner.
Created attachment 116132 [details] Defer tiler creation until prepareToUpdate
(In reply to comment #8) > (In reply to comment #5) > > (In reply to comment #4) > > > > It's kind of awkward that createTiler and setTileSize have to be called in a particular order and that calling createTiler will un-set your tile size. Maybe this function should take both tile size and texel option? > > > > Hmmm....currently the tile size is calculated and set much later than setLayerTreeHost() which is where the tiler is created. Could probably defer creating the tiler until we know the tile size since we just invalidate everything when changing the tile size anyway. Would require a bit of careful thought, though, to not get it wrong. How would you feel about doing this later? > > If it's not too complicated, deferring the creation of the tiler until the tile size is known seems like it'd make this cleaner. This patch defers until prepareToUpdate() which seems like the first place where the tiler really needs to be there. If tile size or tiling option are changed before the first prepareToUpdate() call then we just store the new values to use then. If we have a tiler and the tile size / tiling option change, then the existing tiler is thrown away and a new one created in prepareToUpdate(). I think it is cleaner this way.
Comment on attachment 116132 [details] Defer tiler creation until prepareToUpdate Looks good to me.
Comment on attachment 116132 [details] Defer tiler creation until prepareToUpdate Sounds good. Big thumbs up on the test. rs=me
Committed r100958: <http://trac.webkit.org/changeset/100958>