[webkit-reviews] review granted: [Bug 27314] Implement Accelerated Compositing on Windows (3D CSS Transforms) : [Attachment 43743] Replacement patch for all 3 previous patches
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 24 10:56:16 PST 2009
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 27314: Implement Accelerated Compositing on Windows (3D CSS Transforms)
https://bugs.webkit.org/show_bug.cgi?id=27314
Attachment 43743: Replacement patch for all 3 previous patches
https://bugs.webkit.org/attachment.cgi?id=43743&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/WebCore.vcproj/WebCore.vcproj
> ===================================================================
> <File
> +
RelativePath="..\platform\graphics\TextRenderingMode.h"
> + >
> + </File>
> + <File
>
RelativePath="..\platform\win\WCDataObject.cpp"
> >
> <FileConfiguration
> @@ -22110,6 +22120,68 @@
> >
> </File>
> <File
> + RelativePath="..\WebCorePrefix.cpp"
> + >
Not sure why this moved around.
> Index: WebCore/platform/graphics/GraphicsLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/GraphicsLayer.cpp (revision 51122)
> +++ WebCore/platform/graphics/GraphicsLayer.cpp (working copy)
> @@ -69,7 +69,7 @@ GraphicsLayer::GraphicsLayer(GraphicsLay
> , m_drawsContent(false)
> , m_paintingPhase(GraphicsLayerPaintAll)
> , m_geometryOrientation(CompositingCoordinatesTopDown)
> - , m_contentsOrientation(CompositingCoordinatesTopDown)
> + , m_contentsOrientation(CompositingCoordinatesBottomUp)
Why this change?
> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================
> +GraphicsLayerCACF::GraphicsLayerCACF(GraphicsLayerClient* client)
> +: GraphicsLayer(client)
> +, m_contentsLayerPurpose(NoContentsLayer)
These should be indented.
> +void GraphicsLayerCACF::removeFromParent()
> +{
> + GraphicsLayer::removeFromParent();
> + layerForSuperlayer()->removeFromSuperlayer();
> +}
Why isn't this updating the parent's sublayer list?
> +void GraphicsLayerCACF::setAnchorPoint(const FloatPoint3D& point)
> +{
> + GraphicsLayer::setAnchorPoint(point);
> +
> + // set the value on the layer to the new transform
Comment makes no sense.
> + primaryLayer()->setAnchorPoint(FloatPoint(point.x(), point.y()));
> + primaryLayer()->setAnchorPointZ(m_anchorPoint.z());
> +
> + // position depends on anchor point
> + setPosition(m_position);
> +}
> +
> +void GraphicsLayerCACF::setSize(const FloatSize& size)
> +{
> + GraphicsLayer::setSize(size);
> +
> + CGRect rect = CGRectMake(0.0f, 0.0f, m_size.width(), m_size.height());
> +
> + if (m_transformLayer) {
> + m_transformLayer->setBounds(rect);
> +
> + // the anchor of the contents layer is always at 0.5, 0.5, so the
position
> + // is center-relative
Sentence-case, please.
> + bool needTiledLayer = requiresTiledLayer(m_size);
> + if (needTiledLayer != m_usingTiledLayer) {
> + swapFromOrToTiledLayer(needTiledLayer);
I think you should just remove all the tiled layer code. It's better ot just
make large layers
than have layers stop working when they get big.
> + // if we've changed the bounds, we need to recalculate the position
> + // of the layer, taking anchor point into account
Sentence-case.
> +void GraphicsLayerCACF::setTransform(const TransformationMatrix& t)
> +{
> + GraphicsLayer::setTransform(t);
> +
> + CATransform3D transform;
> + copyTransform(transform, t);
> + primaryLayer()->setTransform(transform);
> +}
This should bail if t == m_transform.
> +void GraphicsLayerCACF::setChildrenTransform(const TransformationMatrix& t)
> +{
This should bail if t == m_childrenTransform.
> +void GraphicsLayerCACF::setPreserves3D(bool preserves3D)
> +{
> + GraphicsLayer::setPreserves3D(preserves3D);
Do an early return if it didn't change.
> + CGPoint point = CGPointMake(m_size.width() / 2.0f, m_size.height() /
2.0f);
> + CGPoint centerPoint = CGPointMake(0.5f, 0.5f);
> +
> + if (preserves3D && !m_transformLayer) {
Rather than do all this work here, I think it would be better for future
refactoring
if you stayed closer to GraphicsLayerCA, and kept the update...() functions, so
here
you'd call updateLayerPreserves3D().
Also, you have some tabs in this code.
> +void GraphicsLayerCACF::setDrawsContent(bool inDrawsContent)
> +{
> + if (inDrawsContent != m_drawsContent) {
Early return is easier to read.
> +void GraphicsLayerCACF::setBackfaceVisibility(bool visible)
> +{
> + if (m_backfaceVisibility == visible)
> + return;
> +
> + GraphicsLayer::setBackfaceVisibility(visible);
> +
> + m_layer->setDoubleSided(visible);
> +}
> +
Extra whitespace
> +void GraphicsLayerCACF::setOpacity(float opacity)
> +{
> + float clampedOpacity = max(min(opacity, 1.0f), 0.0f);
> +
> + bool opacitiesDiffer = (m_opacity != clampedOpacity);
opacitiesDiffer is unused. Use the early return form.
> +void GraphicsLayerCACF::updateContentsRect()
So here is one update function. Why this and not the others?
> +PlatformLayer* GraphicsLayerCACF::hostLayerForSublayers() const
> +{
> + return m_transformLayer ? m_transformLayer.get() : m_layer.get();
> +}
> +
> +PlatformLayer* GraphicsLayerCACF::layerForSuperlayer() const
> +{
> + if (m_transformLayer)
> + return m_transformLayer.get();
> +
> + return m_layer.get();
> +}
Not sure why those methods don't both use the conditional operator.
> +#ifndef NDEBUG
> +void GraphicsLayerCACF::setDebugBackgroundColor(const Color& color)
These are no longer debug-only.
> +void GraphicsLayerCACF::updateContentsTransform()
> +{
> + ASSERT(contentsOrientation() == CompositingCoordinatesTopDown);
Since updateContentsTransform() is an internal method, I'm not sure why this is
useful.
> +void GraphicsLayerCACF::setupContentsLayer(WKCACFLayer* contentsLayer)
> +#ifndef NDEBUG
> + if (showDebugBorders()) {
> + setLayerBorderColor(m_contentsLayer.get(), Color(0, 0, 128,
180));
> + m_contentsLayer->setBorderWidth(1.0f);
> + }
> +#endif
> + }
> +#ifndef NDEBUG
> + updateDebugIndicators();
> +#endif
No longer debug-only.
> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.h
> ===================================================================
> +class GraphicsLayerCACF : public GraphicsLayer {
> +public:
> +
> + GraphicsLayerCACF(GraphicsLayerClient*);
> + virtual ~GraphicsLayerCACF();
> +
> + virtual void layout() { }
Where did layout() come from?
> +#ifndef NDEBUG
> + virtual void setDebugBackgroundColor(const Color&);
> + virtual void setDebugBorder(const Color&, float borderWidth);
> +#endif
No longer debug-only.
> + CompositingCoordinatesOrientation defaultContentsOrientation() const;
> + void updateContentsTransform();
> + void updateSublayerList();
You have tabs here.
> Index: WebCore/platform/graphics/win/WKCACFContextFlusher.cpp
> ===================================================================
> +WKCACFContextFlusher& WKCACFContextFlusher::shared()
> +{
> + static WKCACFContextFlusher& flusher = *new WKCACFContextFlusher;
This should use the DEFINE_STATIC_LOCAL macro.
> +void WKCACFContextFlusher::addContext(CACFContextRef context)
> +{
> + if (!context)
> + return;
Or ASSERT, perhaps?
> +
> + if (m_contexts.add(context).second)
> + CFRetain(context);
This is pretty confusing. What is add().second testing?
> +void WKCACFContextFlusher::removeContext(CACFContextRef context)
> +{
> + if (!context)
> + return;
> +
> + ContextSet::iterator found = m_contexts.find(context);
> + if (found == m_contexts.end())
> + return;
> +
> + CFRelease(*found);
> + m_contexts.remove(found);
> +}
Do calls to addContext() and removeContext() with the same context properly
nest?
This seems like fragile code.
> Index: WebCore/platform/graphics/win/WKCACFContextFlusherWin.cpp
> ===================================================================
> +void WKCACFContextFlusher::didFlushContext(CACFContextRef context)
> +{
> + WKCACFLayerWindow::didFlushContext(context);
> +}
Since WKCACFContextFlusher will probably only ever be used on Windows,
I don't see the benefit of slicing this part into a separate file.
> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> +void WKCACFLayer::display(PlatformGraphicsContext* context)
> +#ifndef NDEBUG
No longer debug-only.
> +void WKCACFLayer::layout()
> +{
> + if (m_owner)
> + m_owner->layout();
> +}
GraphicsLayers have no notion of layout. I think this should just be a stub.
> +void WKCACFLayer::becomeLayerForContext(CACFContextRef context)
> +{
> + CACFContextSetLayer(context, layer());
> + setNeedsCommit();
> +}
I've no idea what this means. Does it make this layer the root layer for that
context,
or something else? It needs a comment, and a better name.
> +CGPoint WKCACFLayer::convertPoint(const CGPoint& p, const WKCACFLayer*
toLayer) const
> +{
> + CGPoint convertedPoint = p;
> + CACFLayerConvertPoint(layer(), toLayer ? toLayer->layer() : 0,
&convertedPoint);
> + return convertedPoint;
> +}
> +
> +CGRect WKCACFLayer::convertRect(const CGRect& r, const WKCACFLayer* toLayer)
const
> +{
> + CGRect convertedRect = r;
> + CACFLayerConvertRect(layer(), toLayer ? toLayer->layer() : 0,
&convertedRect);
> + return convertedRect;
> +}
I don't think we'll ever need these.
> +bool WKCACFLayer::isDescendantOf(WKCACFLayer* ancestor) const
Will return true when called with itself. That should be more explicit in a
comment
or in the method name.
> +void WKCACFLayer::replaceSublayer(WKCACFLayer* reference,
PassRefPtr<WKCACFLayer> newLayer)
> +{
> + //RefPtr<WKCACFLayer> newLayerRef = newLayer;
Don't check in commented code.
> +void WKCACFLayer::removeFromSuperlayer()
> +{
> + WKCACFLayer* superlayer = this->superlayer();
> + if (!superlayer)
> + return;
> +
> + superlayer->removeSublayer(this);
> + CACFLayerRemoveFromSuperlayer(layer());
> + setNeedsCommit();
> +}
Why is setNeedsCommit() called on this, and not on the superlayer?
> +WKCACFLayer* WKCACFLayer::ancestorOrSelfWithSuperlayer(WKCACFLayer*
superlayer) const
Is this used in a way that isDescendantOf() would not work for?
> +void WKCACFLayer::removeAllSublayers()
> +{
> + CACFLayerSetSublayers(layer(), 0);
> +}
I don't grok why setNeedsCommit() isn't needed here either.
> +void WKCACFLayer::setSublayers(Vector<RefPtr<WKCACFLayer> >& sublayers)
Can that parameter be const?
> +{
> + if (sublayers.isEmpty())
> + CACFLayerSetSublayers(layer(), 0);
> + else {
> + // Create a vector of CACFLayers.
> + Vector<const void*> layers;
> + for (size_t i = 0; i < sublayers.size(); i++)
> + layers.append(sublayers[i]->layer());
> +
> + RetainPtr<CFArrayRef> layersArray(AdoptCF, CFArrayCreate(0,
layers.data(), layers.size(), 0));
> + CACFLayerSetSublayers(layer(), layersArray.get());
Does this work? Does CACFLayerSetSublayers() just copy out of the array and do
its own retaining, or is it just
taking ownership of the array? If the latter, then the array members are not
being retained.
> +void WKCACFLayer::moveSublayers(WKCACFLayer* fromLayer, WKCACFLayer*
toLayer)
> +{
> + if (!fromLayer || !toLayer)
> + return;
> +
> + CACFLayerSetSublayers(toLayer->layer(),
CACFLayerGetSublayers(fromLayer->layer()));
Why no setNeedsCommit()?
> +void WKCACFLayer::print()
> +{
> + //CARenderShow(CACFLayerCopyRenderValue(layer()));
> +}
Why commented out?
> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> + void becomeLayerForContext(CACFContextRef);
This needs a comment explaining what it means.
> + using RefCounted<WKCACFLayer>::ref;
> + using RefCounted<WKCACFLayer>::deref;
Are these still required?
> + CGPoint anchorPoint() { return CACFLayerGetAnchorPoint(layer()); }
> + CGFloat anchorPointZ() { return CACFLayerGetAnchorPointZ(layer()); }
> + CFStringRef contentsGravity() { return
CACFLayerGetContentsGravity(layer()); }
These should all be const.
> + void setNeedsDisplay(const CGRect& dirtyRect)
> + {
> + if (m_owner)
> + CACFLayerSetNeedsDisplay(layer(), &dirtyRect);
> + setNeedsCommit();
> + }
> +
> + void setNeedsDisplay()
> + {
> + if (m_owner)
> + CACFLayerSetNeedsDisplay(layer(), 0);
> + setNeedsCommit();
> + }
I don't think these should be inline. I'd prefer to see all the calls to
setNeedsCommit() in the .cpp file.
> + void setOpacity(float opacity)
> + {
> + CACFLayerSetOpacity(layer(), opacity);
> + setNeedsCommit();
> + }
Ditto.
> + void setSublayers(Vector<RefPtr<WKCACFLayer> >&);
const Vector?
> + int findSublayer(const WKCACFLayer*);
Badly named. What is the return value? Should be const.
> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.cpp
> ===================================================================
> +inline static CGRect winRectToCGRect(RECT rc)
> +{
> + return CGRectMake((float)rc.left, (float)rc.top, (float)(rc.right -
rc.left), (float)(rc.bottom - rc.top));
Use static_cast?
> +inline static CGRect winRectToCGRect(RECT rc, RECT relativeToRect)
> +{
> + return CGRectMake((float)rc.left,
(float)(relativeToRect.bottom-rc.bottom), (float)(rc.right - rc.left),
(float)(rc.bottom - rc.top));
Ditto.
> +static ContextToWindowMap& windowsForContexts()
> +{
> + static ContextToWindowMap& map = *new ContextToWindowMap;
> + return map;
Use DEFINE_STATIC_LOCAL.
> +WKCACFLayerWindow::WKCACFLayerWindow()
> + : m_triedToCreateD3DRenderer(false)
> + , m_renderContext(0)
> + , m_renderer(0)
> + , m_hostWindow(0)
> + , m_renderTimer(this, &WKCACFLayerWindow::renderTimerFired)
> + , m_scrollFrameWidth(1)
> + , m_scrollFrameHeight(1)
Are those reasonable defaults for m_scrollFrameWidth and m_scrollFrameHeight?
> +void WKCACFLayerWindow::createRenderer()
> + CGColorRef debugColor = createCGColor(Color(255, 0, 0, 204));
> + m_rootLayer->setBackgroundColor(debugColor);
> + CGColorRelease(debugColor);
Do you really always want to do this?
> +void WKCACFLayerWindow::createRenderer()
> + D3DXMATRIXA16 projection;
> + D3DXMatrixOrthoOffCenterRH(&projection, rect.left, rect.right, rect.top,
rect.bottom, -1.0f, 1.0f);
Why isn't this done with a call to initD3DGeometry() or resetDevice()?
> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.h
> ===================================================================
This needs some comments explaining what it does, its lifetime, how many there
are etc.
> +class WKCACFLayerWindow : Noncopyable {
Should be public Noncopyable.
> Index: WebKit/win/WebView.cpp
> ===================================================================
> +void WebView::setAcceleratedCompositing(bool accelerated)
> +{
> + if (m_isAcceleratedCompositing == accelerated)
> + return;
> +
> + m_isAcceleratedCompositing = accelerated;
> + if (m_isAcceleratedCompositing) {
> + // Create the root layer
> + ASSERT(m_viewWindow);
> + m_layerWindow.setHostWindow(m_viewWindow);
> + setRootLayerContents();
> + } else {
> + // Tear down the root layer
> + m_layerWindow.destroyRenderer();
> + }
> +
> + //invalidateBackingStore(0);
> + //::UpdateWindow(m_viewWindow);
> + paint(0, 0);
Should not commit commented code.
> +}
> +
> +void WebView::setRootLayerContents()
It's weird having a set() method with no parameters. Maybe
updateRootLayerContents()?
> + RetainPtr<CFDataRef> data(AdoptCF,
> + CFDataCreateWithBytesNoCopy(
> + 0,
static_cast<UInt8*>(bitmap.bmBits),
> + bmSize, kCFAllocatorNull));
> + CGDataProviderRef cgData = CGDataProviderCreateWithCFData(data.get());
> + CGColorSpaceRef space = CGColorSpaceCreateDeviceRGB();
Would be good to use RetainPtr consistently here, for these two.
> Index: WebKit/win/WebCoreSupport/WebChromeClient.cpp
> ===================================================================
> +void WebChromeClient::attachRootGraphicsLayer(Frame* frame, GraphicsLayer*
graphicsLayer)
> +{
> + m_webView->setRootChildLayer(graphicsLayer ?
graphicsLayer->platformLayer() : 0);
Tab here.
> Index: WebKit/win/WebKit.vcproj/WebKit.vcproj
> ===================================================================
> </File>
> <File
> - RelativePath="..\WebScriptWorld.h"
> - >
> - </File>
> - <File
> RelativePath="..\WebKitStatistics.h"
> >
> </File>
> @@ -794,6 +790,10 @@
> >
> </File>
> <File
> + RelativePath="..\WebScriptWorld.h"
> + >
> + </File>
> + <File
> RelativePath="..\WebScrollBar.h"
> >
> </File>
> @@ -1094,10 +1094,6 @@
> >
> </File>
> <File
> - RelativePath="..\WebScriptWorld.cpp"
> - >
> - </File>
> - <File
> RelativePath="..\WebKitStatistics.cpp"
> >
> </File>
> @@ -1130,6 +1126,10 @@
> >
> </File>
> <File
> + RelativePath="..\WebScriptWorld.cpp"
> + >
> + </File>
> + <File
> RelativePath="..\WebScrollBar.cpp"
> >
> </File>
Maybe edit out thes unwanted changes.
> Index: WebKitTools/Scripts/webkitdirs.pm
> ===================================================================
> --- WebKitTools/Scripts/webkitdirs.pm (revision 51122)
> +++ WebKitTools/Scripts/webkitdirs.pm (working copy)
> @@ -647,7 +647,8 @@ sub checkWebCoreSVGSupport
>
> sub hasAcceleratedCompositingSupport
> {
> - return 0 if isCygwin() || isQt();
> + # On platforms other than Mac the Skipped files are used to skip
compositing tests
> + return 1 if !isAppleMacWebKit();
Tabs here.
> my $path = shift;
> return libraryContainsSymbol($path, "GraphicsLayer");
> @@ -667,7 +668,8 @@ sub checkWebCoreAcceleratedCompositingSu
>
> sub has3DRenderingSupport
> {
> - return 0 if isQt();
> + # On platforms other than Mac the Skipped files are used to skip 3D
tests
> + return 1 if !isAppleMacWebKit();
Tabs here
r=me with all those changes addressed, but I would prefer that
GraphicsLayerCACF stays closer to GraphicsLayerCA.
More information about the webkit-reviews
mailing list