Bug 74133 - Convert some fast/regions pixel tests to reftests
Summary: Convert some fast/regions pixel tests to reftests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Jacob Goldstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-08 15:07 PST by Jacob Goldstein
Modified: 2012-11-02 12:06 PDT (History)
4 users (show)

See Also:


Attachments
Patch (111.23 KB, patch)
2011-12-14 10:32 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (147.47 KB, patch)
2011-12-14 10:48 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (111.27 KB, patch)
2011-12-14 11:11 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (1021 bytes, patch)
2011-12-14 11:19 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (112.26 KB, patch)
2011-12-14 11:39 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (113.61 KB, patch)
2011-12-15 13:37 PST, Jacob Goldstein
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (134.26 KB, patch)
2011-12-16 11:07 PST, Jacob Goldstein
tony: review-
tony: commit-queue-
Details | Formatted Diff | Diff
Patch (112.58 KB, patch)
2011-12-16 11:29 PST, Jacob Goldstein
tony: review-
tony: commit-queue-
Details | Formatted Diff | Diff
Patch (112.63 KB, patch)
2012-01-16 14:01 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (225.32 KB, patch)
2012-01-16 14:18 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff
Patch (112.69 KB, patch)
2012-01-16 14:21 PST, Jacob Goldstein
tony: review+
Details | Formatted Diff | Diff
Patch (113.14 KB, patch)
2012-01-17 12:46 PST, Jacob Goldstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Goldstein 2011-12-08 15:07:51 PST
Convert some fast/regions pixel tests to reftests
Comment 1 Jacob Goldstein 2011-12-14 10:32:41 PST
Created attachment 119245 [details]
Patch
Comment 2 Jacob Goldstein 2011-12-14 10:48:09 PST
Created attachment 119247 [details]
Patch
Comment 3 Jacob Goldstein 2011-12-14 11:11:40 PST
Created attachment 119256 [details]
Patch
Comment 4 Jacob Goldstein 2011-12-14 11:19:32 PST
Created attachment 119259 [details]
Patch
Comment 5 Jacob Goldstein 2011-12-14 11:39:37 PST
Created attachment 119267 [details]
Patch
Comment 6 WebKit Review Bot 2011-12-14 12:42:09 PST
Comment on attachment 119267 [details]
Patch

Attachment 119267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10873348

New failing tests:
fast/regions/webkit-flow-inlines-dynamic.html
Comment 7 Jacob Goldstein 2011-12-15 10:21:51 PST
Failure listed in Comment #6 not associated with the test changed by this patch
Comment 8 Jacob Goldstein 2011-12-15 10:29:18 PST
Failure is related - investigating

(In reply to comment #7)
> Failure listed in Comment #6 not associated with the test changed by this patch
Comment 9 Tony Chang 2011-12-15 13:34:54 PST
I suspect the failure is because on different OSes, the word wrapping will happen at different places.  This is probably not the easiest test to convert.  You could maybe change the text to spans with a fixed width or try using the Ahem font.
Comment 10 Jacob Goldstein 2011-12-15 13:37:13 PST
Created attachment 119494 [details]
Patch
Comment 11 WebKit Review Bot 2011-12-15 16:06:42 PST
Comment on attachment 119494 [details]
Patch

Attachment 119494 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10914419

New failing tests:
fast/regions/webkit-flow-inlines-dynamic.html
Comment 12 Jacob Goldstein 2011-12-16 11:07:34 PST
Created attachment 119635 [details]
Patch

Removed change to test_expectations.txt - results from the updated test will still be ignored on Chromium
Comment 13 Tony Chang 2011-12-16 11:12:26 PST
Comment on attachment 119635 [details]
Patch

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

> LayoutTests/ChangeLog:16
> +2011-12-15  Eric Carlson  <eric.carlson@apple.com>
> +

Looks like the ChangeLog files didn't merge properly.
Comment 14 Jacob Goldstein 2011-12-16 11:29:43 PST
Created attachment 119640 [details]
Patch

Corrected ChangeLog
Comment 15 Tony Chang 2011-12-16 11:42:55 PST
Comment on attachment 119640 [details]
Patch

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

> LayoutTests/ChangeLog:4
> +        updates
> +

Did you mean to include these lines?  Maybe just remove them.

> LayoutTests/fast/regions/webkit-flow-inlines-dynamic.html:39
>  
>  <div id="content">

Could you add some text saying what the test is testing?  E.g., The text in the test should not get out of the region.

> LayoutTests/fast/regions/webkit-flow-inlines-dynamic.html:-58
> -<script>
> -document.body.offsetWidth;
> -document.getElementById('test').appendChild(document.createTextNode(" Here is some added content that will mess things up, since it shoves things down."));

Is this an important part of the test?  Why is it being removed?
Comment 16 Jacob Goldstein 2012-01-16 14:01:55 PST
Created attachment 122682 [details]
Patch
Comment 17 Jacob Goldstein 2012-01-16 14:18:36 PST
Created attachment 122683 [details]
Patch
Comment 18 Jacob Goldstein 2012-01-16 14:21:20 PST
Created attachment 122684 [details]
Patch
Comment 19 Jacob Goldstein 2012-01-16 14:25:58 PST
Latest patch address Comment #15:

- Updated ChangeLog to remove addition line
- Added text to explain what is being tested
- Modified test to include dynamic insertion of text to verify that existing text remains within region after insertion


NOTE: The text being used in the test files was changed to help protect against failures on various platforms where font size may result in slightly difference spacing.  With the new text being used, slightly changes should not affect text wrapping
Comment 20 Tony Chang 2012-01-17 11:46:42 PST
Comment on attachment 122684 [details]
Patch

Can we use the ahem font to make the test even more cross platform compatible?
Comment 21 Jacob Goldstein 2012-01-17 12:46:09 PST
Created attachment 122800 [details]
Patch

Updated to use the Ahem font
Comment 22 WebKit Review Bot 2012-01-17 15:26:08 PST
Comment on attachment 122800 [details]
Patch

Clearing flags on attachment: 122800

Committed r105200: <http://trac.webkit.org/changeset/105200>
Comment 23 WebKit Review Bot 2012-01-17 15:26:15 PST
All reviewed patches have been landed.  Closing bug.