Bug 123279 – Make TileController manipulate PlatformCALayers instead of CALayers

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

[Tim Horton]

As the first of a bunch of steps towards making TileController work with the remote layer tree code, make TileController operate on PlatformCALayers instead of directly on Objective-C CALayers.

It will still explicitly make PlatformCALayerMac instances, and it will still live in mac/, and so it won't yet work with remote layer trees - it's just step one.

This might all help get TileController working on Windows, too.

[Tim Horton]

Created attachment 215125 [details]
preliminary patch

[Build Bot]

Comment on attachment 215125 [details]
preliminary patch

Attachment 215125 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/11678029

[Tim Horton]

Created attachment 215133 [details]
patch

[Tim Horton]

Note that even with this patch, TileController still explicitly makes PlatformCALayer*Mac* instances, but that's why it's "the first of a bunch of steps" :D

[Tim Horton]

Comment on attachment 215133 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215133&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests, just a (largeish) refactor.

Probably needs more description of the overall change here.

[Build Bot]

Comment on attachment 215133 [details]
patch

Attachment 215133 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/10408242

[Darin Adler]

Comment on attachment 215133 [details]
patch

1>..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(179): error C2259: 'WebCore::MediaPlayerPrivateQuickTimeVisualContext::LayerClient' : cannot instantiate abstract class
                 due to following members:
                 'void WebCore::PlatformCALayerClient::platformCALayerPaintContents(WebCore::PlatformCALayer *,WebCore::GraphicsContext &,const WebCore::IntRect &)' : is abstract
                 c:\cygwin\home\buildbot\webkit\source\webcore\platform\graphics\ca\PlatformCALayerClient.h(53) : see declaration of 'WebCore::PlatformCALayerClient::platformCALayerPaintContents'
                 'int WebCore::PlatformCALayerClient::platformCALayerIncrementRepaintCount(WebCore::PlatformCALayer *)' : is abstract
                 c:\cygwin\home\buildbot\webkit\source\webcore\platform\graphics\ca\PlatformCALayerClient.h(56) : see declaration of 'WebCore::PlatformCALayerClient::platformCALayerIncrementRepaintCount'

[Tim Horton]

Created attachment 215208 [details]
windows, please.

[Build Bot]

Comment on attachment 215208 [details]
windows, please.

Attachment 215208 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/13068012

[Tim Horton]

Now WebKit is failing on Windows, but at least we got past WebCore! Will fix.

[Tim Horton]

Created attachment 215218 [details]
windows again

[Tim Horton]

Created attachment 215219 [details]
somehow uploaded old patch

[Tim Horton]

Created attachment 215221 [details]
missed one fixme (removing stale repaint counts)

[Simon Fraser (smfr)]

Comment on attachment 215221 [details]
missed one fixme (removing stale repaint counts)

View in context: https://bugs.webkit.org/attachment.cgi?id=215221&action=review

> Source/WebCore/platform/graphics/ca/mac/TileController.h:142
> +    virtual void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*) OVERRIDE { }
> +    virtual bool platformCALayerRespondsToLayoutChanges() const OVERRIDE { return false; }
> +    virtual void platformCALayerAnimationStarted(CFTimeInterval) OVERRIDE { }
> +    virtual GraphicsLayer::CompositingCoordinatesOrientation platformCALayerContentsOrientation() const OVERRIDE { return GraphicsLayer::CompositingCoordinatesTopDown; }
> +    virtual void platformCALayerPaintContents(PlatformCALayer*, GraphicsContext&, const IntRect&) OVERRIDE;
> +    virtual bool platformCALayerShowDebugBorders() const OVERRIDE;
> +    virtual bool platformCALayerShowRepaintCounter(PlatformCALayer*) const OVERRIDE;
> +    virtual int platformCALayerIncrementRepaintCount(PlatformCALayer*) OVERRIDE;
> +
> +    virtual bool platformCALayerContentsOpaque() const OVERRIDE { return m_tilesAreOpaque; }
> +    virtual bool platformCALayerDrawsContent() const OVERRIDE { return true; }
> +    virtual void platformCALayerLayerDidDisplay(PlatformLayer*) OVERRIDE { }
> +
> +    virtual void platformCALayerSetNeedsToRevalidateTiles() OVERRIDE { }
> +    virtual float platformCALayerDeviceScaleFactor() OVERRIDE;

It's weird that these client functions don't all take a PlatformCALayer*

Not sure why platformCALayerDeviceScaleFactor isnt' const.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:-120
> -    [CATransaction commit];

You should explain why it's OK to remove the transaction.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:193
> +    return m_tileCacheLayer->owner()->platformCALayerDeviceScaleFactor();

These redirections are pretty confusing.

[Tim Horton]

