[webkit-reviews] review denied: [Bug 56431] Add support for preserveDrawingBuffer context creation attribute : [Attachment 86353] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied John Bauman
<jbauman at chromium.org>'s request for review:
Bug 56431: Add support for preserveDrawingBuffer context creation attribute
https://bugs.webkit.org/show_bug.cgi?id=56431

Attachment 86353: Patch
https://bugs.webkit.org/attachment.cgi?id=86353&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86353&action=review

Overall this looks excellent; thanks for taking care of it. There are a few
comments that need to be fixed, and there are a couple of questions about
completeness of the logic.

> Source/WebCore/ChangeLog:16
> +	   No new tests. It seems difficult to be difficult/impossible to
trigger

Grammar: "seems difficult" -> "seems"

> Source/WebCore/ChangeLog:18
> +	   an early compositing operation in DumpRenderTree, making this hard
to
> +	   test automatically. There is a WebGL conformance test for it.

For the record, we might be able to test this automatically by making it a
pixel test.

Please also mention the manual testing that was done with this patch. In
particular, we should test the following cases:
  - Chromium with normal flags
  - Chromium with --disable-accelerated-compositing
  - Safari on Mac

with the conformance test as well as a few demos.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:531
> +    if (contextAttributes->stencil())
> +	   clearMask |= GraphicsContext3D::STENCIL_BUFFER_BIT;

Per Vlad's email to the WebGL working group, we may need to be careful with the
clearing of the stencil buffer. We prefer to use packed a depth stencil
renderbuffer in implementations, so we need to make sure that if there's a
stencil buffer attached, even if the user didn't request it, that we properly
clear it here. If this doesn't already work (perhaps getContextAttributes() is
already doing the right thing) then we should at least add a FIXME here.

> Source/WebCore/html/canvas/WebGLRenderingContext.h:481
> +    // Clear if the backbuffer if it was composited since the last
operation.

Typo: "Clear if" -> "Clear".

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1482
> +    // FIXME: Any accelerated compositor needs to be notified of the
changes.

What are the consequences of this FIXME for the Safari / Mac port? We
definitely do not want to regress that port; it is working well now and needs
to continue to do so.