[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