[Chromium] Layout Test canvas/philip/tests/2d.composite.globalAlpha.fill.html with --accelerated-2d-canvas.
Created attachment 81369 [details] Patch
Please review my patch.
Comment on attachment 81369 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81369&action=review > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:103 > +#if PLATFORM(SKIA) > + int a = SkAlphaMul(c.alpha(), s); > +#else > + int a = (c.alpha() * s) >> 8; > +#endif The Skia-specific code path is not really necessary here. I'd just remove it. > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:104 > + return Color((c.rgb() & 0x00FFFFFF) | (a << 24)); I think you should avoid relying on Color's internal representation. You could write this portably as return Color(c.red(), c.green(), c.blue(), a);
Created attachment 81581 [details] Patch
Comment on attachment 81581 [details] Patch Please review it.
We probably need this for fillPath() as well.
So do you mean I have to fix at the same time? Or do I have to make another bug entry? (In reply to comment #6) > We probably need this for fillPath() as well.
Comment on attachment 81581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81581&action=review > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:90 > + Color applyAlpha(const Color& c) So this is just premultiplying? Should this be a member on Color instead?
Can I add the function into the Color.h? Because Color class is very primitive, so I think we need to be careful. As I wrote in the comment, applyAlpha() is almost the same as PlatformContextSkia::State::applyAlpha(). But the type is difference. If PlatformContextSkia::State::applyAlpha() were in SkColor, I would include the function into Color.h. What do you think? (In reply to comment #8) > (From update of attachment 81581 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81581&action=review > > > Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:90 > > + Color applyAlpha(const Color& c) > > So this is just premultiplying? Should this be a member on Color instead?
I'm not familiar with PlatformContextSkia::State::applyAlpha. What's the purpose?
The purpose is to multiply globalAlpha() to fillStyle alpha. So if only software rendering is used, applyAlpha() is correctly applied. But if hardware is used, applyAlpha() is not called, and globalAlpha() is not applied(). That is the problem here. (In reply to comment #10) > I'm not familiar with PlatformContextSkia::State::applyAlpha. What's the purpose?
Ah yes, I misread things. This has nothing to do with the Color class as you correctly stated.
James, You are right, I confirmed the bug with my new test case. So how should I treat the bug? Should I make another entry? (In reply to comment #7) > So do you mean I have to fix at the same time? > > Or do I have to make another bug entry? > > (In reply to comment #6) > > We probably need this for fillPath() as well.
Just add another patch to this bug that fixes both issues and updates the ChangeLog. Thanks!
Also I will add another test related to fillPaht() problem. (In reply to comment #14) > Just add another patch to this bug that fixes both issues and updates the ChangeLog. Thanks!
Created attachment 81729 [details] Patch
Comment on attachment 81729 [details] Patch Please review.
The canvas/philip/ directory contains an imported test suite from Philip Taylor. Instead of modifying that directory, you should ask him to add your test to the suite or add the new test in some other directory. Otherwise it'll be difficult to update our copy of the suite in the future.
I see. I didn't know the policy how to add a test. Thank you for letting me know. (In reply to comment #18) > The canvas/philip/ directory contains an imported test suite from Philip Taylor. Instead of modifying that directory, you should ask him to add your test to the suite or add the new test in some other directory. Otherwise it'll be difficult to update our copy of the suite in the future.
Taylor, Can I add my test case as my upload patch? The new test is canvas/philip/tests/2d.composite.globalAlpha.fillPath.html In the patch, I fixed fillRect() but also fillPath(). My new test case is for fillPath(). Thanks,
James, It looks like hard to contact Taylor. If he doesn't like to include my test case, which directory is the good place to go? Do you have any idea? Thanks, (In reply to comment #18) > The canvas/philip/ directory contains an imported test suite from Philip Taylor. Instead of modifying that directory, you should ask him to add your test to the suite or add the new test in some other directory. Otherwise it'll be difficult to update our copy of the suite in the future.
In the meantime, this test could go in fast/canvas/
Ok, I'll do that. (In reply to comment #22) > In the meantime, this test could go in fast/canvas/
Created attachment 82095 [details] Patch
Comment on attachment 82095 [details] Patch According to Jame's suggestion, I move my test, and change some checking routine.
Comment on attachment 82095 [details] Patch Could you reivew?
Comment on attachment 82095 [details] Patch R=me. Thanks!
Comment on attachment 82095 [details] Patch Clearing flags on attachment: 82095 Committed r78315: <http://trac.webkit.org/changeset/78315>
All reviewed patches have been landed. Closing bug.