Bug 33909 - Intro text at Star Wars demo is clipped
Summary: Intro text at Star Wars demo is clipped
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://www.gesteves.com/experiments/s...
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-01-20 10:00 PST by Adam Roben (:aroben)
Modified: 2010-03-18 10:32 PDT (History)
6 users (show)

See Also:


Attachments
screenshot (15.31 KB, image/png)
2010-01-20 10:00 PST, Adam Roben (:aroben)
no flags Details
Patch (64.73 KB, patch)
2010-03-17 15:02 PDT, Enrica Casucci
simon.fraser: review-
Details | Formatted Diff | Diff
Patch2 (49.78 KB, patch)
2010-03-17 17:01 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
patch3 (49.89 KB, patch)
2010-03-17 17:14 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch4 (28.08 KB, patch)
2010-03-18 10:00 PDT, Enrica Casucci
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-01-20 10:00:48 PST
Created attachment 47045 [details]
screenshot

To reproduce:

1. Go to http://www.gesteves.com/experiments/starwars.html?nouacheck=1
2. Wait

The "A long time ago in a galaxy far, far away..." text is clipped. See screenshot.
Comment 1 Adam Roben (:aroben) 2010-01-20 10:03:05 PST
<rdar://problem/7560979>
Comment 2 Enrica Casucci 2010-03-17 15:02:10 PDT
Created attachment 50962 [details]
Patch
Comment 3 WebKit Review Bot 2010-03-17 15:05:11 PDT
Attachment 50962 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/GraphicsLayerCACF.cpp:62:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/win/GraphicsLayerCACF.cpp:63:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Simon Fraser (smfr) 2010-03-17 15:11:05 PDT
Comment on attachment 50962 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 56128)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> +2010-03-17  Enrica Casucci  <enrica@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.
Comment 5 mitz 2010-03-17 15:14:07 PDT
> > +            return;
> > +        }
> > +        WKCACFLayer::setNeedsDisplay(dirtyRect);
> 
> I think this would be clearer with an 'else'.

If this is referring to the last statement, then WebKit style is no “else” after “return”.
Comment 6 Enrica Casucci 2010-03-17 17:01:25 PDT
Created attachment 50973 [details]
Patch2

I think I've addressed all of your comments except for the return, because I've changed the code.
Comment 7 Enrica Casucci 2010-03-17 17:14:59 PDT
Created attachment 50977 [details]
patch3
Comment 8 Enrica Casucci 2010-03-18 10:00:02 PDT
Created attachment 51039 [details]
Patch4
Comment 9 Simon Fraser (smfr) 2010-03-18 10:07:01 PDT
Comment on attachment 51039 [details]
Patch4

> Index: LayoutTests/compositing/repaint/layer-repaint.html
> ===================================================================
> --- LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> +++ LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> @@ -0,0 +1,40 @@
> +<!DOCTYPE html>
> +
> +<html>
> +<head>
> +  <style type="text/css" media="screen">
> +    .container {
> +        border: 1px solid black;
> +        height: 200px;
> +        width: 200px;
> +        -webkit-transform: translateZ(0);
> +    }
> +    
> +    #tester {
> +        margin: 50px;

I think this box needs to not be centered to show the bug, so something like margin: 10px 50px 90px 50px;

> +        height: 100px;
> +        width: 100px;
> +        background-color: red;
> +    }
> +  </style>
> +  <script type="text/javascript" charset="utf-8">
> +    function doTest() {
> +        setTimeout("changeBackground();", 500);

We normally try to make tests run as fast as possible; an extra 0.5 secs is undesirable. It would be good to know if setTimeout( , 0) is enough here.

r=me with those changes to the test.
Comment 10 Enrica Casucci 2010-03-18 10:08:42 PDT
(In reply to comment #9)
> (From update of attachment 51039 [details])
> > Index: LayoutTests/compositing/repaint/layer-repaint.html
> > ===================================================================
> > --- LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> > +++ LayoutTests/compositing/repaint/layer-repaint.html	(revision 0)
> > @@ -0,0 +1,40 @@
> > +<!DOCTYPE html>
> > +
> > +<html>
> > +<head>
> > +  <style type="text/css" media="screen">
> > +    .container {
> > +        border: 1px solid black;
> > +        height: 200px;
> > +        width: 200px;
> > +        -webkit-transform: translateZ(0);
> > +    }
> > +    
> > +    #tester {
> > +        margin: 50px;
> 
> I think this box needs to not be centered to show the bug, so something like
> margin: 10px 50px 90px 50px;
> 
> > +        height: 100px;
> > +        width: 100px;
> > +        background-color: red;
> > +    }
> > +  </style>
> > +  <script type="text/javascript" charset="utf-8">
> > +    function doTest() {
> > +        setTimeout("changeBackground();", 500);
> 
> We normally try to make tests run as fast as possible; an extra 0.5 secs is
> undesirable. It would be good to know if setTimeout( , 0) is enough here.
> 
> r=me with those changes to the test.

My bad, didn't copy the latest version over from Windows that had exactly what you pointed out.
Comment 11 Enrica Casucci 2010-03-18 10:32:29 PDT
Committed revision 56169.
Thanks Simon for helping me on this :-)