[webkit-reviews] review denied: [Bug 35557] Implement composited graphics layers with Skia : [Attachment 50059] Fixed previous patch per reviewer's comments

[bugzilla-daemon at webkit.org]

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

Attachment 50059: Fixed previous patch per reviewer's comments
https://bugs.webkit.org/attachment.cgi?id=50059&action=review

------- Additional Comments from David Levin <levin at chromium.org>
In general this looks pretty good. I guess this is one of your initial patches
because a lot of my comments are style related (though some have a little more
substance).

If you stick to addressing the comments (everywhere that they apply --
sometimes I only noted the first instance of an issue) and not adding new
functionality the next review round should go pretty fast.

Also, if you don't understnad any of my comment or are unsure how to apply them
in the future, please let me know.

Thanks!

> Index: WebCore/ChangeLog
> +2010-03-02  Vangelis Kokkevis  <vangelis at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Implement s/w composited graphics layers in Chromium using the Skia
library
s/w -> software
Please add a period at the end.

> +	   https://bugs.webkit.org/show_bug.cgi?id=35557
> +	   This is an initial step in the implementation. Layer compositing is
functioning
> +	   but not optimized in any way. Subesquent check-ins will be necessary
to finetune
> +	   it.
> +
> +	   No new tests. (OOPS!)

Either add layout tests or explain why they aren't necessary at this point.

For example, if no new functionality is exposed to web pages/js, then no new
tests are possible. (which would result in adding something like this to the
ChangeLog "No new exposed functionality so no new tests.")

> +
> +	   * WebCore.gypi:
> +	     Added new source files to the chromium build
> +	   * platform/graphics/GraphicsLayer.h:
> +	     Added necessary typedef's and forward declarations for CHROMIUM
s/CHROMIUM/Chromium./

> +	     Declaration and implementation of the Skia-based s/w compositor.
s/w -> software


> Index: WebCore/platform/graphics/skia/GraphicsLayerSkia.cpp
> +/*
> + * Copyright (c) 2010, Google Inc. All rights reserved.

Ideally a capital C and no comma after the year. (The same comment for all
copyright headers.)

> + * Copyright (C) 2009 Apple Inc. All rights reserved.

You kept the Apply copyright line but changed the license text. If you're
copying that other code (which I can see is the case), it seems most
appropriate to keep the original license text from the other file.

> +/** FIXME(vangelis)

s/FIXME(vangelis)/TODO/

> + * 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.

Why not do this? (If you don't do it, it will likely not happen.) Of course, it
may not make sense for various reasons as well, but this comment implies that
it does make sense.

> +using namespace std;
> +
> +namespace WebCore {
> +
> +static void setLayerBorderColor(LayerSkia* layer, const Color& color)

Since layer cannot be 0, why not make this take a "LayerSkia&"? (Ditto for the
other functions below.)


> +static void clearBorderColor(LayerSkia* layer)
> +{
> +    layer->setBorderColor(Color(0, 0, 0, 0));
> +}

> +static void clearLayerBackgroundColor(LayerSkia* layer)
> +{
> +    layer->setBackgroundColor(0);

For some reason the asymmetry of this (when I compare it to the
clearBorderColor method) bothers me. Is it possible to make clearBorderColor
just do "layer->setBorderColor(0);"

> +GraphicsLayerSkia::~GraphicsLayerSkia()
> +{
> +    // clean up the Skia layer
"// Clean up Skia layer."

Add a capital letter and a period.

> +void GraphicsLayerSkia::setTransform(const TransformationMatrix& t)

Use transform instead of "t". (Avoid abbreviations in WK code.)

> +void GraphicsLayerSkia::setChildrenTransform(const TransformationMatrix& t)

Use childrenTransform instead of "t". (Avoid abbreviations in WK code.)

> +
> +void GraphicsLayerSkia::setContentsToImage(Image* image)
> +{
> +    // FIXME(vangelis): Implement

s/FIXME(vangelis)/TODO/ everywhere

> +GraphicsLayer::CompositingCoordinatesOrientation
GraphicsLayerSkia::defaultContentsOrientation() const
> +{
> +    return CompositingCoordinatesTopDown;
> +}

What is the point of this method since it always returns the same value (and
isn't virtual). Other code that branches based on the return value will only go
down on path.

> +void GraphicsLayerSkia::updateSublayerList()
> +{
...
> +	   newSublayers.append(m_layer.get());
> +    } else if (m_contentsLayer) {
> +	   // FIXME: add the contents layer in the correct order with negative
z-order children.

s/FIXME/TODO/


> +void GraphicsLayerSkia::updateLayerPosition()
> +{
> +    // Position is offset on the layer by the layer anchor point.
> +    SkPoint posPoint;

Why not just call this layerPosition (instead of using an abbreviation "pos"
which is discouraged in WK code)?

> +    posPoint.set(m_position.x() + m_anchorPoint.x() * m_size.width(),
> +		    m_position.y() + m_anchorPoint.y() * m_size.height());
> +
> +    primaryLayer()->setPosition(posPoint);
> +}




> Index: WebCore/platform/graphics/skia/GraphicsLayerSkia.h
> +/*
> + * Copyright (c) 2010, Google Inc. All rights reserved.
> + * Copyright (C) 2009 Apple Inc. All rights reserved.

Same comments as before.

> +#include "GraphicsContext.h"
			       ^^^^ odd character here.

> +#include "GraphicsLayer.h"
> +
> +namespace WebCore {
> +
> +class LayerSkia;
> +
> +class GraphicsLayerSkia : public GraphicsLayer {
> +public:
> +
No blank line here.

> +    GraphicsLayerSkia(GraphicsLayerClient*);
> +    virtual ~GraphicsLayerSkia();
> +
> +    virtual void setName(const String& inName);

It doesn't look like "inName" adds any information. I'd suggest removing the
param name here.

> +
> +    // for hosting this GraphicsLayer in a native layer hierarchy
> +    virtual NativeLayer nativeLayer() const;
> +
> +    virtual bool setChildren(const Vector<GraphicsLayer*>&);
> +    virtual void addChild(GraphicsLayer *layer);

layer adds no information. Please remove it.

Also the * is in the wrong place. It should be next to the type instead of the
declared item.

These comments apply in other several places too in this header file.


> +#endif // GraphicsLayerSkia_h_

This doesn't match the define above (and you don't even need to do this comment
anyway).

> Index: WebCore/platform/graphics/skia/LayerRendererSkia.cpp
> +/*
> + * Copyright (c) 2010, Google Inc. All rights reserved.

Same comment as before (and all other copyright headers).

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

> +namespace WebCore {
> +
> +class LayerRendererSkia : public Noncopyable {
> +public:
> +    static PassOwnPtr<LayerRendererSkia> create();
> +
> +    LayerRendererSkia();
> +    ~LayerRendererSkia();
> +
> +    void drawLayersInCanvas(skia::PlatformCanvas* canvas, SkRect clipRect);

No need for "canvas" param name here.

> +    void setRootLayer(LayerSkia* layer) { m_rootLayer = layer; }

LayerSkia* should be a PassRefPtr<LayerSkia>

> +    void setScrollFrame(SkIRect& scrollFrame) { m_scrollFrame = scrollFrame;
}
> +
> +private:
> +    void drawLayerInCanvasRecursive(skia::PlatformCanvas* canvas, LayerSkia*
layer, float opacity);

No need for "canvas" or "layer".

> +    void updateLayerContentsRecursive(LayerSkia* layer);

No need for "layer".

> +#endif // WKCACFLayer_h

I don't think this comment is correct.

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


> +void LayerSkia::updateGraphicsContext(const SkIRect& rect)
> +{
> +    // Create new canvas and context. OwnPtr takes care of freeing up
> +    // the old ones.
> +    m_canvas = new skia::PlatformCanvas(rect.width(), rect.height(), false);

> +    m_skiaContext = new PlatformContextSkia(m_canvas.get());
> +
> +    // This is needed to get text to show up correctly.  Without it,

One space after "." in comments.

> +    // Backing store a layer could be different than then layer's bounds.

I don't understand this sentence.


> +void LayerSkia::setNeedsCommit()
> +{
> +    // Call notifySyncRequired(), which in this implementation plumbs
through to
> +    // call setRootLayerNeedsDisplay() on the WebView, which causes the
CACFRenderer

Does your implementation use CACFRenderer? Also this comment seems a bit
redundant with the code.
It sounds like it could be simplified to be "Make CACFRenderer render a frame."


> +void LayerSkia::replaceSublayer(LayerSkia* reference, PassRefPtr<LayerSkia>
newLayer)
> +{
> +    ASSERT_ARG(reference, reference);
> +    ASSERT_ARG(reference, reference->superlayer() == this);
> +
> +    if (reference == newLayer)
> +	   return;
> +
> +    if (!newLayer) {
> +	   removeSublayer(reference);
> +	   return;
> +    }
> +
> +    newLayer->removeFromSuperlayer();
> +
> +    int referenceIndex = indexOfSublayer(reference);
> +    ASSERT(referenceIndex != -1);
> +    if (referenceIndex == -1)
> +	   return;

Why assert and check in an if. Either if it possible or it shouldn't happen
ever. Which one?
Also, why doesn't this insert the newLayer just because the reference wasn't a
sublayer?

> +
> +    reference->removeFromSuperlayer();

Why call "reference->removeFromSuperlayer();" here but
"removeSublayer(reference);" above? (They look pretty equivalent anyway. If you
change these to be the same, I think the code can be condensed slightly by
having only one call to this function.)

> +    insertSublayer(newLayer, referenceIndex);

> +}
> +
> +void LayerSkia::removeFromSuperlayer()
> +{
> +    LayerSkia* superlayer = this->superlayer();
> +    if (!superlayer)
> +	   return;
> +
> +    superlayer->removeSublayer(this);
> +    superlayer->setNeedsCommit();

Why does this call setNeedsCommit on the superlayer? It seems like
removeSublayer should do this if it is needed (just like
LayerSkia::removeSublayer does below).

> +void LayerSkia::setBounds(const SkIRect& rect)
> +{
> +    if (rect == m_bounds)
> +	   return;
> +
> +    m_bounds = rect;
> +
> +    // Re-create the canvas and associated contexts.
> +    updateGraphicsContext(m_bounds);
> +
> +    setNeedsCommit();
> +}
> +
> +void LayerSkia::setFrame(const SkRect& rect)
> +{

Why doesn't this check to see if the frame is already rect and just return like
what happens in so many of these other methods?

> +    m_frame = rect;
> +    setNeedsCommit();
> +}
> +


> Index: WebCore/platform/graphics/skia/LayerSkia.h
> +    // Returns the index of the passed layer in this layer's sublayers list
> +    // or -1 if not found
Add a period. Also the fact that it returns the index of the passed in layer is
indicated
by the name. The comment at most needs to mention the -1. something like
"If the sublayer isn't found, returns -1."
> +    int indexOfSublayer(const LayerSkia*);
> +
> +    // This should only be called from removeFromSuperlayer.

In general these comments can go out of date so no need to put them in there.
Better to put in comments about when it should be called (if any comment is
needed).

> +    void removeSublayer(LayerSkia*);
> +
> +    // Re-created the canvas and graphics context.  This method

One space after periods in comments.


> +    Vector<RefPtr<LayerSkia> > m_sublayers;
> +    LayerSkia* m_superlayer;
> +
> +    bool m_needsDisplayOnBoundsChange;
> +    GraphicsLayerSkia* m_owner;
> +
> +    OwnPtr<skia::PlatformCanvas> m_canvas;
> +    OwnPtr<PlatformContextSkia> m_skiaContext;
> +    OwnPtr<GraphicsContext> m_graphicsContext;
> +    Color m_borderColor;
> +    float m_borderWidth;
> +
> +    LayerType m_layerType;
> +
> +    SkIRect m_bounds;
> +    SkIRect m_backingStoreRect;
> +    SkPoint m_position;
> +    SkPoint m_anchorPoint;
> +    float m_anchorPointZ;
> +    Color m_backgroundColor;
> +    bool m_clearsContext;
> +    bool m_doubleSided;
> +    uint32_t m_edgeAntialiasingMask;
> +    SkRect m_frame;
> +    bool m_hidden;
> +    bool m_masksToBounds;
> +    ContentsGravityType m_contentsGravity;
> +    float m_opacity;
> +    bool m_opaque;
> +    float m_zPosition;
> +    bool m_geometryFlipped;
> +    String m_name;
> +
> +    SkMatrix m_transform;
> +    SkMatrix m_sublayerTransform;

Have you considered arranging these variables to minimize padding in the
structure?

> +#endif // WKCACFLayer_h
Incorrect comment.

> Index: WebKit/chromium/ChangeLog

> Index: WebKit/chromium/src/ChromeClientImpl.h

> +#if USE(ACCELERATED_COMPOSITING)
> +    // Pass 0 as the GraphicsLayer to detatch the root layer.
> +    virtual void attachRootGraphicsLayer(WebCore::Frame*,
WebCore::GraphicsLayer*);

It would be nice to put a blank line here (since ou have little comments for
each method, the 
blank lines will allow flks to find the next function quicker).

> +    // Sets a flag to specify that the next time content is drawn to the
window,
> +    // the changes appear on the screen in synchrony with updates to
GraphicsLayers.
> +    virtual void setNeedsOneShotDrawingSynchronization() { }
> +    // Sets a flag to specify that the view needs to be updated, so we need
> +    // to do an eager layout before the drawing.
> +    virtual void scheduleCompositingLayerSync();
> +#endif

> Index: WebKit/chromium/src/WebViewImpl.cpp

> +#if USE(ACCELERATED_COMPOSITING)
> +#include "webkit/glue/webkit_glue.h"

I'm pretty sure you shouldn't be using glue in this file. Does any other file
in WebKit/chromium/src use it?

> +#if WEBKIT_USING_SKIA
> +#include "skia/LayerSkia.h"
> +#endif
> +#endif

See comments in WebKit/chromium/src/WebViewImpl.h.


>  void WebViewImpl::paint(WebCanvas* canvas, const WebRect& rect)
>  {
> +	   // Composite everything into the canvas that's passed to us.
> +	   SkIRect canvasIRect;
> +	   canvasIRect.set(rect.x, rect.y, rect.x+rect.width,
rect.y+rect.height);

Put spaces around +

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

What about this:
    setAcceleratedCompositing(layer);
?

> +    if (m_layerRenderer)
> +	   m_layerRenderer->setRootLayer(layer);
> +}
> +

> +void WebViewImpl::updateRootLayerContents(const WebRect& rect)
> +{
> +    if (!isAcceleratedCompositing())
> +	   return;
> +
> +    if (WebFrameImpl* webframe = mainFrameImpl()) {
Please consider a fail fast approach.

WebFrameImpl* webframe = mainFrameImpl();
if (!webframe)
    return;
etc.

> +	   if (FrameView* view = webframe->frameView()) {
> +	       WebRect viewRect = view->frameRect();
> +
> +	       SkIRect scrollFrame;
> +	       scrollFrame.set(view->scrollX(), view->scrollY(),
view->layoutWidth()+view->scrollX(), view->layoutHeight()+view->scrollY());

Put in spaces around +

> +	       m_layerRenderer->setScrollFrame(scrollFrame);
> +	       if (LayerSkia* rootLayer = m_layerRenderer->rootLayer()) {
> +		   SkIRect rootLayerBounds;
> +		   IntRect visibleRect = view->visibleContentRect(true);
> +
> +		   // 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

Single space after . in comments.
Also, there should be a comma after size.

> +		   // but there's no reason to waste memory by allocating
backing store larger than the
> +		   // visible portion.
> +		   rootLayerBounds.set(0, 0, visibleRect.width(),
visibleRect.height());
> +		   rootLayer->setBackingStoreRect(rootLayerBounds);
> +		   GraphicsContext* rootLayerContext =
rootLayer->graphicsContext();
> +		   rootLayerContext->save();
> +
> +		   webframe->paintWithContext(*(rootLayer->graphicsContext()),
rect);
> +		   rootLayerContext->restore();
> +	       }
> +	   }
> +    }
> +}
> +
> +void WebViewImpl::setRootLayerNeedsDisplay()
> +{
> +    // FIXME(vangelis): For now we're posting a repaint event for the entire
page
> +    //		  which is an overkill.

I wouldn't suggest trying to align this wrapping part of the comment, but it
looks like it is attempted but doesn't align. You could simply not even do a
line break here. (WK doesn't have an 80 column rule.)


> Index: WebKit/chromium/src/WebViewImpl.h
> +#if USE(ACCELERATED_COMPOSITING)
> +#if WEBKIT_USING_SKIA

These if's should be in the header, so you should be able to just #include the
header with the rest of them.

Also "#if WEBKIT_USING_SKIA" seems odd. It seems like it should be "#if
USE(SKIA)"

> +#include "skia/LayerRendererSkia.h"

WebKit code doesn't put path names in the #includes in general, so the "skia/"
looks incorrect. Do you need to add some include paths?


> +#if USE(ACCELERATED_COMPOSITING)
> +    void setRootLayerNeedsDisplay();
> +    void setRootGraphicsLayer(WebCore::PlatformLayer* layer);

No need for "layer" here.


> +#if USE(ACCELERATED_COMPOSITING)
> +    void setAcceleratedCompositing(bool);
> +    bool isAcceleratedCompositing() const { return
m_isAcceleratedCompositing; }
> +    void updateRootLayerContents(const WebRect& rect);

No need for "rect" here.