Bug 35557 – [chromium] Implement composited graphics layers with Skia

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Vangelis Kokkevis]

Provide a software implementation of the ACCELERATED_COMPOSITING layer rendering path in WebKit via Skia. While it is unlikely that this path will be enabled for anything other than testing (or a fallback in case of missing h/w support), it will serve as a stepping stone towards a cross-platform GPU-based compositor for Chromium.

[Darin Fisher (:fishd, Google)]

What is the plan for Chromium Mac s/w fallback (where we do not use Skia presently)?

[Vangelis Kokkevis]

We should be able to do something similar using CG, assuming we thing it's worth the effort.

[Vangelis Kokkevis]

Created attachment 49851 [details]
Proposed patch

[Vangelis Kokkevis]

Created attachment 49861 [details]
Proposed patch with style issues fixed

[WebKit Review Bot]

Attachment 49861 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/skia/LayerSkia.h:76:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:79:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:82:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:94:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:97:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:100:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:103:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:109:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:123:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:126:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:129:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:133:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:154:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.cpp:259:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 14 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vangelis Kokkevis]

A quick comment on the remaining issues caught by the webkit style checker:

The "More than one command on the same line" warning comes from inlined methods defined in the header file like this one:

class LayerSkia {
  ...
    void setHidden(bool hidden) { m_hidden = hidden; setNeedsCommit(); }
  ...
};


I could split them into one command per line but that would bloat the header file length:

class LayerSkia {
    ...
    void setHidden(bool hidden)
    {
        m_hidden = hidden;
        setNeedsCommit();
    }
    ...
}


The "One line control clauses should not use braces" comes from a for() loop with an empty body:

for( ... ) {} 

Which could be replaced by:

for( ... );

although I do find this more confusing and error prone.

[Shinichiro Hamaji]

> The "One line control clauses should not use braces" comes from a for() loop
> with an empty body:
> 
> for( ... ) {} 

Yeah, this is actually a false alarm. I filed a bug and submitted a patch: Bug 35717

[Shinichiro Hamaji]

Comment on attachment 49861 [details]
Proposed patch with style issues fixed

I think I cannot be the main reviewer of this change, but I'd like to give some nitpicks. It's OK to ignore some of them, but you may want to consider some others.

> +/** TODO(Vangelis)

I'm not sure, but I think this is WebKit-styled code and we should use FIXME instead of TODO.

> +namespace WebCore {
> +
> +

An unnecessary blank line?

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

Looks weird. Maybe we should remove "posPoint.fY = " ?

> +    // if we've changed the bounds, we need to recalculate the position
> +    // of the layer, taking anchor point into account.

s/if/If/ ?

> +    rect.set(0,
> +             0,
> +             m_contentsRect.width(),
> +             m_contentsRect.height());

Should be in a single line? I know the original code has the same issue though...

> +        if (defaultContentsOrientation() == CompositingCoordinatesBottomUp) {
> +            SkPoint anchorPoint;
> +            anchorPoint.set(0.0f, 1.0f);
> +            m_contentsLayer->setAnchorPoint(anchorPoint);

It seems the corresponding code in GraphicsLayerCACF is flipping Y-coordinate. Don't we need this operation?

> +    for (size_t ii = 0; ii < sublayers.size(); ii++)
> +        drawLayerInCanvasRecursive(canvas, sublayers[ii].get(), opacity);

Are there any reason we prefer "size_t ii" to "size_t i" ?

> +    SkMatrix transform = layer->transform();

It seems SkMatrix isn't the tiniest object. I guess it's better to use const reference here and elsewhere, but I'm not sure.

> +    SkPaint opacityPaint;
> +    opacity = opacity * layer->opacity();

opacity *= layer->opacity() ?

> +#include "LayerSkia.h"
> +#include <wtf/Noncopyable.h>
> +
> +#include <wtf/PassOwnPtr.h>
> +#include <wtf/Vector.h>

An unnecessary blank line between wtf headers.

> Index: WebCore/platform/graphics/skia/LayerSkia.cpp
> ===================================================================
> --- WebCore/platform/graphics/skia/LayerSkia.cpp	(revision 0)
> +++ WebCore/platform/graphics/skia/LayerSkia.cpp	(revision 0)
> @@ -0,0 +1,296 @@
> +/*
> + * Copyright (c) 2010, Google Inc. All rights reserved.
... snipped ...
> + */
> +
> +
> +#include "config.h"

