This setting will be used for debugging and evaluating the overhead which may be incurred due to a large tile size.
Created attachment 262808 [details] Patch
Comment on attachment 262808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262808&action=review > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:307 > +std::unique_ptr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerFactory* factory, GraphicsLayerClient& client, Type layerType, const IntSize& tileSize) It's pretty weird that we're passing tileSize in GraphicsLayer::create for every layer when most layers aren't tiled. Can't we just make a static class method on TiledBacking and change the defaults there? It would make this patch a lot smaller.
Created attachment 262816 [details] Patch
Created attachment 262821 [details] Patch
Comment on attachment 262821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262821&action=review > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:317 > +WK_EXPORT void WKPreferencesSetSingleLargeTile(WKPreferencesRef, bool); This name makes it seem like the function should be passed a single large tile. Can you change it to something like WKPreferencesSetUsesSingleLargeTile()? This should probably be changed in the underlying WebPreferences implementation as well. > Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:318 > +WK_EXPORT bool WKPreferencesGetSingleLargeTile(WKPreferencesRef); Same comment here. WKPreferencesGetUsesSingleLargeTile()?
Created attachment 262875 [details] Patch
Comment on attachment 262821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262821&action=review >> Source/WebKit2/UIProcess/API/C/WKPreferencesRefPrivate.h:318 >> +WK_EXPORT bool WKPreferencesGetSingleLargeTile(WKPreferencesRef); > > Same comment here. WKPreferencesGetUsesSingleLargeTile()? Done, the name was changed.
Comment on attachment 262875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262875&action=review > Source/WebCore/page/Settings.in:204 > +usesSingleLargeTile initial=false "use single" is a misnomer. It may still create multiple tiles. I'd call it useGiantTiles or something. > Source/WebCore/platform/graphics/TiledBacking.h:31 > +inline static IntSize defaultTileSize(bool usesSingleLargeTile = false) usesSingleLargeTile -> useGiantTiles > Source/WebCore/platform/graphics/TiledBacking.h:35 > + // The width and height of a single tile in a tiled layer. Should be large enough to > + // avoid lots of small tiles (and therefore lots of drawing callbacks), but small enough > + // to keep the overall tile cost low. I don't think this comment adds anything. > Source/WebCore/platform/graphics/TiledBacking.h:38 > + // This is an experimental value for Tdebugging and evaluating the overhead which may be Tdebugging
Comment on attachment 262875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262875&action=review > Source/WebCore/platform/graphics/ca/PlatformCALayer.h:118 > + void setTileSize(const IntSize& tileSize) { m_tileSize = tileSize; } I still think we should avoid the getter/setter entirely. Add something to PlatformCALayerClient that returns the state of the *setting* (the boolean state), and make the decision by calling that. Just a thought! > Source/WebKit/mac/ChangeLog:1 > +2015-10-11 Said Abou-Hallawa <sabouhallawa@apple.com> There is no need for WebKit1 SPI for this. Please omit it.
Created attachment 262938 [details] Patch
Created attachment 262939 [details] Patch
Comment on attachment 262875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262875&action=review >> Source/WebCore/page/Settings.in:204 >> +usesSingleLargeTile initial=false > > "use single" is a misnomer. It may still create multiple tiles. I'd call it useGiantTiles or something. Done. The name was changed to useGiantTiles. >> Source/WebCore/platform/graphics/TiledBacking.h:31 >> +inline static IntSize defaultTileSize(bool usesSingleLargeTile = false) > > usesSingleLargeTile -> useGiantTiles Done. The argument was changed to useGiantTiles. >> Source/WebCore/platform/graphics/TiledBacking.h:35 >> + // to keep the overall tile cost low. > > I don't think this comment adds anything. Done. The comment was deleted. >> Source/WebCore/platform/graphics/TiledBacking.h:38 >> + // This is an experimental value for Tdebugging and evaluating the overhead which may be > > Tdebugging Done. The spelling mistake was fixed. >> Source/WebCore/platform/graphics/ca/PlatformCALayer.h:118 >> + void setTileSize(const IntSize& tileSize) { m_tileSize = tileSize; } > > I still think we should avoid the getter/setter entirely. Add something to PlatformCALayerClient that returns the state of the *setting* (the boolean state), and make the decision by calling that. Just a thought! Done. Since GraphicsLayerCA is derived from GraphicsLayer and PlatformCALayerClient, the setter is defined as the virtual function GraphicsLayer::setTileSize and the getter is defined as the virtual function PlatformCALayerClient::platformCALayerTileSize(). GraphicsLayerCA overrides both. The RenderLayerBacking::createGraphicsLayer() will call the setter. And TileController::tileSize() will call the getter. >> Source/WebKit/mac/ChangeLog:1 >> +2015-10-11 Said Abou-Hallawa <sabouhallawa@apple.com> > > There is no need for WebKit1 SPI for this. Please omit it. Done. WebKit SPI was removed.
Created attachment 262951 [details] Patch
Comment on attachment 262951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262951&action=review > Source/WebCore/platform/graphics/GraphicsLayerClient.h:29 > +#include "FloatRect.h" Do you really need the FloatRect.h? > Source/WebCore/platform/graphics/TiledBacking.h:31 > +inline static IntSize defaultTileSize(bool useGiantTiles = false) Could you use enum class instead? -this could also be a static TiledBacking::defaultTileSize() instead of this standalone function, but I don't really have a strong preference.
Created attachment 262989 [details] Patch
Comment on attachment 262951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262951&action=review >> Source/WebCore/platform/graphics/GraphicsLayerClient.h:29 >> +#include "FloatRect.h" > > Do you really need the FloatRect.h? TiledBacking.h was missing forward declaration for FloatPoint and FloatRect. I added them in there and deleted the #include "FloatRect.h" from GraphicsLayerClient.h. >> Source/WebCore/platform/graphics/TiledBacking.h:31 >> +inline static IntSize defaultTileSize(bool useGiantTiles = false) > > Could you use enum class instead? -this could also be a static TiledBacking::defaultTileSize() instead of this standalone function, but I don't really have a strong preference. An enum called TileSizeMode is added and used as the parameter to defaultTileSize() instead of passing the bool.
Comment on attachment 262989 [details] Patch Rejecting attachment 262989 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 262989, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/279980
Created attachment 262991 [details] Patch
Comment on attachment 262991 [details] Patch Clearing flags on attachment: 262991 Committed r190998: <http://trac.webkit.org/changeset/190998>
All reviewed patches have been landed. Closing bug.
<rdar://problem/23017093>