Bug 80191 – [CSS Regions] Convert some fast/regions pixel tests to reftests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Rebecca Hauck]

Convert some fast/regions pixel tests to reftests

[Rebecca Hauck]

Created attachment 130402 [details]
Patch for the bug

[Dirk Schulze]

Comment on attachment 130402 [details]
Patch for the bug

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?

> 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?

[Rebecca Hauck]

(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!

[Rebecca Hauck]

Created attachment 131689 [details]
Added more description to the Changelog about edits made to the original test file

[Dirk Schulze]

(In reply to comment #4)
> Created an attachment (id=131689) [details]
> Added more description to the Changelog about edits made to the original test file

Seems that the patch could not get applied.

[Rebecca Hauck]

Created attachment 131733 [details]
fixed conflicts - re-uploading patch with more descriptive changelog

[Hajime Morrita]

Comment on attachment 131733 [details]
fixed conflicts - re-uploading patch with more descriptive changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=131733&action=review

> LayoutTests/ChangeLog:6100
> [phone]  Rebecca Hauck  <[e-mail]>

The diff should be on the top of the file.

[Rebecca Hauck]

Created attachment 132398 [details]
Moved diffs to top

[Eric Seidel (no email)]

Comment on attachment 131733 [details]
fixed conflicts - re-uploading patch with more descriptive changelog

Cleared Hajime Morita's review+ from obsolete attachment 131733 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Mihnea Ovidenie]

Created attachment 180656 [details]
Patch

[Mihnea Ovidenie]

Created attachment 180657 [details]
Patch 2

[WebKit Review Bot]

Comment on attachment 180657 [details]
Patch 2

Clearing flags on attachment: 180657

Committed r138446: <http://trac.webkit.org/changeset/138446>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.