[webkit-reviews] review denied: [Bug 54710] [chromium] Create a LayerChromium layerTreeAsText. Add HUD to LayerRendererChromium that draws compositor FPS and, optionally, the layer tree. : [Attachment 84599] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 54710: [chromium] Create a LayerChromium layerTreeAsText. Add HUD to
LayerRendererChromium that draws compositor FPS and, optionally, the layer
tree.
https://bugs.webkit.org/show_bug.cgi?id=54710

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84599&action=review

Lots more nitpicks. Overall it looks fine, though.  I promise that all these
nitpicks will make your future WebKit patching life easier :)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:55
> +
> +

Unnecessary whitespace here

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:128
> +    m_headsUpDisplay.clear(); // explicitly destroy the HUD before the
TextureManager dies

Caps, period

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:129
> +    CCHeadsUpDisplay* getHeadsUpDisplay() { return m_headsUpDisplay.get(); }


nit: in webkit getters don't have "get" in the name, so this should be
headsUpDisplay();

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:39
> +#if PLATFORM(SKIA)

nit: should be USE(SKIA) nowadays. I don't know why this was changed, tbqh.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:143
> +    // draw fps

Caps, period

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:158
> +    // draw layer tree, if enabled

Caps, period

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:40
> +class CCHeadsUpDisplay {
> +public:
> +    explicit CCHeadsUpDisplay(LayerRendererChromium* owner);

nit:
Since this is single-ownership, add WTF_MAKE_NONCOPYABLE here (like in
PlatformCanvas), add a static PassOwnPtr<CCHeadsUpDisplay> create(), and make
the constructor private so you have to use the create() function.  That way the
type system will help make sure that you don't leak these guys accidentally -
you have to either assign it to an OwnPtr<> or call .leakPtr() explicitly.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:55
> +    void drawHudContents(GraphicsContext* ctx, const IntSize& hudSize);

nit: don't name the "ctx" parameter name here.