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.
<rdar://problem/7560979>
Created attachment 50962 [details] Patch
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 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.
> > + 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”.
Created attachment 50973 [details] Patch2 I think I've addressed all of your comments except for the return, because I've changed the code.
Created attachment 50977 [details] patch3
Created attachment 51039 [details] Patch4
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.
(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.
Committed revision 56169. Thanks Simon for helping me on this :-)