Bug 85913 – Test from bug 34875 does not work on chromium/mac

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shezan Baig]

The Ahem font is not the same size on all platforms, we need to explicitly set the font-size

[Shezan Baig]

Created attachment 140789 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140789 [details]
Patch

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

New failing tests:
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html

[WebKit Review Bot]

Created attachment 140808 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Oliver Hunt]

Created attachment 140825 [details]
Patch

[Oliver Hunt]

Created attachment 140826 [details]
Patch

[Oliver Hunt]

I'm waiting to see if the chrome ews manages to pass this test, and if it does i'll land

[Shezan Baig]

Thanks Oliver, just got back home and saw the chromium ews fail.  I'm still not sure why it would fail though, doesn't setting the font & font-size guarantee that the size will be the same on all platforms?

[Oliver Hunt]

(In reply to comment #7)
> Thanks Oliver, just got back home and saw the chromium ews fail.  I'm still not sure why it would fail though, doesn't setting the font & font-size guarantee that the size will be the same on all platforms?

No, it selects an appropriate font size to match what you've requested, it doesn't make perfect guarantees.  The way to fix it is as Ojan said: fixed-size inline-blocks -- no font details are involved in computing the metrics.

[Oliver Hunt]

Apparently it still doesn't work on chrome.  It would be nice if chrome would provide the actual test output though.

[Oliver Hunt]

Created attachment 140832 [details]
Patch

(In reply to comment #8)
> (In reply to comment #7)
> > Thanks Oliver, just got back home and saw the chromium ews fail.  I'm still not sure why it would fail though, doesn't setting the font & font-size guarantee that the size will be the same on all platforms?
> 
> No, it selects an appropriate font size to match what you've requested, it doesn't make perfect guarantees.  The way to fix it is as Ojan said: fixed-size inline-blocks -- no font details are involved in computing the metrics.

Font details still factor into the width of spaces between those inline-block spans. Is there a reason why this test cannot use the Ahem font?

[Shezan Baig]

Julien, should we just revert back to the ref-test from https://bugs.webkit.org/attachment.cgi?id=139014&action=review ?  That has the benefit of working in other browsers too :)

[Oliver Hunt]

Created attachment 140833 [details]
Patch

[Julien Chaffraix]

(In reply to comment #12)
> Julien, should we just revert back to the ref-test from https://bugs.webkit.org/attachment.cgi?id=139014&action=review ?  That has the benefit of working in other browsers too :)

No objection on that as it looks like getting something cross-platform using dumpAsText() is non-trivial. If anyone wants to revert to the ref-tests (or just ask me to), see https://bug-34875-attachments.webkit.org/attachment.cgi?id=139014

Dan, the test is already using Ahem but for some reason the output is not cross-platform (we thought somehow the font-size was chosen differently but it doesn't look like this is it).

[Shezan Baig]

(In reply to comment #14)
> No objection on that as it looks like getting something cross-platform using dumpAsText() is non-trivial. If anyone wants to revert to the ref-tests (or just ask me to), see https://bug-34875-attachments.webkit.org/attachment.cgi?id=139014

Cool - I think I still have the test in my git repo.  I'll be out all day tomorrow, but I can upload a patch for this when I get back on Thursday.

[WebKit Review Bot]

Comment on attachment 140833 [details]
Patch

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

New failing tests:
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html

[WebKit Review Bot]

Created attachment 140850 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ojan Vafai]

FYI: suppressed the failure for Chromium in http://trac.webkit.org/changeset/116477.

[Julien Chaffraix]

Created attachment 140854 [details]
Revert back to a ref-test using one of the original patch. Tested on Cr-Linux / Mac + Apple Mac SnowLeopard without problem.

[Julien Chaffraix]

Comment on attachment 140854 [details]
Revert back to a ref-test using one of the original patch. Tested on Cr-Linux / Mac + Apple Mac SnowLeopard without problem.

Sorry for jumping in late, this was the original approach for the test and it is passing on several port / platforms. I didn't think it was worth doing a ref-test when reviewing so feel free to disagree on the approach.

[Ojan Vafai]

(In reply to comment #20)
> (From update of attachment 140854 [details])
> Sorry for jumping in late, this was the original approach for the test and it is passing on several port / platforms. I didn't think it was worth doing a ref-test when reviewing so feel free to disagree on the approach.

I only see advantages to doing this as a reftest. What cost am I missing?

[WebKit Review Bot]

Comment on attachment 140854 [details]
Revert back to a ref-test using one of the original patch. Tested on Cr-Linux / Mac + Apple Mac SnowLeopard without problem.

Rejecting attachment 140854 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
oning/offsetLeft-offsetTop-multicolumn-expected.txt'
patching file LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn.html
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 FAILED at 3920.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ojan Vafai']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12655470

[Shezan Baig]

Created attachment 140903 [details]
Patch

[Antti Koivisto]

Skipped for Mac in fast/block/positioning/offsetLeft-offsetTop-multicolumn.html

[Antti Koivisto]

I mean in http://trac.webkit.org/changeset/116523

[Shezan Baig]

Created attachment 141164 [details]
Patch (enables test on mac/Skipped)

[Julien Chaffraix]

Comment on attachment 141164 [details]
Patch (enables test on mac/Skipped)

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

> LayoutTests/ChangeLog:19
> +        * platform/mac/Skipped:
> +        Re-enabled the test.

qt-mac/Skipped also has an entry, it should be re-enabled as part of this change too.

(btw, you can group the test_expectations.txt / Skipped edits together as it's a similar change)

[Shezan Baig]

Created attachment 141193 [details]
Patch (with change from comment 27)

[WebKit Review Bot]

Comment on attachment 141193 [details]
Patch (with change from comment 27)

Clearing flags on attachment: 141193

Committed r116663: <http://trac.webkit.org/changeset/116663>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.