WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
123279
Make TileController manipulate PlatformCALayers instead of CALayers
https://bugs.webkit.org/show_bug.cgi?id=123279
Summary
Make TileController manipulate PlatformCALayers instead of CALayers
Tim Horton
Reported
2013-10-24 11:05:53 PDT
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.
Attachments
preliminary patch
(77.82 KB, patch)
2013-10-24 17:48 PDT
,
Tim Horton
buildbot
: commit-queue-
Details
Formatted Diff
Diff
patch
(91.21 KB, patch)
2013-10-24 18:43 PDT
,
Tim Horton
buildbot
: commit-queue-
Details
Formatted Diff
Diff
windows, please.
(94.95 KB, patch)
2013-10-25 13:47 PDT
,
Tim Horton
buildbot
: commit-queue-
Details
Formatted Diff
Diff
windows again
(94.95 KB, patch)
2013-10-25 14:56 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
somehow uploaded old patch
(96.94 KB, patch)
2013-10-25 15:00 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
missed one fixme (removing stale repaint counts)
(98.90 KB, patch)
2013-10-25 15:19 PDT
,
Tim Horton
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2013-10-24 17:48:29 PDT
Created
attachment 215125
[details]
preliminary patch
Build Bot
Comment 2
2013-10-24 18:28:24 PDT
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
Comment 3
2013-10-24 18:43:03 PDT
Created
attachment 215133
[details]
patch
Tim Horton
Comment 4
2013-10-24 18:43:38 PDT
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 5
2013-10-24 18:45:53 PDT
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 6
2013-10-24 19:16:30 PDT
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 7
2013-10-25 12:39:49 PDT
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
Comment 8
2013-10-25 13:47:47 PDT
Created
attachment 215208
[details]
windows, please.
Build Bot
Comment 9
2013-10-25 14:46:01 PDT
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
Comment 10
2013-10-25 14:54:52 PDT
Now WebKit is failing on Windows, but at least we got past WebCore! Will fix.
Tim Horton
Comment 11
2013-10-25 14:56:31 PDT
Created
attachment 215218
[details]
windows again
Tim Horton
Comment 12
2013-10-25 15:00:08 PDT
Created
attachment 215219
[details]
somehow uploaded old patch
Tim Horton
Comment 13
2013-10-25 15:19:21 PDT
Created
attachment 215221
[details]
missed one fixme (removing stale repaint counts)
Simon Fraser (smfr)
Comment 14
2013-10-28 11:36:11 PDT
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
Comment 15
2013-10-28 12:04:47 PDT
(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
Comment 16
2013-10-28 12:31:01 PDT
(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
Comment 17
2013-10-28 12:59:51 PDT
http://trac.webkit.org/changeset/158122
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug