Bug 133895 – [Win] Use TileController on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

This patch adds the necessary files to activate Tiled Drawing on Windows.

[Brent Fulgham]

Created attachment 233103 [details]
Patch

[Simon Fraser (smfr)]

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]

(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)]

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]

Created attachment 233111 [details]
WIP #2: Not complete

[Brent Fulgham]

(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]

Created attachment 233164 [details]
Patch

[Brent Fulgham]

<rdar://problem/17260516>

[Brent Fulgham]

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 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 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]

(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]

Created attachment 233173 [details]
Fix WK2 build error.

[Brent Fulgham]

(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 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]

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]

Created attachment 233189 [details]
Revised patch without unrelated loader change.

[Brent Fulgham]

(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 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]

Committed r170037: <http://trac.webkit.org/changeset/170037>