Bug 23482 - Replaces Yin-Yang character with a thumb-nail image in two layout tests
Summary: Replaces Yin-Yang character with a thumb-nail image in two layout tests
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-22 12:53 PST by Jungshik Shin
Modified: 2009-03-23 17:15 PDT (History)
5 users (show)

See Also:


Attachments
patch (87.59 KB, patch)
2009-01-22 14:30 PST, Jungshik Shin
darin: review-
Details | Formatted Diff | Diff
patch with tests in fast/block/float (120.81 KB, patch)
2009-01-27 11:22 PST, Jungshik Shin
darin: review+
Details | Formatted Diff | Diff
patch with in-place modifications of css2.1 tests (242.25 KB, patch)
2009-03-04 15:40 PST, Jungshik Shin
eric: review-
Details | Formatted Diff | Diff
patch same as the previous but without the roll-out of r41393 (assuming that'll be done separately) (154.30 KB, patch)
2009-03-06 13:56 PST, Jungshik Shin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2009-01-22 12:53:04 PST
css2.1/t0905-c414-flt-fit-01-d-g.html and
css2.1/t0905-c5526-flthw-00-c-g.html
have YingYang symbol. That makes them rather fragile in the sense that they depend on the availability of a font with that character. For instance, on Windows, none of fonts (shipped by MS) has that character covered. We can replace YingYang character with a thumbnail image of YingYang symbol.
Comment 1 Jungshik Shin 2009-01-22 14:30:16 PST
Created attachment 26940 [details]
patch

I added 'yinyang.png' to 'support' directory in css2.1 and made two tests refer to it in place of actual U+262F (Yin Yang symbol).
Comment 2 mitz 2009-01-22 14:37:36 PST
I think there is a policy of refraining from modifying tests in the css2.1 directory, because they have an external source. Instead, an equivalent test, with the proposed modifications, can be created in a different directory.
Comment 3 Jungshik Shin 2009-01-22 15:54:43 PST
Thank you for the info. What directory would be best for putting modified versions of two tests? 

There are other css tests with similar issues (e.g. tests with arrows in various directions). Aha, LayoutTests/platform/win/css2.1/resources has Mac-compatible-font-fallback.css and two html files (run-webkit-tests-{prologue,epilogue}.html) to work around them. However, that trick makes those tests rely on an unrelated feature (CSS @font-face with unicode range). I think it's better to avoid that dependency if possible. 

Comment 4 mitz 2009-01-22 15:57:05 PST
The names suggest that those are float tests, so fast/block/float may be a good place for them.
Comment 5 Darin Adler 2009-01-23 08:58:28 PST
Comment on attachment 26940 [details]
patch

Setting flag to review- based on the comments in this bug.

Making an equivalent test you can run when you lack a font with that symbol seems fine -- it seems slightly silly to add an actual "yin/yang" image, but probably OK.
Comment 6 Jungshik Shin 2009-01-27 11:22:21 PST
Created attachment 27078 [details]
patch with tests in fast/block/float

I copied the following 3 tests to fast/block/float (as suggested) and modified the copies to use png images (YinYang symbol and SE arrow) and '<-' (Leftward white arrow).

              css2.1/t0905-c414-flt-fit-01-d-g.html
              css2.1/t0905-c5525-fltblck-00-d-ag.html           
              css2.1/t0905-c5526-flthw-00-c-g.html
Comment 7 Darin Adler 2009-02-24 11:32:01 PST
Comment on attachment 27078 [details]
patch with tests in fast/block/float

Seems OK. r=me
Comment 8 Dimitri Glazkov (Google) 2009-03-03 10:34:01 PST
Landed as http://trac.webkit.org/changeset/41393.
Comment 9 Pam Greene (IRC:pamg) 2009-03-03 14:28:37 PST
The patch that was landed included a bunch of *.rej and *.orig files. I removed them in http://trac.webkit.org/changeset/41405 .

Dimitri, try adding those to your svn global-ignores list. :)
Comment 10 Eric Seidel (no email) 2009-03-04 11:59:53 PST
Hyatt is complaining that the results checked in are wrong.

