[Webkit-unassigned] [Bug 25417] Skia CompositeCopy should be mapped to kSrc_Mode not kSrcOver_Mode.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25417





------- Comment #16 from hamaji at google.com  2009-05-18 09:34 PDT -------
Thanks for the review!

(In reply to comment #15)
> (From update of attachment 30207 [review])
> 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 thought we cannot add a failing tests because the guideline is saying that
all tests should pass.

http://webkit.org/coding/contributing.html

The test case for alpha=0 will fail even on Mac OSX Leopard. Is this
acceptable? If so, I'll add test cases for these cases and upload another
patch. Of course, I'll also fix other parts according to your comments.

Thanks,

> 
> 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.
> 


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.