RESOLVED WONTFIX 23482
Replaces Yin-Yang character with a thumb-nail image in two layout tests
https://bugs.webkit.org/show_bug.cgi?id=23482
Summary Replaces Yin-Yang character with a thumb-nail image in two layout tests
Jungshik Shin
Reported 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.
Attachments
patch (87.59 KB, patch)
2009-01-22 14:30 PST, Jungshik Shin
darin: review-
patch with tests in fast/block/float (120.81 KB, patch)
2009-01-27 11:22 PST, Jungshik Shin
darin: review+
patch with in-place modifications of css2.1 tests (242.25 KB, patch)
2009-03-04 15:40 PST, Jungshik Shin
eric: review-
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
Jungshik Shin
Comment 1 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).
mitz
Comment 2 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.
Jungshik Shin
Comment 3 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.
mitz
Comment 4 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.
Darin Adler
Comment 5 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.
Jungshik Shin
Comment 6 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
Darin Adler
Comment 7 2009-02-24 11:32:01 PST
Comment on attachment 27078 [details] patch with tests in fast/block/float Seems OK. r=me
Dimitri Glazkov (Google)
Comment 8 2009-03-03 10:34:01 PST
Pam Greene (IRC:pamg)
Comment 9 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. :)
Eric Seidel (no email)
Comment 10 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.
Dimitri Glazkov (Google)
Comment 11 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.
Jungshik Shin
Comment 12 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.
Jungshik Shin
Comment 13 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.
Eric Seidel (no email)
Comment 14 2009-03-04 16:11:30 PST
Where do these tests come from? Can we get them "fixed" upstream instead?
Jungshik Shin
Comment 15 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).
Jungshik Shin
Comment 16 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.
Jungshik Shin
Comment 17 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 :-).
Eric Seidel (no email)
Comment 18 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.
Eric Seidel (no email)
Comment 19 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.
Eric Seidel (no email)
Comment 20 2009-03-09 17:23:47 PDT
Sam rolled out the previous commit earlier today: http://trac.webkit.org/changeset/41539
Pam Greene (IRC:pamg)
Comment 21 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.
Eric Seidel (no email)
Comment 22 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
Note You need to log in before you can comment on or make changes to this bug.