I'm not sure we even want to fix this bug this way. Why don't we just add some fonts w/ this symbol to the ones which test_shell uses for running the tests?  It seems silly to hack our tests to work around Windows lame fonts.
Comment 11 Dimitri Glazkov (Google) 2009-03-04 12:09:14 PST
My apologies. I had to hand-roll this patch (as you can see from the diff, the copying of the files wasn't done correctly), and I didn't notice the merge artifacts.

Let me know what you guys decide and I'll make it right.
Comment 12 Jungshik Shin 2009-03-04 13:19:04 PST
(In reply to comment #10)
> Hyatt is complaining that the results checked in are wrong.
> 
> I'm not sure we even want to fix this bug this way. Why don't we just add some
> fonts w/ this symbol to the ones which test_shell uses for running the tests? 
> It seems silly to hack our tests to work around Windows lame fonts.

On the other hand, these tests were written for visual inspection (by Ian) but I don't think they're suitable for automated pixel tests. For automated pixel tests, we should make them NOT depend on anything other than what's actually tested. And, my patch tried to achieve 'that independence'. 

The test failure is due to the svn-copied tests (html files) got landed without being revised. It seems that 'svn-apply' does not work well when I 'svn-copy foo bar', revise 'bar' and generate 'svn-create-patch'.  Or did I do something wrong (as Dimitri wrote)?  

Anyway, Dave told me on IRC that it's better to revise the original tests in place (as opposed to copy them and put revised versions in a new location). So, I'll make a new patch to do that. 


Comment 13 Jungshik Shin 2009-03-04 15:40:48 PST
Created attachment 28288 [details]
patch with in-place modifications of css2.1 tests

Instead of duping css2.1 tests to a new place and revising them there, this patch modifies the original tests in place.  It also removes the previously added tests in fast/block/float. 

Somehow, png files added in attachment 27078 [details] that are removed in this patch are not recognized as binary and diff are not base64-encoded in the patch. (new png files in css2.1/supports are recognized as image/png and base64-encoded). However, running 'svn-unapply' and 'svn-apply' multiple times work. Also after each 'svn-apply', webkit test passes.
Comment 14 Eric Seidel (no email) 2009-03-04 16:11:30 PST
Where do these tests come from?  Can we get them "fixed" upstream instead?
Comment 15 Jungshik Shin 2009-03-04 17:26:55 PST
They come from CSS 2.1 test suite.

Ian, what do you think of these changes? With actual characters replaced by thumbnail images, 'human testers' would still know what to look for and visual inspection wouldn't be affected, which you're worried about when I proposed removing those characters altogether (instead of replacing with thumbnail images). 

Comment 16 Jungshik Shin 2009-03-06 08:10:14 PST
BTW, this is http://crbug.com/2304 (chromium bug 2304). Among 5 tests deferred (see http://crbug.com/2304#c11), all but css2.1/t0805-c5519-brdr-r-01-e.html  are taken care of by the latest patch here. 
Comment 17 Jungshik Shin 2009-03-06 13:56:14 PST
Created attachment 28371 [details]
patch same as the previous but without the roll-out of r41393 (assuming that'll be done separately)

This is the same as attachment 28288 [details], but does not contain the patch for removing files added
in r41393 ( http://trac.webkit.org/changeset/41393). This can be used if r41393 is 
rolled out before I get r+ for attachment 28288 [details]. 

Perhaps, this is  safer and makes everybody happy sooner :-).
Comment 18 Eric Seidel (no email) 2009-03-09 17:22:57 PDT
I don't think we want this change.  I think that Chromium should either include some fonts when running test_shell which include this symbol (which is what Apple does when running DumpRenderTree on windows), or that you should lobby the CSS working group to get a similar change in the upstream test suite.  This is minimal value, for possibly a lot of pain trying to keep this diff if we later update our copy of the CSS 2.1 tests.
Comment 19 Eric Seidel (no email) 2009-03-09 17:23:20 PDT
Comment on attachment 28288 [details]
patch with in-place modifications of css2.1 tests

See my comments above.  I don't think we want this change.
Comment 20 Eric Seidel (no email) 2009-03-09 17:23:47 PDT
Sam rolled out the previous commit earlier today:
http://trac.webkit.org/changeset/41539
Comment 21 Pam Greene (IRC:pamg) 2009-03-23 17:10:17 PDT
(In reply to comment #18)
> ...Chromium should either include
> some fonts when running test_shell which include this symbol (which is what
> Apple does when running DumpRenderTree on windows)

Can you be more specific about how DRT calls up fonts that aren't installed on the system? Are they distributed along with DRT, and if so, where did the fonts come from (i.e. what licensing terms, and are they in the tree)? Trying to figure out how we can remove this font dependency, which really just gets in the way, without having to lose coverage of what they're intending to be testing. Changing the CSS 2.1 tests is the best solution, but it may take a long time.
Comment 22 Eric Seidel (no email) 2009-03-23 17:15:52 PDT
(In reply to comment #21)
> Can you be more specific about how DRT calls up fonts that aren't installed on
> the system? Are they distributed along with DRT, and if so, where did the fonts
> come from (i.e. what licensing terms, and are they in the tree)? Trying to
> figure out how we can remove this font dependency, which really just gets in
> the way, without having to lose coverage of what they're intending to be
> testing. Changing the CSS 2.1 tests is the best solution, but it may take a
> long time.

Sadly, the solution for DRT isn't particularly elegant either.  WebKit.org does not distribute the fonts.  however the Apple (and qt) versions of DRT require the set of ttf fonts distributed with Safari for Windows.  Individual contributers find these fonts in their own ways, and install them into a directory pointed to by WEBKIT_TEST_FONTS and drt takes care of loading them into process:
http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp#L67