Bug 74133

Summary: Convert some fast/regions pixel tests to reftests
Product: WebKit Reporter: Jacob Goldstein <jacobg>
Component: Tools / TestsAssignee: Jacob Goldstein <jacobg>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, jacobg, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.6   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
webkit.review.bot: commit-queue-
Patch
tony: review-, tony: commit-queue-
Patch
tony: review-, tony: commit-queue-
Patch
none
Patch
none
Patch
tony: review+
Patch none

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.