(In reply to comment #14)
> (From update of attachment 215221 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215221&action=review
> 
> > Source/WebCore/platform/graphics/ca/mac/TileController.h:142
> > +    virtual void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*) OVERRIDE { }
> > +    virtual bool platformCALayerRespondsToLayoutChanges() const OVERRIDE { return false; }
> > +    virtual void platformCALayerAnimationStarted(CFTimeInterval) OVERRIDE { }
> > +    virtual GraphicsLayer::CompositingCoordinatesOrientation platformCALayerContentsOrientation() const OVERRIDE { return GraphicsLayer::CompositingCoordinatesTopDown; }
> > +    virtual void platformCALayerPaintContents(PlatformCALayer*, GraphicsContext&, const IntRect&) OVERRIDE;
> > +    virtual bool platformCALayerShowDebugBorders() const OVERRIDE;
> > +    virtual bool platformCALayerShowRepaintCounter(PlatformCALayer*) const OVERRIDE;
> > +    virtual int platformCALayerIncrementRepaintCount(PlatformCALayer*) OVERRIDE;
> > +
> > +    virtual bool platformCALayerContentsOpaque() const OVERRIDE { return m_tilesAreOpaque; }
> > +    virtual bool platformCALayerDrawsContent() const OVERRIDE { return true; }
> > +    virtual void platformCALayerLayerDidDisplay(PlatformLayer*) OVERRIDE { }
> > +
> > +    virtual void platformCALayerSetNeedsToRevalidateTiles() OVERRIDE { }
> > +    virtual float platformCALayerDeviceScaleFactor() OVERRIDE;
> 
> It's weird that these client functions don't all take a PlatformCALayer*

Agreed, but I didn't want to do that here in places that aren't relevant to the patch because of the changes required to Windows.

> Not sure why platformCALayerDeviceScaleFactor isnt' const.

Ditto. I'll add them both to my list of good cleanups.

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:-120
> > -    [CATransaction commit];
> 
> You should explain why it's OK to remove the transaction.

Yikes! I should probably double check that it is :)

> > Source/WebCore/platform/graphics/ca/mac/TileController.mm:193
> > +    return m_tileCacheLayer->owner()->platformCALayerDeviceScaleFactor();
> 
> These redirections are pretty confusing.

Maybe tileCacheClient()?

[Tim Horton]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 215221 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=215221&action=review
> > 
> > > Source/WebCore/platform/graphics/ca/mac/TileController.h:142
> > > +    virtual void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*) OVERRIDE { }
> > > +    virtual bool platformCALayerRespondsToLayoutChanges() const OVERRIDE { return false; }
> > > +    virtual void platformCALayerAnimationStarted(CFTimeInterval) OVERRIDE { }
> > > +    virtual GraphicsLayer::CompositingCoordinatesOrientation platformCALayerContentsOrientation() const OVERRIDE { return GraphicsLayer::CompositingCoordinatesTopDown; }
> > > +    virtual void platformCALayerPaintContents(PlatformCALayer*, GraphicsContext&, const IntRect&) OVERRIDE;
> > > +    virtual bool platformCALayerShowDebugBorders() const OVERRIDE;
> > > +    virtual bool platformCALayerShowRepaintCounter(PlatformCALayer*) const OVERRIDE;
> > > +    virtual int platformCALayerIncrementRepaintCount(PlatformCALayer*) OVERRIDE;
> > > +
> > > +    virtual bool platformCALayerContentsOpaque() const OVERRIDE { return m_tilesAreOpaque; }
> > > +    virtual bool platformCALayerDrawsContent() const OVERRIDE { return true; }
> > > +    virtual void platformCALayerLayerDidDisplay(PlatformLayer*) OVERRIDE { }
> > > +
> > > +    virtual void platformCALayerSetNeedsToRevalidateTiles() OVERRIDE { }
> > > +    virtual float platformCALayerDeviceScaleFactor() OVERRIDE;
> > 
> > It's weird that these client functions don't all take a PlatformCALayer*
> 
> Agreed, but I didn't want to do that here in places that aren't relevant to the patch because of the changes required to Windows.
> 
> > Not sure why platformCALayerDeviceScaleFactor isnt' const.
> 
> Ditto. I'll add them both to my list of good cleanups.
> 
> > > Source/WebCore/platform/graphics/ca/mac/TileController.mm:-120
> > > -    [CATransaction commit];
> > 
> > You should explain why it's OK to remove the transaction.
> 
> Yikes! I should probably double check that it is :)

PlatformCALayerMac fills the implicit animation dictionary with nulls, so it's OK. Pretty sure that's the only thing that transaction was there for.

> > > Source/WebCore/platform/graphics/ca/mac/TileController.mm:193
> > > +    return m_tileCacheLayer->owner()->platformCALayerDeviceScaleFactor();
> > 
> > These redirections are pretty confusing.
> 
> Maybe tileCacheClient()?

Going with (after talking to smfr) owningGraphicsLayer() for clarity, despite it not *strictly* being a GraphicsLayer.

[Tim Horton]

http://trac.webkit.org/changeset/158122