[webkit-reviews] review denied: [Bug 27314] 3D CSS Transforms are not implemented in WebKit for Windows : [Attachment 41071] First patch (new files)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 12 21:30:57 PDT 2009
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 27314: 3D CSS Transforms are not implemented in WebKit for Windows
https://bugs.webkit.org/show_bug.cgi?id=27314
Attachment 41071: First patch (new files)
https://bugs.webkit.org/attachment.cgi?id=41071&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
In general, I wonder if we should try to share more code between the Mac and
Windows implementations.
I feel like we're going to be fixing all the bugs twice if the code is in both.
> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================
> +namespace WebCore {
> +
> +// The threshold width or height above which a tiled layer will be used.
This should be
> +// large enough to avoid tiled layers for most GraphicsLayers, but less than
the OpenGL
> +// texture size limit on all supported hardware.
> +static const int cMaxPixelDimension = 2000;
> +
> +// The width and height of a single tile in a tiled layer. Should be large
enough to
> +// avoid lots of small tiles (and therefore lots of drawing callbacks), but
small enough
> +// to keep the overall tile cost low.
> +static const int cTiledLayerTileSize = 512;
I think you should just remove all the tiled layer-related code.
> +} // namespace WebCore
> +
> +namespace WebCore {
Why close and re-open the namespace?
> +#ifndef NDEBUG
> +bool GraphicsLayer::showDebugBorders()
> +{
> + static bool showDebugBorders = false; //[[NSUserDefaults
standardUserDefaults] boolForKey:@"WebCoreLayerBorders"];
> + return showDebugBorders;
> +}
> +
> +bool GraphicsLayer::showRepaintCounter()
> +{
> + static bool showRepaintCounter = false; //[[NSUserDefaults
standardUserDefaults] boolForKey:@"WebCoreLayerRepaintCounter"];
> + return showRepaintCounter;
> +}
> +#endif
You shouldn't check in commented out code. We should figure out how to store
preferences on Windows, or remove these methods.
> +void GraphicsLayerCACF::addChildBelow(GraphicsLayer* childLayer,
GraphicsLayer* sibling)
> +{
> + // FIXME share code
> + ASSERT(!childLayer->parent());
> + ASSERT(childLayer != this);
This is what used to be in GraphicsLayerCA.mm before I implemented batching. I
wonder if it makes sense to share the same code on Windows but just commit the
batch more eagerly? In addition, the change to use batching also fixed a number
of bugs; you're copying older code with bugs in here.
> +void GraphicsLayerCACF::removeFromParent()
> +{
> + GraphicsLayer::removeFromParent();
> +
> +
> + layerForSuperlayer()->removeFromSuperlayer();
> +}
Extra space here.
> +
> +void GraphicsLayerCACF::setPosition(const FloatPoint& point)
> +{
> + // don't short-circuit here, because position and anchor point
> + // are inter-related
> + GraphicsLayer::setPosition(point);
> +
> + // position is offset on the layer by the layer anchor point
> + CGPoint posPoint = CGPointMake(m_position.x() + m_anchorPoint.x() *
m_size.width(),
> + m_position.y() + m_anchorPoint.y() *
m_size.height());
> +
> + primaryLayer()->setPosition(posPoint);
> +}
> +
> +void GraphicsLayerCACF::setAnchorPoint(const FloatPoint3D& point)
> +{
> + // Don't short-circuit here, because position and anchor point are
inter-dependent.
Use this same comment formatting in setPosition().
> +void GraphicsLayerCACF::setOpacity(float opacity)
> +{
> + float clampedOpacity = std::max(std::min(opacity, 1.0f), 0.0f);
You did 'using namespace std' at the top so this should obmit the std::
> +void GraphicsLayerCACF::setNeedsDisplayInRect(const FloatRect& rect)
> +{
> + if (drawsContent()) {
> + CGRect r;
> + r.origin.x = rect.x(); r.origin.y = rect.y();
> + r.size.width = rect.width(); r.size.height = rect.height();
Tabs here!
There's an operator CGRect on FloatRect; no need to convert here.
> +void GraphicsLayerCACF::setContentsToImage(Image* image)
> +{
> + if (image) {
> + if (!m_contentsLayer.get()) {
> +
> + RefPtr<WKCACFLayer> imageLayer = WKCACFLayer::create(kCACFLayer,
this);
> +#ifndef NDEBUG
> + imageLayer->setName("Image Layer");
> +#endif
> + setupContentsLayer(imageLayer.get());
> + }
> +
> + // FIXME: maybe only do trilinear if the image is being scaled down,
> + // but then what if the layer size changes?
> + m_contentsLayer->setMinificationFilter(kCACFFilterTrilinear);
> + CGImageRef theImage = image->nativeImageForCurrentFrame();
> + m_contentsLayer->setContents(theImage);
This is missing the bug fix related to image color profiles.
> +void GraphicsLayerCACF::setupContentsLayer(WKCACFLayer* contentsLayer)
> +{
> + if (contentsLayer == m_contentsLayer)
> + return;
> +
> + if (m_contentsLayer) {
> + m_contentsLayer->removeFromSuperlayer();
> + m_contentsLayer = 0;
> + }
> +
> + if (contentsLayer) {
> + // Turn off implicit animations on the inner layer.
> + m_contentsLayer = contentsLayer;
The comment is not relevant.
> +#endif // USE(ACCELERATED_COMPOSITI
> \ No newline at end of file
Looks like you truncated the file here.
> Property changes on: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ___________________________________________________________________
> Added: svn:executable
Really? Other files have this too.
> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.h
> ===================================================================
> + // return true if we started an animation
> + virtual void setOpacity(float opacity);
Comment is bogus.
> Index: WebCore/platform/graphics/win/WKCACFContextFlusher.cpp
> ===================================================================
> +/*
> + * ContextFlusher.cpp
> + * WebCore
> + *
> + * Created by Adam Roben on 6/24/08.
> + * Adapted from ContextFlusher by Chris Marrin on 10/21/08.
> + * Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */
Needs a real license.
Does the whole file need #if USE(ACCELERATED_COMPOSITING) ?
> +#include <QuartzCore/CACFContext.h>
How will people building WebKit standalone get that header file?
> +void WKCACFContextFlusher::flushAllContexts()
> +{
> + // addContext might get called beneath CACFContextFlush, and we don't
want m_contexts to change while
> + // we're iterating over it, so we move the contexts into a local
ContextSet and iterate over that instead.
> + ContextSet contextsToFlush;
> + contextsToFlush.swap(m_contexts);
> +
> + ContextSet::const_iterator end = contextsToFlush.end();
> + for (ContextSet::const_iterator it = contextsToFlush.begin(); it != end;
++it) {
> + CACFContextRef context = *it;
> + CACFContextFlush(context);
> + didFlushContext(context);
> + CFRelease(context);
Why is this CFRelease here? Did *it implicitly CFRetain?
> Index: WebCore/platform/graphics/win/WKCACFContextFlusher.h
> ===================================================================
> +/*
> + * ContextFlusher.h
> + * WebCore
> + *
> + * Created by Adam Roben on 6/24/08.
> + * Adapted from ContextFlusher by Chris Marrin on 10/21/08.
> + * Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */
Needs a real license.
Does the whole file need #if USE(ACCELERATED_COMPOSITING) ?
> +
> +#ifndef WKCACFContextFlusher_h
> +#define WKCACFContextFlusher_h
> +
> +#include "config.h"
config.h should not be included in header files.
> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> +/*
> + * WKCACFLayer.cpp
> + * WebCore
> + *
> + * Created by Adam Roben on 6/24/08.
> + * Adapted from CoreAnimationLayer by Chris Marrin on 10/21/08.
> + * Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */
Needs license.
Does the whole file need #if USE(ACCELERATED_COMPOSITING) ?
> +
> +#include "WKCACFLayer.h"
> +
> +#include "WKCACFContextFlusher.h"
> +
> +#include <stdio.h>
> +#include <QuartzCore/CACFContext.h>
> +#include <QuartzCore/CARender.h>
> +#include <QuartzCore/CATransform3DPrivate.h>
Can non-Apple people build this?
> + // These macros were copied from
QuartzCore/LayerKit/utilities/CACGUtil.h
I don't think you should say this here.
> +void WKCACFLayer::display(PlatformGraphicsContext* context)
> +{
> + if (!m_owner)
> + return;
> +
> + CGContextSaveGState(context);
> +
> + CGRect layerBounds = bounds();
> + if (m_owner->contentsOrientation() ==
WebCore::GraphicsLayer::CompositingCoordinatesBottomUp) {
> + CGContextScaleCTM(context, 1, -1);
> + CGContextTranslateCTM(context, 0, -layerBounds.size.height);
> + }
> +
> + if (m_owner->client()) {
> + GraphicsContext graphicsContext(context);
> +
> + // It's important to get the clip from the context, because it may
be significantly
> + // smaller than the layer bounds (e.g. tiled layers)
> + CGRect clipBounds = CGContextGetClipBoundingBox(context);
> + IntRect clip(enclosingIntRect(clipBounds));
> + m_owner->paintGraphicsLayerContents(graphicsContext, clip);
> + }
> +#ifndef NDEBUG
> + else {
> + ASSERT_NOT_REACHED();
> +
> + // FIXME: ideally we'd avoid calling -setNeedsDisplay on a layer
that is a plain color,
> + // so CA never makes backing store for it (which is what
-setNeedsDisplay will do above).
> + CGContextSetRGBFillColor(context, 0.0f, 1.0f, 0.0f, 1.0f);
> + CGContextFillRect(context, layerBounds);
> + }
> +#endif
> +
> +#ifndef NDEBUG
> + if (m_owner->showRepaintCounter()) {
> + char text[16]; // that's a lot of repaints
> + _snprintf(text, sizeof(text), "%d",
m_owner->incrementRepaintCount());
> +
> + CGContextSaveGState(context);
> + CGContextSetRGBFillColor(context, 1.0f, 0.0f, 0.0f, 0.8f);
> +
> + CGRect aBounds = layerBounds;
> +
> + aBounds.size.width = 10 + 12 * strlen(text);
> + aBounds.size.height = 25;
> + CGContextFillRect(context, aBounds);
> +
> + CGContextSetRGBFillColor(context, 0.0f, 0.0f, 0.0f, 1.0f);
> +
> + CGContextSetTextMatrix(context, CGAffineTransformMakeScale(1.0f,
-1.0f));
> + CGContextSelectFont(context, "Helvetica", 25, kCGEncodingMacRoman);
> + CGContextShowTextAtPoint(context, aBounds.origin.x + 3.0f,
aBounds.origin.y + 20.0f, text, strlen(text));
> +
> + CGContextRestoreGState(context);
> + }
> +#endif
> +
> + CGContextRestoreGState(context);
> +}
I'm not sure this code belongs here. WKCACFLayer is really just a C++ wrapper
around CACFLayer, and, and such, it should not have anything WebKit-specific. I
think you need a subclass which knows about GraphicsContexts and painting.
> +void WKCACFLayer::layout()
> +{
> + if (m_owner)
> + m_owner->layout();
> +}
Tabs here.
> +void WKCACFLayer::becomeLayerForContext(CACFContextRef context)
> +{
> + CACFContextSetLayer(context, layer());
> + setNeedsCommit();
> +}
> +
> +void WKCACFLayer::setNeedsCommit()
> +{
> +
WKCACFContextFlusher::shared().addContext(CACFLayerGetContext(rootLayer()->laye
r()));
> + if (m_owner)
> + m_owner->notifySyncRequired();
> +}
notifySyncRequired() is only required if you do layer property batching.
> +void WKCACFLayer::insertSublayer(PassRefPtr<WKCACFLayer> prpSublayer, size_t
index)
prpSublayer? No Hungarian notation please. Why is it a PassRefPtr when the code
immediately makes a RefPtr out of it?
> +void WKCACFLayer::setSublayersByAdoptingVector(Vector<RefPtr<WKCACFLayer> >&
sublayers)
> +{
> + checkSublayersConsistency();
> +
> + if (sublayers.isEmpty()) {
> + CACFLayerSetSublayers(layer(), 0);
> + m_sublayers.clear();
> + } else {
> + // FIXME: This call to CACFLayerSetSublayers is a workaround for
> + // <rdar://6199008>. We can remove it once that bug is fixed.
> + CACFLayerSetSublayers(layer(), 0);
> +
> + // 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());
> +
> + m_sublayers.clear();
> + m_sublayers.swap(sublayers);
> + }
> +
> + setNeedsCommit();
> +
> + checkSublayersConsistency();
> +}
I don't really like how both WKCACFLayer and CACFLayer both store an array of
sublayers. That means that both have to be kept in sync, and is yet another
paralell tree that we have to maintain (GraphicsLayer also has one). If you can
always get from a CACFLayer to a WKCACFLayer, then no extra sublayer storage is
required.
> +#endif // CHECK_SUBLAYERS_CONSISTENCY
> +
> +}
> \ No newline at end of file
Fix this.
> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.h (revision 0)
> +++ WebCore/platform/graphics/win/WKCACFLayer.h (revision 0)
> @@ -0,0 +1,255 @@
> +/*
> + * WKCACFLayer.h
> + * WebKit
> + *
> + * Created by Adam Roben on 6/11/08.
> + * Adapted from CoreAnimationLayer by Chris Marrin on 10/21/08.
> + * Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */
Needs license.
> +#pragma mark Conversion to CFType
> + static RetainPtr<CFTypeRef> cfValue(float value) { return
RetainPtr<CFTypeRef>(AdoptCF, CFNumberCreate(0, kCFNumberFloat32Type, &value));
}
> + static RetainPtr<CFTypeRef> cfValue(const TransformationMatrix& value)
> + {
> + CATransform3D t;
> + t.m11 = value.m11(); t.m12 = value.m12(); t.m13 = value.m13();
t.m14 = value.m14();
> + t.m21 = value.m21(); t.m22 = value.m22(); t.m23 = value.m23();
t.m24 = value.m24();
> + t.m31 = value.m31(); t.m32 = value.m32(); t.m33 = value.m33();
t.m34 = value.m34();
> + t.m41 = value.m41(); t.m42 = value.m42(); t.m43 = value.m43();
t.m44 = value.m44();
> + return RetainPtr<CFTypeRef>(AdoptCF,
CACFVectorCreateTransform(t));
> + }
> + static RetainPtr<CFTypeRef> cfValue(const FloatPoint& value)
> + {
> + CGPoint p;
> + p.x = value.x(); p.y = value.y();
> + return RetainPtr<CFTypeRef>(AdoptCF, CACFVectorCreatePoint(p));
> + }
> + static RetainPtr<CFTypeRef> cfValue(const FloatRect& rect)
> + {
> + CGRect r;
> + r.origin.x = rect.x(); r.origin.y = rect.y();
> + r.size.width = rect.width(); r.size.height = rect.height();
> + CGFloat v[4] = { CGRectGetMinX(r), CGRectGetMinY(r),
CGRectGetMaxX(r), CGRectGetMaxY(r) };
Lots of tabs here.
> +#pragma mark Coordinate Mapping
I don't think you should use #pragma mark in WebCore code.
> + // FIXME: Make this private once <rdar://6200213> is fixed and the
> + // workaround in CylindricalGridLayer.cpp has been removed.
> + void setNeedsCommit();
Comment is not relevant here.
> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.cpp
> ===================================================================
> +/*
> + * WKCACFLayerWindow.cpp
> + * WebCore
> + *
> + * Created by Adam Roben on 6/24/08.
> + * Adapted from CALayerWindow by Chris Marrin on 10/21/08.
> + * Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */
License needed.
> +#include "WKCACFLayerWindow.h"
> +
> +#include "WKCACFContextFlusher.h"
> +#include "WKCACFLayer.h"
> +//#include "DrawingUtilities.h"
Remove it if it's not needed.
> +// pulled from DrawingUtilities.h
Not relevant here.
> +static D3DPRESENT_PARAMETERS initialPresentationParameters()
> +{
> + D3DPRESENT_PARAMETERS parameters = {0};
> + parameters.Windowed = TRUE;
> + parameters.SwapEffect = D3DSWAPEFFECT_COPY;
> + parameters.BackBufferCount = 1;
> + parameters.BackBufferFormat = D3DFMT_A8R8G8B8;
> + parameters.MultiSampleType = D3DMULTISAMPLE_NONE;
> +
> + return parameters;
Tab here.
> +void WKCACFLayerWindow::setScrollFrame(int width, int height, int scrollX,
int scrollY)
> +{
> + CGRect contentsRect = bounds();
> + contentsRect.size.width = width;
> + contentsRect.size.height = height;
> + contentsRect.origin.x = scrollX;
> + contentsRect.origin.y = scrollY;
Why init the rect to bounds() and then replace every member?
> +void WKCACFLayerWindow::setRootContents(CGImageRef image)
> +{
> + ASSERT(m_rootLayer);
> + m_rootLayer->setContents(image);
> + renderSoon();
Tab here.
> +void WKCACFLayerWindow::setRootChildLayer(WebCore::PlatformLayer* layer)
> +{
> + if (!m_scrollLayer)
> + return;
> +
> + m_scrollLayer->removeAllSublayers();
> + if (layer)
> + m_scrollLayer->addSublayer(layer);
Tab here.
> + // D3D doesn't like to make back buffers for 0 size windows. We skirt
this problem if we make the
> + // passed backbuffer width and height non-zero. The window will
necessarily get set to a non-zero
> + // size eventually, and then the backbuffer size will get reset.
> + RECT rect;
> + GetClientRect(m_hostWindow, &rect);
> +
> + if (rect.left-rect.right == 0 || rect.bottom-rect.top == 0) {
> + parameters.BackBufferWidth = 1;
> + parameters.BackBufferHeight = 1;
> + }
Lots of tabs.
> + // Create the root hierarchy
> + m_rootLayer = WKCACFLayer::create(kCACFLayer);
> + m_scrollLayer = WKCACFLayer::create(kCACFLayer);
Tabs.
> +void WKCACFLayerWindow::resize()
> +{
> + if (!m_d3dDevice)
> + return;
> +
> + resetDevice();
> +
> + if (m_rootLayer) {
> + m_rootLayer->setFrame(bounds());
> +
> + // FIXME: This should be the bounds inside the scrollbars
> + m_scrollLayer->setFrame(bounds());
It worries me that there's code down here that has to know about scrollbars.
> +void WKCACFLayerWindow::render(const Vector<CGRect>& dirtyRects)
> +{
> + ASSERT(m_d3dDevice);
> +
> + // Flush the root layer to the render tree.
> + WKCACFContextFlusher::shared().flushAllContexts();
> +
> + CGRect bounds = this->bounds();
> +
> + CFTimeInterval t = CACurrentMediaTime();
> +
> + char space[4096];
This is very myserious. What is it for?
> + CARenderUpdate* u = CARenderUpdateBegin(space, sizeof(space), t, 0, 0,
&bounds);
> + /* FIXME: don't need to clear dirty region if layer tree is opaque.
*/
The FIXMEs need bugs filed on them. Use C++ comments, not C-style comments.
> + if (err == D3DERR_DEVICELOST) {
> + /* Lost device situation. */
Use C++ comments.
> + CARenderUpdateFinish (u);
No space before (
> +void WKCACFLayerWindow::renderSoon()
> +{
> + m_renderTimer.startOneShot(0);
Should this make sure the timer isn't already active?
> +}
> \ No newline at end of file
Fix this.
> Index: WebCore/platform/graphics/win/WKCACFLayerWindow.h
> ===================================================================
> +/*
> + * WKCACFLayerWindow.h
> + * WebCore
> + *
> + * Created by Adam Roben on 6/24/08.
> + * Adapted from CALayerWindow by Chris Marrin on 10/21/08.
> + * Copyright 2008 Apple Inc. All rights reserved.
> + *
> + */
Needs license.
So my main concerns here are:
1. Is it buildable by someone outside of Apple?
2. GraphicsLayerCACF should be closer to GraphicsLayerCA, and maybe share some
code.
3. The extra sublayer storage by WKCACFLayer
4. WKCACFLayer knowing about WebCore stuff
More information about the webkit-reviews
mailing list