An unnecessary blank line before config.h .

> +using namespace std;
> +
> +

An unnecessary blank line.

> +    delete m_graphicsContext;
> +    delete m_skiaContext;
> +    delete m_canvas;

Cannot we use OwnPtr ?

> +LayerSkia* LayerSkia::ancestorOrSelfWithSuperlayer(LayerSkia* superlayer) const
> +{
> +    LayerSkia* layer = const_cast<LayerSkia*>(this);

The use of const_cast is exceptional. I think it's better to add a comment which describes why we need this. I'm not sure, cannot we remove this const_cast easily in this case?

> +void LayerSkia::setBackingStoreRect(const SkIRect& rect)
> +{
> +    if (m_backingStoreRect == rect)
> +        return;
> +
> +    updateGraphicsContext(rect);
> +}
> +
> +

An unnecessary blank line.

> +void LayerSkia::setFrame(const SkRect& rect)
> +{
> +    m_frame = rect;
> +    setNeedsCommit();
> +}
> +
> +

Ditto.

> +LayerSkia* LayerSkia::rootLayer() const
> +{
> +    LayerSkia* layer = const_cast<LayerSkia*>(this);

const_cast again.

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

An unnecessary blank line.

> +    WebFrameImpl* webframe = mainFrameImpl();
> +    if (webframe) {
> +        FrameView* view = webframe->frameView();
> +        if (view) {

I think WebKit tends to like the following idiom:

    if (WebFrameImpl* webframe = mainFrameImpl()) {

> +            LayerSkia* rootLayer = m_layerRenderer->rootLayer();
> +            if (rootLayer) {

Ditto.

> +    WebFrameImpl* webframe = mainFrameImpl();
> +    if (webframe) {
> +        FrameView* view = webframe->frameView();
> +        if (view) {

Ditto.

> +    bool isAcceleratedCompositing() { return m_isAcceleratedCompositing; }

I think this can be a const method.

[Vangelis Kokkevis]

(In reply to comment #8)
> (From update of attachment 49861 [details])
> I think I cannot be the main reviewer of this change, but I'd like to give some
> nitpicks. It's OK to ignore some of them, but you may want to consider some
> others.

Thanks for the review!

> 
> > +/** TODO(Vangelis)
> 
> I'm not sure, but I think this is WebKit-styled code and we should use FIXME
> instead of TODO.
> 
Changed all the TODO's to FIXME's
Done.
> > +namespace WebCore {
> > +
> > +
> 
> An unnecessary blank line?

Done.

> 
> > +    posPoint.set(m_position.x() + m_anchorPoint.x() * m_size.width(), m_position.y() + m_anchorPoint.y() * m_size.height());
> 
> Looks weird. Maybe we should remove "posPoint.fY = " ?
> 

Done.

> > +    // if we've changed the bounds, we need to recalculate the position
> > +    // of the layer, taking anchor point into account.
> 
> s/if/If/ ?
> 

Done.

> > +    rect.set(0,
> > +             0,
> > +             m_contentsRect.width(),
> > +             m_contentsRect.height());
> 
> Should be in a single line? I know the original code has the same issue
> though...
> 

Done.

> > +        if (defaultContentsOrientation() == CompositingCoordinatesBottomUp) {
> > +            SkPoint anchorPoint;
> > +            anchorPoint.set(0.0f, 1.0f);
> > +            m_contentsLayer->setAnchorPoint(anchorPoint);
> 
> It seems the corresponding code in GraphicsLayerCACF is flipping Y-coordinate.
> Don't we need this operation?
> 

Done.

> > +    for (size_t ii = 0; ii < sublayers.size(); ii++)
> > +        drawLayerInCanvasRecursive(canvas, sublayers[ii].get(), opacity);
> 
> Are there any reason we prefer "size_t ii" to "size_t i" ?
> 

Done.

> > +    SkMatrix transform = layer->transform();
> 
> It seems SkMatrix isn't the tiniest object. I guess it's better to use const
> reference here and elsewhere, but I'm not sure.
> 

Done.

> > +    SkPaint opacityPaint;
> > +    opacity = opacity * layer->opacity();
> 
> opacity *= layer->opacity() ?
> 

Done.

> > +#include "LayerSkia.h"
> > +#include <wtf/Noncopyable.h>
> > +
> > +#include <wtf/PassOwnPtr.h>
> > +#include <wtf/Vector.h>
> 
> An unnecessary blank line between wtf headers.
> 

Done.

> > Index: WebCore/platform/graphics/skia/LayerSkia.cpp
> > ===================================================================
> > --- WebCore/platform/graphics/skia/LayerSkia.cpp	(revision 0)
> > +++ WebCore/platform/graphics/skia/LayerSkia.cpp	(revision 0)
> > @@ -0,0 +1,296 @@
> > +/*
> > + * Copyright (c) 2010, Google Inc. All rights reserved.
> ... snipped ...
> > + */
> > +
> > +
> > +#include "config.h"
> 
> An unnecessary blank line before config.h .
> 

Done.

> > +using namespace std;
> > +
> > +
> 
> An unnecessary blank line.

Done.

> 
> > +    delete m_graphicsContext;
> > +    delete m_skiaContext;
> > +    delete m_canvas;
> 
> Cannot we use OwnPtr ?
> 
Good suggestion.  It cleaned up the code quite a bit!
Done.

> > +LayerSkia* LayerSkia::ancestorOrSelfWithSuperlayer(LayerSkia* superlayer) const
> > +{
> > +    LayerSkia* layer = const_cast<LayerSkia*>(this);
> 
> The use of const_cast is exceptional. I think it's better to add a comment
> which describes why we need this. I'm not sure, cannot we remove this
> const_cast easily in this case?
> 
Removed the method altogether as it wasn't being used.
Done.

> > +void LayerSkia::setBackingStoreRect(const SkIRect& rect)
> > +{
> > +    if (m_backingStoreRect == rect)
> > +        return;
> > +
> > +    updateGraphicsContext(rect);
> > +}
> > +
> > +
> 
> An unnecessary blank line.
> 

Done.

> > +void LayerSkia::setFrame(const SkRect& rect)
> > +{
> > +    m_frame = rect;
> > +    setNeedsCommit();
> > +}
> > +
> > +
> 
> Ditto.
> 

Done.

> > +LayerSkia* LayerSkia::rootLayer() const
> > +{
> > +    LayerSkia* layer = const_cast<LayerSkia*>(this);
> 
> const_cast again.
> 
Method now returns a const LayerSkia* and there's no need for casting the const away.
Done.

> > +void LayerSkia::setNeedsDisplay()
> > +{
> > +    // TODO(vangelis): implement
> > +}
> > +
> > +
> 
> An unnecessary blank line.
> 

Done.

> > +    WebFrameImpl* webframe = mainFrameImpl();
> > +    if (webframe) {
> > +        FrameView* view = webframe->frameView();
> > +        if (view) {
> 
> I think WebKit tends to like the following idiom:
> 
>     if (WebFrameImpl* webframe = mainFrameImpl()) {
> 

Done.

> > +            LayerSkia* rootLayer = m_layerRenderer->rootLayer();
> > +            if (rootLayer) {
> 
> Ditto.
> 

Done.

> > +    WebFrameImpl* webframe = mainFrameImpl();
> > +    if (webframe) {
> > +        FrameView* view = webframe->frameView();
> > +        if (view) {
> 
> Ditto.
> 

Done.

> > +    bool isAcceleratedCompositing() { return m_isAcceleratedCompositing; }
> 
> I think this can be a const method.


Done.

[Vangelis Kokkevis]

Created attachment 50059 [details]
Fixed previous patch per reviewer's comments

[WebKit Review Bot]

Attachment 50059 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/skia/LayerSkia.h:75:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:78:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:81:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:93:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:96:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:99:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:102:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:108:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:122:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:125:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:128:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:132:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:153:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.cpp:236:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 14 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 50059 [details]
Fixed previous patch per reviewer's comments

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
> [phone]  Vangelis Kokkevis  <[e-mail]>
> +
> +        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.

[David Levin]

I made a mistake in my comments (when I said to use TODO). Use FIXME instead of TODO.

However, as I tried to mention, there should not be a name/email with the comment, so "// FIXME(vangelis):" should never appear in the patch. (The commit log will show who added the comment.)

[Vangelis Kokkevis]

Thanks very much for the review, David!  I think I've addressed all your comments.

(In reply to comment #12)
> (From update of attachment 50059 [details])
> 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
> > [phone]  Vangelis Kokkevis  <[e-mail]>
> > +
> > +        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.

Done

> 
> > +        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.")
> 

Done

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

Done

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

Done.

> > 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.)

Done.

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

Replaced all FIXME(vangelis) with FIXME.
Done.

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

I have no way of building Apple's code as they haven't yet checked in the binaries/headers for their windows implementation of Core Animation. I adjusted the comment accordingly.

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

Done.

> 
> 
> > +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);"

Done.

> 
> > +GraphicsLayerSkia::~GraphicsLayerSkia()
> > +{
> > +    // clean up the Skia layer
> "// Clean up Skia layer."
> 
> Add a capital letter and a period.

Done.

> 
> > +void GraphicsLayerSkia::setTransform(const TransformationMatrix& t)
> 
> Use transform instead of "t". (Avoid abbreviations in WK code.)

Done.

> 
> > +void GraphicsLayerSkia::setChildrenTransform(const TransformationMatrix& t)
> 
> Use childrenTransform instead of "t". (Avoid abbreviations in WK code.)

Done.

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

FIXME(vangelis) -> FIXME
Done.

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

I guess it could be overwritten by a derived class but since that's not expected I removed it altogether.
Done.

> 
> > +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)?

Done.

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

Done.

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

Done.

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

Done.

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

Done.

> 
> > +#endif // GraphicsLayerSkia_h_
> 
> This doesn't match the define above (and you don't even need to do this comment
> anyway).
> 

Done.

> > Index: WebCore/platform/graphics/skia/LayerRendererSkia.cpp
> > +/*
> > + * Copyright (c) 2010, Google Inc. All rights reserved.
> 
> Same comment as before (and all other copyright headers).

Done.

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

Done.

> > +    void setRootLayer(LayerSkia* layer) { m_rootLayer = layer; }
> 
> LayerSkia* should be a PassRefPtr<LayerSkia>
> 

Done.

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

Done.

> > +    void updateLayerContentsRecursive(LayerSkia* layer);
> 
> No need for "layer".
> 
> > +#endif // WKCACFLayer_h
> 
> I don't think this comment is correct.
> 

Done.

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

Done.

> 
> > +    // Backing store a layer could be different than then layer's bounds.
> 
> I don't understand this sentence.
> 

Me neither :)  Fixed.
Done.

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

The comment actually explains the sequence of events that result in a frame
getting rendered so it's kind of useful. But you are right about the CACFRenderer bit which I changed it over to say LayerRendererSkia.

Done.

> 
> > +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);
> 
> > +}

Actually this entire method wasn't getting used at all so I removed it... It was one of the ones that moved over from Apple's port.  Sorry you had to waste time on it.


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

You are right. 
Done.

> 
> > +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?
> 

Good point!
Done.

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

Done.

> > +    void removeSublayer(LayerSkia*);
> > +
> > +    // Re-created the canvas and graphics context.  This method
> 
> One space after periods in comments.
> 

Done.

> 
> > +    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?

Re-arranged them a bit to separate multi-byte entities from smaller ones.
Done.

> 
> > +#endif // WKCACFLayer_h
> Incorrect comment.
> 

Done.

> > 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).

Done.

> 
> > +    // 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?

Turns out that these includes aren't really needed in the .cpp file anyway so I removed them.
Done.

> 
> > +#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);
> ?

Maybe personal preference but I'd rather pass a bool when a bool is expected.  Does it look too strange to you? 

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

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

Done.

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

Done.
> 
> > +                // 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.)
> 
Done.
> 
> > 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.

I removed the unnecessary includes from the .cpp file
Done.

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

It seems to be used as such in a whole bunch of files.  I don't believe there's a macro for it. 

> 
> > +#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?

Removed "skia/".
Done

> 
> 
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    void setRootLayerNeedsDisplay();
> > +    void setRootGraphicsLayer(WebCore::PlatformLayer* layer);
> 
> No need for "layer" here.
> 

Done.

> 
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    void setAcceleratedCompositing(bool);
> > +    bool isAcceleratedCompositing() const { return m_isAcceleratedCompositing; }
> > +    void updateRootLayerContents(const WebRect& rect);
> 
> No need for "rect" here.

Done.

[Vangelis Kokkevis]

Created attachment 50753 [details]
Patch addressing review comments

[David Levin]

Comment on attachment 50753 [details]
Patch addressing review comments

Adding the r? which I believe was left off by mistake.

[WebKit Review Bot]

Attachment 50753 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/skia/LayerSkia.h:74:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:77:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:80:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:83:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:86:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:92:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:95:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:98:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:101:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:107:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:121:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:124:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:127:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:131:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:152:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 15 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 50753 [details]
Patch addressing review comments

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

[Vangelis Kokkevis]

(In reply to comment #18)
> (From update of attachment 50753 [details])
> 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/
> 
Done.
> 
> > +>>>>>>> .r56022
> This shouldn't be here.
> 
Done.
> 
> > 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.

Actually our license file is a strict superset of Apple's (Apple's doesn't have the third item) so I'm thinking it works better in this case.

> 
> 
> > +/** 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?
> 

The issue is the following: GraphicsLayerCACF.cpp requires headers (and libaries) that haven't been checked in to the public WebKit tree so it's impossible for me to build it. If I were to refactor code to minimize duplication I would have no way of testing the CACF side.

> 
> > 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.)
> 

Removed the comment.  Also removed the Apple (C) since the file doesn't really contain any source code.

> > +    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);
> 

Done.

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

Done.

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

Done.

> > 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".

Done.

> 
> 
> 
> > Index: WebCore/platform/graphics/skia/LayerRendererSkia.h
> 
> > +
> > +private:
> > +    void drawLayerInCanvasRecursive(skia::PlatformCanvas*, LayerSkia*, float opacity);
> > +    void updateLayerContentsRecursive(LayerSkia* layer);
> 
> Remove param name "layer".
> 

Done.

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

Any better now? 

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

Done.

> 
> > +    SkMatrix sublayerTransform() const { return m_sublayerTransform; }
> 
> Why isn't the return value "const SkMatrix&"?

Done.

> 
> > +    void setBackingStoreRect(const SkIRect& rect);
> 
> No need for the param name "rect" here.

Done.

> 
> > +    void updateGraphicsContext(const SkIRect& rect);
> 
> No need for the param name "rect" here.

Done.

> 
> > 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/

Done.

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

Done.

> 
> 
> > +#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.
> 
Done.

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

Unfortunately there's a decision to make here depending on whether we're using Skia (Win/Linux) or Cg (Mac) and whether we do software compositing or hardware. Over time this will turn into something like:

#if SOFTWARE_COMPOSITING
#if WEBKIT_USING_SKIA
#include "LayerRendererSkia.h"
#elif WEBKIT_USING_CG
#include "LayerRendererCg.h"
#else  // SOFTWARE_COMPOSITING
#include "LayerRendererGL.h"
#endif

so, while the inner #if isn't necessary for now, it will very soon. In any case, I'm fine with removing it now and putting it back in when it's necessary.

Done.

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

[Vangelis Kokkevis]

Created attachment 50866 [details]
Patch addressing more recent review comments

[WebKit Review Bot]

Attachment 50866 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/skia/LayerSkia.h:74:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:77:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:80:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:83:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:86:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:92:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:95:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:98:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:101:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:107:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:121:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:124:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:127:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:131:  More than one command on the same line  [whitespace/newline] [4]
WebCore/platform/graphics/skia/LayerSkia.h:152:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 15 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 50866 [details]
Patch addressing more recent review comments

Rejecting patch 50866 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12495 test cases.
fast/loader/api-test-new-window-data-load-base-url.html -> failed

Exiting early after 1 failures. 7692 tests run.
128.80s total testing time

7691 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/984004

[Adam Barth]

Comment on attachment 50866 [details]
Patch addressing more recent review comments

Lets try that again.

[WebKit Commit Bot]

Comment on attachment 50866 [details]
Patch addressing more recent review comments

Rejecting patch 50866 from commit-queue.

Failed to parse ChangeLog: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog

[Eric Seidel (no email)]

Hmm... I was in the ChangeLog parsing code last night.  I may have caused this regression.  I'll investigate.

[Eric Seidel (no email)]

Comment on attachment 50866 [details]
Patch addressing more recent review comments

Nope.  I didn't break anything.  Could the diff be malformed?

OR maybe svn-apply isn't handling it correctly.  Once it's applied the date line reads:
10-03-15  Vangelis Kokkevis  <[e-mail]>

which is definitely wrong (notice the missing 20 in 2010).

Dont' diffs normally have a space between + and the actual content?

[Eric Seidel (no email)]

If you believe this to be an svn-apply bug, please file.

[Chris Jerdonek]

(In reply to comment #27)
> If you believe this to be an svn-apply bug, please file.

The patch itself seems to be missing the 20, so it looks like it's applying correctly:

+
+#endif
Index: WebKit/chromium/ChangeLog
===================================================================
--- WebKit/chromium/ChangeLog	(revision 56022)
+++ WebKit/chromium/ChangeLog	(working copy)
@@ -1,3 +1,41 @@
+10-03-15  Vangelis Kokkevis  <[e-mail]>
+
+        Reviewed by NOBODY (OOPS!).
+
+        Adding support for the ACCELERATED_COMPOSITING render path to Chromium.

[Eric Seidel (no email)]

Ah, I was looking at the wrong ChangeLog entry in the patch!

[Vangelis Kokkevis]

I think my mistake was that I tried to merge the changelog manually as other check-ins took place while my CL was out for review.  Any suggestions on how to fix it?

[Eric Seidel (no email)]

resolve-ChangeLogs will take care of merging changelogs for you.

[Vangelis Kokkevis]

Created attachment 51078 [details]
Fixed changelog entries so that the patch can be applied correctly (hopefully)

Is it enough to set commit-queue to '+' to get it back there?

[Vangelis Kokkevis]

(In reply to comment #31)
> resolve-ChangeLogs will take care of merging changelogs for you.

Aha, thanks for the tip!
I just re-created the changelog files and uploaded a new CL. Not sure what else needs to happen to get it landed.

[Adam Barth]

Comment on attachment 51078 [details]
Fixed changelog entries so that the patch can be applied correctly (hopefully)

You can't land something with OOPS in it, but if there's a reviewer, the bot will fill in their name (in this case, me).

[WebKit Commit Bot]

Comment on attachment 51078 [details]
Fixed changelog entries so that the patch can be applied correctly (hopefully)

Rejecting patch 51078 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[Adam Barth]

Comment on attachment 51078 [details]
Fixed changelog entries so that the patch can be applied correctly (hopefully)

You also need to be a committer to commit code.  :)

[WebKit Commit Bot]

Comment on attachment 51078 [details]
Fixed changelog entries so that the patch can be applied correctly (hopefully)

Clearing flags on attachment: 51078

Committed r56212: <http://trac.webkit.org/changeset/56212>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

(In reply to comment #22)
> (From update of attachment 50866 [details])
> Rejecting patch 50866 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--exit-after-n-failures=1', '--quiet']" exit_code: 1
> Running build-dumprendertree
> Compiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 12495 test cases.
> fast/loader/api-test-new-window-data-load-base-url.html -> failed

This was bug 35594.

[Eric Seidel (no email)]

(In reply to comment #22)
> (From update of attachment 50866 [details])
> Rejecting patch 50866 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--exit-after-n-failures=1', '--quiet']" exit_code: 1
> Running build-dumprendertree
> Compiling Java tests
> make: Nothing to be done for `default'.
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 12495 test cases.
> fast/loader/api-test-new-window-data-load-base-url.html -> failed

This was bug 35594.