[webkit-reviews] review denied: [Bug 99083] [chromium] Add WebLayer::canUseLCDText : [Attachment 168271] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 99083: [chromium] Add WebLayer::canUseLCDText
https://bugs.webkit.org/show_bug.cgi?id=99083

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

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


> Source/Platform/chromium/public/WebLayer.h:206
> +    // Returns true if text can be painted into this layer with LCD
anti-aliasing.
> +    // Typically LCD text is allowed if the layer is opaque and is not
rotated
> +    // or scaled.

I don't think this API belongs on WebLayer - WebLayers don't have painted
content (in general), WebContentLayers do.  It doesn't make any sense to talk
about LCD text on a video or external texture layer.

> Source/Platform/chromium/public/WebLayer.h:207
> +    // TODO(alokp): Make pure virtual after implementing the impl side.

Don't use TODO(person) in WebKit comments

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:649
> +    WebLayer* childHost = platformLayer();

This change seems unrelated

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:871
> +    // TODO(alokp): Rename PlatformContextSkia::setDrawingToImageBuffer

Why don't you just do that?

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:53
> +    // TODO(alokp): Remove LCD text setting after WebLayer::canUseLCDText is
implemented.

FIXME

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:175
> +    // TODO(alokp): Remove the LCD logic from here.

we don't use TODO(person) in WebKit - just FIXME and optionally cite a WebKit
bug if you have one filed