[webkit-reviews] review denied: [Bug 61388] [chromium] edge aliasing on 3D transformed elements : [Attachment 95975] Layer anti-aliasing

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 10 18:43:30 PDT 2011


James Robinson <jamesr at chromium.org> has denied David Reveman
<reveman at google.com>'s request for review:
Bug 61388: [chromium] edge aliasing on 3D transformed elements
https://bugs.webkit.org/show_bug.cgi?id=61388

Attachment 95975: Layer anti-aliasing
https://bugs.webkit.org/attachment.cgi?id=95975&action=review

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

Would you mind updating this patch to apply to ToT?  I think Vince, Vangelis,
or Enne should review it - the actual graphics stuff here is well beyond me.

> WebCore/ChangeLog:12
> +	   No new tests. (OOPS!)

you need to get rid of the OOPS! here - replace it with a list of tests that
cover this change

> WebCore/ChangeLog:16
> +	   Change maxUntiledSize to 510 to ensure that tiles are not greater
> +	   than 512 with outer boarders.

typo: boarders->borders

> WebCore/ChangeLog:18
> +	   (WebCore::ContentLayerChromium::updateLayerSize): To avoid tiling,
> +	   pass empty tile size to tiler.

this comment doesn't make a lot of sense to me

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:189
> -    const IntSize tileSize(min(defaultTileSize, layerSize.width()),
min(defaultTileSize, layerSize.height()));
> +    const IntSize tileSize(defaultTileSize, defaultTileSize);

this change seems bad - all tiles are 256x256 regardless of the layer size?

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:88
> +    bool m_borderTexels;

why does ContentLayerChromium need an m_borderTexels? is this a bad merge?

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:92
> +    int x1 = rect.x();
> +    int y1 = rect.y();
> +    int x2 = rect.maxX();
> +    int y2 = rect.maxY();

should this be an IntRect?

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:104
> +    FloatPoint pt[4];

we have a FloatQuad data type, would that be better here?

> WebCore/platform/graphics/chromium/GeometryBinding.cpp:111
> +    for (int i = 0; i < 4; i++)
> +	   pt[i] = matrix.mapPoint(pt[i]);

i believe we can map FloatQuads directly without having to do it point at a
time

> WebCore/platform/graphics/chromium/GeometryBinding.h:51
> +    explicit GeometryBinding(GraphicsContext3D*,

nit: no need for 'explicit' on constructors that take more than one parameter


More information about the webkit-reviews mailing list