Bug 133895 - [Win] Use TileController on Windows
Summary: [Win] Use TileController on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 133704
  Show dependency treegraph
 
Reported: 2014-06-13 21:27 PDT by Brent Fulgham
Modified: 2014-08-15 09:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch (16.88 KB, patch)
2014-06-13 21:33 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
WIP #2: Not complete (21.35 KB, patch)
2014-06-14 10:32 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (32.10 KB, patch)
2014-06-16 11:25 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Fix WK2 build error. (33.84 KB, patch)
2014-06-16 12:49 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
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 Details
Revised patch without unrelated loader change. (32.87 KB, patch)
2014-06-16 15:34 PDT, Brent Fulgham
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-06-13 21:27:33 PDT
This patch adds the necessary files to activate Tiled Drawing on Windows.
Comment 1 Brent Fulgham 2014-06-13 21:33:05 PDT
Created attachment 233103 [details]
Patch
Comment 2 Simon Fraser (smfr) 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?
Comment 3 Brent Fulgham 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?
Comment 4 Simon Fraser (smfr) 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).
Comment 5 Brent Fulgham 2014-06-14 10:32:52 PDT
Created attachment 233111 [details]
WIP #2: Not complete
Comment 6 Brent Fulgham 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!
Comment 7 Brent Fulgham 2014-06-16 11:25:33 PDT
Created attachment 233164 [details]
Patch
Comment 8 Brent Fulgham 2014-06-16 11:41:16 PDT
<rdar://problem/17260516>
Comment 9 Brent Fulgham 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)
Comment 10 Tim Horton 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.
Comment 11 Brent Fulgham 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.
Comment 12 Tim Horton 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.
Comment 13 Brent Fulgham 2014-06-16 12:49:44 PDT
Created attachment 233173 [details]
Fix WK2 build error.
Comment 14 Brent Fulgham 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.
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Brent Fulgham 2014-06-16 15:34:12 PDT
Created attachment 233189 [details]
Revised patch without unrelated loader change.
Comment 18 Brent Fulgham 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.
Comment 19 Tim Horton 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.
Comment 20 Brent Fulgham 2014-06-16 16:57:20 PDT
Committed r170037: <http://trac.webkit.org/changeset/170037>