[webkit-reviews] review denied: [Bug 35557] [chromium] Implement composited graphics layers with Skia : [Attachment 50753] Patch addressing review comments

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 35557: [chromium] Implement composited graphics layers with Skia
https://bugs.webkit.org/show_bug.cgi?id=35557

Attachment 50753: Patch addressing review comments
https://bugs.webkit.org/attachment.cgi?id=50753&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few more comments mostly nits. Almost done....


> Index: WebCore/ChangeLog

> +	   but not optimized in any way. Subesquent check-ins will be necessary
to finetune
s/finetune/fine tune/


> +>>>>>>> .r56022
This shouldn't be here.


> Index: WebCore/platform/graphics/skia/GraphicsLayerSkia.cpp
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + * Copyright (C) 2009 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

I don't think this license text is the text from GraphicsLayerCACF.cpp,
since you borrowed code heavily from that file, it seems that you should retain
the original license.


> +/** FIXME
> + * This file borrows code heavily from
platform/graphics/win/GraphicsLayerCACF.cpp
> + * (and hence it includes both copyrights)
> + * Ideally the common code (e.g. the code that keeps track of the layer
hierarchy)
> + * should be kept separate and shared between platforms. It would be a well
worthwhile
> + * effort once the Windows implementation (binaries and headers) of
CoreAnimation is
> + * checked in to the WebKit repository. Until then only Apple can make this
happen.

Hmmm... I don't see what this comment accomplishes. Thus, I wonder why it is
here.

Also, I really don't understand the comment. Right now, you have a bunch of
code in here that is basically
the same as that in GraphicsLayerCACF.cpp (which is checked in), so why can't
the code be consolidated by you?


> Index: WebCore/platform/graphics/skia/GraphicsLayerSkia.h
> +// FIXME: File heavily borrows from GraphicsLayerCACF.h.  See comment
> +// at the top of GraphicsLayerSkia.cpp.

I don't think this comment is necessary. (If someone really wants to understand
this code,
they'll likely look back at its history and see the other files checked in at
the same time
and can find the comment in that other file.)

> +    virtual void addChildAbove(GraphicsLayer* layer, GraphicsLayer
*sibling);

"layer" as a parameter name adds no information, so please remove it.
"GraphicsLayer *sibling" the * is in the wrong place.

See the next two methods as well (for "layer" and * placement).

> +    virtual void addChildBelow(GraphicsLayer* layer, GraphicsLayer
*sibling);
> +    virtual bool replaceChild(GraphicsLayer* oldChild, GraphicsLayer
*newChild);


> +    virtual void setPosition(const FloatPoint& inPoint);
> +    virtual void setAnchorPoint(const FloatPoint3D& inPoint);
> +    virtual void setSize(const FloatSize& inSize);

I don't think any of these parameter names are adding information "inPoint",
"inSize" so they should be removed.

> +    virtual void setOpacity(float opacity);

No need for the parameter name "opacity" asi ti adds no information. Since the
function name is "setOpacity" and the method only takes one parameter, the
param must be the opacity.

> Index: WebCore/platform/graphics/skia/LayerRendererSkia.cpp

> +void LayerRendererSkia::drawLayerInCanvasRecursive(skia::PlatformCanvas*
canvas, LayerSkia* layer, float opacity)
> +{
...
> +    // The position we get is for the center of the layer but
> +    // drawBitmap starts at the upper-left corner and therefore
> +    // we need to adjust our transform.

There should be commas before "but" and "and".



> Index: WebCore/platform/graphics/skia/LayerRendererSkia.h

> +
> +private:
> +    void drawLayerInCanvasRecursive(skia::PlatformCanvas*, LayerSkia*, float
opacity);
> +    void updateLayerContentsRecursive(LayerSkia* layer);

Remove param name "layer".


> Index: WebCore/platform/graphics/skia/LayerSkia.cpp

> +    // The backing store a layer does not have to be the same size as the
layer's bounds.

I can't parse this sentence.

> +void LayerSkia::updateContents()
> +{
> +    RenderLayerBacking* backing =
static_cast<RenderLayerBacking*>(m_owner->client());
> +
> +    if (backing && !backing->paintingGoesToWindow())
> +	   m_owner->paintGraphicsLayerContents(*m_graphicsContext, IntRect(0,
0, m_bounds.width(), m_bounds.height()));
> +
> +    // Paint the debug border.
> +    // FIXME: Add a flag to check if debug borders are used.
> +    m_graphicsContext->setStrokeColor(m_borderColor, DeviceColorSpace);
> +    m_graphicsContext->setStrokeThickness(m_borderWidth);
> +    m_graphicsContext->drawLine(IntPoint(0, 0), IntPoint(m_bounds.width(),
0));
> +    m_graphicsContext->drawLine(IntPoint(0, 0), IntPoint(0,
m_bounds.height()));
> +    m_graphicsContext->drawLine(IntPoint(m_bounds.width(), 0),
IntPoint(m_bounds.width(), m_bounds.height()));
> +    m_graphicsContext->drawLine(IntPoint(0, m_bounds.height()),
IntPoint(m_bounds.width(), m_bounds.height()));
> +}
> +
> +

extra blank line here.

> +void LayerSkia::setNeedsDisplay()
> +{
> +    // FIXME: implement
> +}
> +
> +

Extra blank line here.

> +}
> +
> +#endif // USE(ACCELERATED_COMPOSITING)

> Index: WebCore/platform/graphics/skia/LayerSkia.h
> +    void addSublayer(PassRefPtr<LayerSkia> sublayer);

No need to say "sublayer" here since it is obvious based on the method name.

> +    SkMatrix sublayerTransform() const { return m_sublayerTransform; }

Why isn't the return value "const SkMatrix&"?

> +    void setBackingStoreRect(const SkIRect& rect);

No need for the param name "rect" here.

> +    void updateGraphicsContext(const SkIRect& rect);

No need for the param name "rect" here.

> Index: WebKit/chromium/ChangeLog
> +	   * src/WebViewImpl.cpp:
> +	   (WebKit::WebViewImpl::WebViewImpl):
> +	   (WebKit::WebViewImpl::paint):
> +	   Modified method to handle the accelerated compositing path. Now,
when doing accelerated compositing,
> +	   paint() results to repainting the contents of the root layer and
then doing a composite operation.

s/to/in/

> Index: WebKit/chromium/src/WebViewImpl.cpp
> +
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!isAcceleratedCompositing()) {
> +#endif
> +	   WebFrameImpl* webframe = mainFrameImpl();
> +	   if (webframe)
> +	       webframe->paint(canvas, rect);
> +#if USE(ACCELERATED_COMPOSITING)
> +    } else {
> +	   // Draw the contents of the root layer

Please add a period.


> +#if USE(ACCELERATED_COMPOSITING)
> +void WebViewImpl::setRootGraphicsLayer(WebCore::PlatformLayer* layer)
> +{
> +    setAcceleratedCompositing(layer ? true : false);

Everytime I see this, I think of 
  bool b = value;
  func(b ? true : false);

Of course, it isn't in the same league as some code that I've seen that does if
(b == true) (where b is a bool already).
I understand why you like it though so you can leave it as is.


> +void WebViewImpl::updateRootLayerContents(const WebRect& rect)
> +{
...
> +	   // Set the backing store size used by the root layer to be the size
of the visible
> +	   // area. Note that the root layer bounds could be larger than the
backing store size

There should be a comma after size.


> Index: WebKit/chromium/src/WebViewImpl.h
> @@ -43,9 +43,16 @@
>  #include "ContextMenuClientImpl.h"
>  #include "DragClientImpl.h"
>  #include "EditorClientImpl.h"
> +#include "GraphicsLayer.h"
>  #include "InspectorClientImpl.h"
>  #include "NotificationPresenterImpl.h"
>  
> +#if USE(ACCELERATED_COMPOSITING)
> +#if WEBKIT_USING_SKIA

Typically headers have the if's inside of them, so you should be able to just
#include the header and have no if's.

> +#include "LayerRendererSkia.h"
> +#endif
> +#endif