[webkit-reviews] review denied: [Bug 25417] Skia CompositeCopy should be mapped to kSrc_Mode not kSrcOver_Mode. : [Attachment 30207] Adding layout test

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 25417: Skia CompositeCopy should be mapped to kSrc_Mode not kSrcOver_Mode.
https://bugs.webkit.org/show_bug.cgi?id=25417

Attachment 30207: Adding layout test
https://bugs.webkit.org/attachment.cgi?id=30207&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I think this test would be much more readable if it used some JS to generate
the HTML instead of all the copy/paste HTML.

 16	  // FIXME: we don't exercise corner cases such as alpha=0
intentionally as many platforms are not good enough for now.

It's OK to check in failing tests.  I'd rather have the tests which fail on
some platforms, with understood behavior, than to have no tests at all.

I find the arrays of colors confusing.	Giving i and j longer english names
would be a start at helping.

If you didn't write this test, would you know what expectedColors[i][j]; was
supposed to mean?

i an j shoudl be locally scoped.  No reason for them not to be.

This javascript doesn't match the WebKit coding style guidelines:
http://webkit.org/coding/coding-style.html  However we're generally not as
strict about code style for test cases.  Still... we should try. :)

So the test takes an array of color values, composites them with something, and
then checks to make sure the array is correct.	Seems that a few clearer
variable names and a little less HTML/copy paste would go a long way towards
making this test more readable.

Otherwise the change looks fine.