http://crbug.com/29427
Created attachment 60466 [details] patch
Created attachment 60467 [details] patch without junk cq- because this needs rebaselining
Eric, you reviewed the patch that this patch is fixing. Can you review this, too? It's just one line.
You still have junk in the ChangeLog. Hyatt, Mitz -- can you take a look?
Comment on attachment 60467 [details] patch without junk A method called GraphicsContext::canvasClip() sounds like a bad idea. According to GraphicsContext.h, // This clip function is used only by <canvas> code but this patch makes that comment false and illustrates why having this method is a bad idea. Moreover, the change log (which mentions a bunch of files that the patch doesn’t touch) does a very poor job of explaining the change. I suggest eliminating canvasClip() and devising a better solution to the platform-specific problem it was intended to solve; ideally, such a solution will not require people writing cross-platform code to have to think about the platform-specific problem. As things stand, there is no way for someone writing rendering code to tell whether they should call clip() or canvasClip().
(In reply to comment #5) > but this patch makes that comment false and illustrates why having this method is a bad idea. Moreover, the change log (which mentions a bunch of files that the patch doesn’t touch) does a very poor job of explaining the change. The point of canvas clip was that anti-aliased clipping causes seams when attempting to join two polygons. So the canvas folks didn't want it on by default, but everyone else was calling for my blood over anti-aliased rounded corners. I'm afraid that the ChangeLog here is insufficient to understand the problem. Why does the layered clipping fail? > I suggest eliminating canvasClip() and devising a better solution to the platform-specific problem it was intended to solve; ideally, such a solution will not require people writing cross-platform code to have to think about the platform-specific problem. As things stand, there is no way for someone writing rendering code to tell whether they should call clip() or canvasClip(). Possibly canvasClip should have been called aliasedClip. However, it's very easy to tell if you should be calling canvasClip: if you're not rendering to a <canvas> element then you shouldn't.
> I'm afraid that the ChangeLog here is insufficient to understand the problem. Why does the layered clipping fail? It doesn't seem to be compatible with how skia draws shadows. I'm not sure why not. It works with non-inset shadows by accident, since these are drawn with a clipOut() path, which doesn't use the antialiasing trick (it should – aliased shadows are ugly. But then the problem would be visible with normal, non-inset shadows too). I agree with mitz that the antialiasing trick should be made to work with shadows too and that my patch isn't a good idea.
(In reply to comment #7) > It works with non-inset shadows by accident, since these are drawn with a clipOut() path, which doesn't use the antialiasing trick (it should – aliased shadows are ugly. But then the problem would be visible with normal, non-inset shadows too). clipOut doesn't anti-alias because that would involve duplicating the frame buffer every time a clip was in effect. I don't suppose anyone wants to add the immediate mode anti-aliasing hack into Skia and save all this bother?
Created attachment 75365 [details] Patch
Hi Nico, Dimitri, Darin, could you take a look? This change is now small, and closed under platform/ chromium part.
Makes sense. I'd love agl to have a look, too. Maybe add // The state is popped in applyAntiAliasedClipPaths() to the line you added in clipPathAntiAliased, but LG from me. Also, I guess the shadow edges still look aliased with your patch (but they will still look way better than they do now).
Created attachment 75466 [details] Patch
Hi Nico, thank you for trying this! (In reply to comment #11) > Makes sense. I'd love agl to have a look, too. Sure. > > Maybe add > > // The state is popped in applyAntiAliasedClipPaths() Added. > Also, I guess the shadow edges still look aliased with your patch (but they will still look way better than they do now). Yes, In my understanding, this is because WebKit clips the background rectangle and the inset shadow separately, which causes weird appearance on the edge's partially-covered pixels. I guess even Mac port has same problem because such clipping order is controlled by WebCore. It might be possible to move clipping call down to RenderBox::paintBoxDecorationsWithSize(). (It is another story though.)
Sorry for the delay. Please bug me often if you have a review outstanding! LGTM
Comment on attachment 75466 [details] Patch rs=me if agl says it's good.
Attachment 75466 [details] was posted by a committer and has review+, assigning to MORITA Hajime for commit.
Committed r74089: <http://trac.webkit.org/changeset/74089>
agl, Dimitri, thank you for taking a look! I was unnecessarily hesitate to shoot a ping :-/