[Webkit-unassigned] [Bug 56935] [Qt] Implement accelerated compositing on WK2 Qt port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56935


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #96897|review?                     |review-
               Flag|                            |




--- Comment #14 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2011-06-13 07:44:15 PST ---
(From update of attachment 96897)
View in context: https://bugs.webkit.org/attachment.cgi?id=96897&action=review

> Source/WebKit2/ChangeLog:11
> +        Added WebGraphicsLayer, a subclass of WebCore::GraphicsLayer that serializes the
> +        state of the layer tree to the UI process WebLayerTreeInfo.
> +        For now this patch doesn't serialize the animation information, a feature that will be upstreamed
> +        later on.

You line length quite different per line :-)

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:44
> +static HashMap<WebLayerID, WebGraphicsLayer*>& layerByIDTable()

I believe we normally would call this a map and not a table

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:46
> +    static HashMap<WebLayerID, WebGraphicsLayer*> sLayerByIDTable;

I have never seen the sLayer... style, gLayer or s_layer or similar is the most used way to declare statics. But as it is internal anyway, why not just call it globalMap or so.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:85
> +    static WebLayerID sID = 1000;

Add comment why you start at 1000

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:263
> +bool WebGraphicsLayer::addAnimation(const KeyframeValueList& valueList, const IntSize& boxSize, const Animation* anim, const String& keyframesName, double timeOffset)
> +{
> +    return false;
> +}

Maybe belongs in the header?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:283
> +}
> +void WebGraphicsLayer::setMaskLayer(GraphicsLayer* layer)

some missing new line

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:288
> +    notifyChange();
> +}
> +void WebGraphicsLayer::setReplicatedByLayer(GraphicsLayer* layer)

other places as well

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:391
> +    static const float gDimension = 1024.0;

Shouldnt this be define at the top or so?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:404
> +    WebGraphicsLayer* layerWK2 = toWebGraphicsLayer(layer);

Why not just call it layer? I dont see what the WK2 adds

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:418
> +static void collectCompositingInfoRecursive(GraphicsLayer* rootLayer, WebLayerTreeInfo& outInfo, Vector<WebGraphicsLayer*>& outLayers)

REcursively?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:431
> +void WebGraphicsLayer::sendLayersToUI(WebCore::GraphicsLayer* rootLayer, WebPage* webPage)

ToUIProcess?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:449
> +        WebGraphicsLayer* layerWK2 = layers[i];

layer*

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:67
> +    void pauseAnimation(const String& /*animationName*/, double /*timeOffset*/);

this is a header, please just write animationName and leave out the /* */s

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.