Bug 74133 – Convert some fast/regions pixel tests to reftests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jacob Goldstein]

Convert some fast/regions pixel tests to reftests

[Jacob Goldstein]

Created attachment 119245 [details]
Patch

[Jacob Goldstein]

Created attachment 119247 [details]
Patch

[Jacob Goldstein]

Created attachment 119256 [details]
Patch

[Jacob Goldstein]

Created attachment 119259 [details]
Patch

[Jacob Goldstein]

Created attachment 119267 [details]
Patch

[WebKit Review Bot]

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

[Jacob Goldstein]

Failure listed in Comment #6 not associated with the test changed by this patch

[Jacob Goldstein]

Failure is related - investigating

(In reply to comment #7)
> Failure listed in Comment #6 not associated with the test changed by this patch

[Tony Chang]

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.

[Jacob Goldstein]

Created attachment 119494 [details]
Patch

[WebKit Review Bot]

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

[Jacob Goldstein]

Created attachment 119635 [details]
Patch

Removed change to test_expectations.txt - results from the updated test will still be ignored on Chromium

[Tony Chang]

Comment on attachment 119635 [details]
Patch

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

> LayoutTests/ChangeLog:16
> [phone]  Eric Carlson  <[e-mail]>
> +

Looks like the ChangeLog files didn't merge properly.

[Jacob Goldstein]

Created attachment 119640 [details]
Patch

Corrected ChangeLog

[Tony Chang]

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?

[Jacob Goldstein]

Created attachment 122682 [details]
Patch

[Jacob Goldstein]

Created attachment 122683 [details]
Patch

[Jacob Goldstein]

Created attachment 122684 [details]
Patch

[Jacob Goldstein]

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

[Tony Chang]

Comment on attachment 122684 [details]
Patch

Can we use the ahem font to make the test even more cross platform compatible?

[Jacob Goldstein]

Created attachment 122800 [details]
Patch

Updated to use the Ahem font

[WebKit Review Bot]

Comment on attachment 122800 [details]
Patch

Clearing flags on attachment: 122800

Committed r105200: <http://trac.webkit.org/changeset/105200>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.