RESOLVED FIXED 133895
[Win] Use TileController on Windows
https://bugs.webkit.org/show_bug.cgi?id=133895
Summary [Win] Use TileController on Windows
Brent Fulgham
Reported 2014-06-13 21:27:33 PDT
This patch adds the necessary files to activate Tiled Drawing on Windows.
Attachments
Patch (16.88 KB, patch)
2014-06-13 21:33 PDT, Brent Fulgham
no flags
WIP #2: Not complete (21.35 KB, patch)
2014-06-14 10:32 PDT, Brent Fulgham
no flags
Patch (32.10 KB, patch)
2014-06-16 11:25 PDT, Brent Fulgham
no flags
Fix WK2 build error. (33.84 KB, patch)
2014-06-16 12:49 PDT, Brent Fulgham
no flags
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (577.49 KB, application/zip)
2014-06-16 15:25 PDT, Build Bot
no flags
Revised patch without unrelated loader change. (32.87 KB, patch)
2014-06-16 15:34 PDT, Brent Fulgham
thorton: review+
Brent Fulgham
Comment 1 2014-06-13 21:33:05 PDT
Simon Fraser (smfr)
Comment 2 2014-06-13 22:23:46 PDT
Comment on attachment 233103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233103&action=review > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:101 > + // FIXME: We should actually collect rects to use instead of defaulting to Window's > + // normal drawing path. > + PlatformCALayer::RepaintRectList dirtyRects; > + return dirtyRects; Doesn't this actually need to add a single rect to the list that's the size of the context's clip bounds?
Brent Fulgham
Comment 3 2014-06-14 09:00:08 PDT
(In reply to comment #2) > (From update of attachment 233103 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233103&action=review > > > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:101 > > + // FIXME: We should actually collect rects to use instead of defaulting to Window's > > + // normal drawing path. > > + PlatformCALayer::RepaintRectList dirtyRects; > > + return dirtyRects; > > Doesn't this actually need to add a single rect to the list that's the size of the context's clip bounds? This function is kind of a no-op at the moment. Take a look at the implementation of drawLayerContents (right below it) -- it doesn't use the argument for anything. All of this code builds and runs, but I'm not sure if its actually using any of this new behavior. How can I best test that the Tiled Drawing stuff is actually doing what we expect here?
Simon Fraser (smfr)
Comment 4 2014-06-14 10:12:46 PDT
Well, the web inspector layer list should show lower memory use (assuming the old code actually returned a correct value for its eagerly created tiles). You could also put a breakpoint in TIleController::revalidate tiles and check that the visibleRect stuff looks right (on a test case with a single huge layer).
Brent Fulgham
Comment 5 2014-06-14 10:32:52 PDT
Created attachment 233111 [details] WIP #2: Not complete
Brent Fulgham
Comment 6 2014-06-14 10:57:50 PDT
(In reply to comment #4) > Well, the web inspector layer list should show lower memory use (assuming the old code actually returned a correct value for its eagerly created tiles). > > You could also put a breakpoint in TIleController::revalidate tiles and check that the visibleRect stuff looks right (on a test case with a single huge layer). I realized we have tests to do this (LayoutTests/compositing/tiling/tiled-layer-resize.html). I've still got some work to do on this -- but I'm getting close!
Brent Fulgham
Comment 7 2014-06-16 11:25:33 PDT
Brent Fulgham
Comment 8 2014-06-16 11:41:16 PDT
Brent Fulgham
Comment 9 2014-06-16 11:45:51 PDT
Comment on attachment 233164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233164&action=review > Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:40 > +const float PlatformCALayer::webLayerWastedSpaceThreshold = 0.75f; Needed to move this here due to Windows compiler limitations. > Source/WebCore/platform/graphics/ca/PlatformCALayer.h:-231 > - constexpr static const float webLayerWastedSpaceThreshold = 0.75f; Windows doesn't understand this construct. > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:55 > + || m_owner->layerType() == PlatformCALayer::LayerTypeTiledBackingLayer) { Do we need all of these? Should I just do !PlatformCALayer::LayerTypeWebLayer? (Ditto for all the other methods below)
Tim Horton
Comment 10 2014-06-16 11:49:06 PDT
Comment on attachment 233164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233164&action=review > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:99 > + // FIXME: We should actually collect rects to use instead of defaulting to Window's Window’s? >> Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:55 >> + || m_owner->layerType() == PlatformCALayer::LayerTypeTiledBackingLayer) { > > Do we need all of these? Should I just do !PlatformCALayer::LayerTypeWebLayer? (Ditto for all the other methods below) Maybe a “isTiledSomethingLayer” helper? You don’t want !LayerTypeWebLayer because there are a whole bunch of other layer types.
Brent Fulgham
Comment 11 2014-06-16 11:57:23 PDT
Comment on attachment 233164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233164&action=review >> Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:99 >> + // FIXME: We should actually collect rects to use instead of defaulting to Window's > > Window’s? Windows'? > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.h:40 > + virtual bool usesTiledBackingLayer() const override { return layerType() == LayerTypePageTiledBackingLayer || layerType() == LayerTypeTiledBackingLayer; } This is now identical to the Cocoa version. Can these be shared somehow? > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.h:53 > + virtual const PlatformCALayerList* customSublayers() const override { return m_customSublayers.get(); } Ditto.
Tim Horton
Comment 12 2014-06-16 12:05:44 PDT
(In reply to comment #11) > (From update of attachment 233164 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233164&action=review > > >> Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:99 > >> + // FIXME: We should actually collect rects to use instead of defaulting to Window's > > > > Window’s? > > Windows’? Yes > > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.h:40 > > + virtual bool usesTiledBackingLayer() const override { return layerType() == LayerTypePageTiledBackingLayer || layerType() == LayerTypeTiledBackingLayer; } > > This is now identical to the Cocoa version. Can these be shared somehow? > > > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.h:53 > > + virtual const PlatformCALayerList* customSublayers() const override { return m_customSublayers.get(); } > > Ditto. Are they also the same as the *Remote ones? If so, sure, scoot it into PlatformCALayer itself and devirtualize.
Brent Fulgham
Comment 13 2014-06-16 12:49:44 PDT
Created attachment 233173 [details] Fix WK2 build error.
Brent Fulgham
Comment 14 2014-06-16 14:38:42 PDT
(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 233164 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=233164&action=review > > > > >> Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:99 > > >> + // FIXME: We should actually collect rects to use instead of defaulting to Window's > > > > > > Window’s? > > > > Windows’? > > Yes > > > > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.h:40 > > > + virtual bool usesTiledBackingLayer() const override { return layerType() == LayerTypePageTiledBackingLayer || layerType() == LayerTypeTiledBackingLayer; } > > > > This is now identical to the Cocoa version. Can these be shared somehow? > > > > > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.h:53 > > > + virtual const PlatformCALayerList* customSublayers() const override { return m_customSublayers.get(); } > > > > Ditto. > > Are they also the same as the *Remote ones? If so, sure, scoot it into PlatformCALayer itself and devirtualize. It looks like usesTileddBackingLayer() is, but not the customSublayers. I'll just modify the one case.
Build Bot
Comment 15 2014-06-16 15:25:02 PDT
Comment on attachment 233173 [details] Fix WK2 build error. Attachment 233173 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5922332803596288 New failing tests: http/tests/security/script-no-crossorigin-onerror-should-be-sanitized.html http/tests/security/canvas-remote-read-remote-image-redirect.html http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html http/tests/security/canvas-remote-read-redirect-to-remote-image.html http/tests/eventsource/eventsource-cors-with-credentials.html http/tests/security/script-with-failed-cors-check-fails-to-load.html http/tests/security/cross-origin-script-window-onerror.html http/tests/canvas/philip/tests/security.pattern.cross.html http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html http/tests/canvas/philip/tests/security.pattern.image.strokeStyle.html http/tests/canvas/philip/tests/security.reset.html http/tests/canvas/philip/tests/security.drawImage.image.html http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html http/tests/loading/cross-origin-XHR-willLoadRequest.html http/tests/canvas/webgl/origin-clean-conformance.html http/tests/security/text-track-crossorigin.html http/tests/security/video-poster-cross-origin-crash.html http/tests/security/shape-image-cors.html http/tests/security/img-with-failed-cors-check-fails-to-load.html http/tests/eventsource/eventsource-cors-basic.html http/tests/canvas/philip/tests/security.pattern.image.fillStyle.html http/tests/security/video-poster-cross-origin-crash2.html http/tests/canvas/philip/tests/security.drawImage.canvas.html http/tests/xmlhttprequest/access-control-and-redirects-async.html http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html http/tests/security/cross-origin-script-window-onerror-redirected.html http/tests/xmlhttprequest/access-control-and-redirects.html http/tests/security/canvas-remote-read-remote-image.html http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html
Build Bot
Comment 16 2014-06-16 15:25:08 PDT
Created attachment 233188 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Brent Fulgham
Comment 17 2014-06-16 15:34:12 PDT
Created attachment 233189 [details] Revised patch without unrelated loader change.
Brent Fulgham
Comment 18 2014-06-16 15:36:02 PDT
(In reply to comment #16) > Created an attachment (id=233188) [details] > Archive of layout-test-results from webkit-ews-07 for mac-mountainlion > > The attached test failures were seen while running run-webkit-tests on the mac-ews. > Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5 I introduced this failure by including some debugging code in the patch I uploaded. Fixed.
Tim Horton
Comment 19 2014-06-16 16:19:55 PDT
Comment on attachment 233189 [details] Revised patch without unrelated loader change. View in context: https://bugs.webkit.org/attachment.cgi?id=233189&action=review > Source/WebCore/platform/graphics/TiledBacking.h:35 > -#if PLATFORM(COCOA) > +#if USE(CA) really no need for this #if guarding a forward declaration at all.
Brent Fulgham
Comment 20 2014-06-16 16:57:20 PDT
Note You need to log in before you can comment on or make changes to this bug.