Bug 41576

Summary: [chromium] Fix drawing of inset box shadows
Product: WebKit Reporter: Nico Weber <thakis>
Component: Layout and RenderingAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: agl, bdakin, dglazkov, eric, fishd, hyatt, mitz, morrita, paulirish, tbassetto+bugzilla
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
thakis: commit-queue-
patch without junk
none
Patch
none
Patch dglazkov: review+

Description Nico Weber 2010-07-03 22:33:54 PDT
http://crbug.com/29427
Comment 1 Nico Weber 2010-07-03 22:38:17 PDT
Created attachment 60466 [details]
patch
Comment 2 Nico Weber 2010-07-03 22:40:24 PDT
Created attachment 60467 [details]
patch without junk

cq- because this needs rebaselining
Comment 3 Nico Weber 2010-07-03 22:46:49 PDT
Eric, you reviewed the patch that this patch is fixing. Can you review this, too? It's just one line.
Comment 4 Dimitri Glazkov (Google) 2010-07-04 09:41:38 PDT
You still have junk in the ChangeLog.

Hyatt, Mitz -- can you take a look?
Comment 5 mitz 2010-07-04 09:56:22 PDT
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().
Comment 6 Adam Langley 2010-07-07 08:36:14 PDT
(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.
Comment 7 Nico Weber 2010-07-07 22:00:34 PDT
> 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.
Comment 8 Adam Langley 2010-07-08 08:00:00 PDT
(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?
Comment 9 Hajime Morrita 2010-12-02 03:19:12 PST
Created attachment 75365 [details]
Patch
Comment 10 Hajime Morrita 2010-12-02 03:21:33 PST
Hi Nico, Dimitri, Darin, could you take a look?
This change is now small, and closed under platform/ chromium part.
Comment 11 Nico Weber 2010-12-02 13:39:52 PST
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).
Comment 12 Hajime Morrita 2010-12-02 22:53:23 PST
Created attachment 75466 [details]
Patch
Comment 13 Hajime Morrita 2010-12-02 22:59:54 PST
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.)
Comment 14 Adam Langley 2010-12-14 07:25:14 PST
Sorry for the delay. Please bug me often if you have a review outstanding!

LGTM
Comment 15 Dimitri Glazkov (Google) 2010-12-14 09:18:19 PST
Comment on attachment 75466 [details]
Patch

rs=me if agl says it's good.
Comment 16 Eric Seidel (no email) 2010-12-14 15:13:36 PST
Attachment 75466 [details] was posted by a committer and has review+, assigning to MORITA Hajime for commit.
Comment 17 Hajime Morrita 2010-12-14 18:09:38 PST
Committed r74089: <http://trac.webkit.org/changeset/74089>
Comment 18 Hajime Morrita 2010-12-14 18:13:56 PST
agl, Dimitri, thank you for taking a look! 
I was unnecessarily hesitate to shoot a ping :-/