Summary: | Make TileController manipulate PlatformCALayers instead of CALayers | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Tim Horton <thorton> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, cmarcelo, commit-queue, eric.carlson, glenn, jamesr, jer.noble, luiz, simon.fraser, tonikitoo | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Tim Horton
2013-10-24 11:05:53 PDT
Created attachment 215125 [details]
preliminary patch
Comment on attachment 215125 [details] preliminary patch Attachment 215125 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/11678029 Created attachment 215133 [details]
patch
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 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. Comment on attachment 215133 [details] patch Attachment 215133 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/10408242 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'
Created attachment 215208 [details]
windows, please.
Comment on attachment 215208 [details] windows, please. Attachment 215208 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/13068012 Now WebKit is failing on Windows, but at least we got past WebCore! Will fix. Created attachment 215218 [details]
windows again
Created attachment 215219 [details]
somehow uploaded old patch
Created attachment 215221 [details]
missed one fixme (removing stale repaint counts)
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. (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()? (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. |