[webkit-reviews] review granted: [Bug 44573] [chromium] Implement clipping for composited layers : [Attachment 65436] Proposed patch - removed temporary stencil clipping debug code

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 25 12:24:09 PDT 2010


Kenneth Russell <kbr at google.com> has granted Vangelis Kokkevis
<vangelis at chromium.org>'s request for review:
Bug 44573: [chromium] Implement clipping for composited layers
https://bugs.webkit.org/show_bug.cgi?id=44573

Attachment 65436: Proposed patch - removed temporary stencil clipping debug
code
https://bugs.webkit.org/attachment.cgi?id=65436&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
Generally looks good. There's one piece of complexity below I'd ask you to
rethink, but you can fix the issues below before commit.

WebCore/platform/graphics/chromium/LayerChromium.cpp:456
 +	GLC(glUniform4f(sv->borderShaderColorLocation(), 0, 1 , 0, 0.7));
It's probably worth a comment that the color values don't matter here since the
color mask is set to all false at this point.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:506
 +		// will allow us up to 255 nested clipping layers which is
hopefuly enough.
hopefully

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:554
 +		ASSERT(!currentStencilValue);
Because of this ASSERT, it feels to me like there must be some simpler logic
just involving a test of "--currentStencilValue == 0" or similar, and getting
rid of mustDisableStencil and stencilTestEnabledForSubtree. I'm not 100% sure
of this though and don't want you to risk breaking your current logic if it's
well tested, so feel free to just consider this for the future.


More information about the webkit-reviews mailing list