[Webkit-unassigned] [Bug 80191] Convert some fast/regions pixel tests to reftests

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Rebecca Hauck <rhauck at adobe.com>  2012-03-12 16:35:14 PST ---
(In reply to comment #2)
> (From update of attachment 130402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130402&action=review
> 
> > LayoutTests/ChangeLog:6
> > +        Convert some fast/regions pixel tests to reftests
> > +        https://bugs.webkit.org/show_bug.cgi?id=80191
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Your change log should be more explicit what you try to do with your patch, why you do it and how you do it. Also, you modified the test itself without a comment why you did it. Is the test still testing the same?
> 
> > LayoutTests/fast/regions/bottom-overflow-out-of-first-region.html:-6
> > -        text-align: justify;
> 
> Why did you remove this line?

Ah, you're right, I should have added a comment about why I removed this.  The reason is that text-align:justify caused a lot of problems when trying to reconstruct a ref file that works on chromium as well as osx.  With the font differences across platforms it's difficult to match natural line breaks on each platform, but it's nearly impossible to match the spacing of text when it is fully justified. 


> 
> > LayoutTests/fast/regions/bottom-overflow-out-of-first-region.html:-61
> > -    <div id="region3"></div>
> 
> It is not desirable to modify the test to match the reference. It should be the other way around. Why did you remove the region here?

Unless I'm missing something about what the original test was testing, it wasn't actually testing anything in region3 - nothing flowed into it. Further, a 1 px black border was applied to it, but no dimensions were specified, so it appeared in the test file as a little glyph in the lower left corner of region2. Since it didn't appear that region3 was part of the test in the first place, I didn't think constructing a ref with the glyph was useful in this case.  Also note that the description of the test in the test file makes no reference to region3 either.  Is this ok?


I'll be sure to add more clarity when I make these sorts of changes to test files in the future though. Thanks!

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