[Webkit-unassigned] [Bug 33909] Intro text at Star Wars demo is clipped

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33909


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50962|review?                     |review-
               Flag|                            |




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com>  2010-03-17 15:11:05 PST ---
(From update of attachment 50962)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.