[webkit-reviews] review denied: [Bug 33909] Intro text at Star Wars demo is clipped : [Attachment 50962] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 33909: Intro text at Star Wars demo is clipped
https://bugs.webkit.org/show_bug.cgi?id=33909

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 56128)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,19 @@
> +2010-03-17  Enrica Casucci  <enrica at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Intro text at Star Wars demo is clipped.
> +	   <rdar://problem/7560979>
> +	   <https://bugs.webkit.org/show_bug.cgi?id=33909>
> +	   
> +	   Test: compositing/repaint/opacity-animation.html
> +
> +	   The origin of the dirty rectangle needs to be flipped.

I'd like to see a bit more verbage here, describing that we flip the coords for
drawing, so we have to flip invalidate rects too.

> Index: WebCore/platform/graphics/win/GraphicsLayerCACF.cpp
> ===================================================================

> +    virtual void setNeedsDisplay(const CGRect* dirtyRect)

I think this method should only to rect flipping in the situation where the
drawing code does flipping (i.e. check the m_owner->contentsOrientation()).

> +	   CGRect layerBounds = bounds();

If dirtyRect is null and we're not showing the repaint counter, this call is
wasting cycles.

> +	   if (m_owner && m_owner->showRepaintCounter()) {
> +	       CGRect repaintCounterRect = layerBounds;
> +	       repaintCounterRect.size.width = 48;     // We assume a maximum
of 4 digits.
> +	       repaintCounterRect.size.height = 25;    // We use 22 as font
size.
> +	       repaintCounterRect.origin.y = layerBounds.size.height -
(layerBounds.origin.y + repaintCounterRect.size.height);
> +	       WKCACFLayer::setNeedsDisplay(&repaintCounterRect);
> +	   }
> +	   if (dirtyRect) {
> +	       CGRect flippedDirtyRect = *dirtyRect;
> +	       flippedDirtyRect.origin.y = layerBounds.size.height -
(flippedDirtyRect.origin.y + flippedDirtyRect.size.height);
> +	       WKCACFLayer::setNeedsDisplay(&flippedDirtyRect);
> +	       return;
> +	   }
> +	   WKCACFLayer::setNeedsDisplay(dirtyRect);

I think this would be clearer with an 'else'.

> Index: LayoutTests/compositing/repaint/opacity-animation.html
> ===================================================================
> --- LayoutTests/compositing/repaint/opacity-animation.html	(revision 0)
> +++ LayoutTests/compositing/repaint/opacity-animation.html	(revision 0)

I think this testcase could be vastly simplified; it should end up as a green
box on success, and a red or partially red box on failure.

r- to check contentsOrientation(), and I'd like to see the other stuff fixed
too.