[webkit-reviews] review denied: [Bug 58003] chromium support for glSetLatch and glWaitLatch between 3D contexts : [Attachment 88914] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 11 15:03:16 PDT 2011
Kenneth Russell <kbr at google.com> has denied John Bates <jbates at google.com>'s
request for review:
Bug 58003: chromium support for glSetLatch and glWaitLatch between 3D contexts
https://bugs.webkit.org/show_bug.cgi?id=58003
Attachment 88914: Patch
https://bugs.webkit.org/attachment.cgi?id=88914&action=review
------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88914&action=review
This looks very good overall and as we've discussed offline the results are
excellent, fixing multiple longstanding bugs. We should do the API change to
wait/setLatchCHROMIUM that we've discussed offline. A few other relatively
minor issues. Don't forget to regenerate the ChangeLogs. Also, I suggest using
"webkit-patch upload" to upload your patch the next time as it will take care
of some things like style checks.
> Source/WebCore/ChangeLog:3
> + Reviewed by Ken Russell.
Leave this line as "Reviewed by NOBODY (OOPS!)". The commit scripts will
rewrite it after it's been reviewed.
> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:73
> + void waitLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId);
> + void setLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId);
Per offline discussion, let's remove the shmId argument now.
> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:177
> - m_a(a), m_b(b), m_c(c), m_d(d)
> + m_a(a), m_b(b), m_c(c), m_d(d)
This whitespace-only change doesn't belong in this patch.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:276
> + // Before drawLayers:
This comment seems useless.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:277
> + if (hardwareCompositing()) {
Even though your current code runs, for correctness, the following block of
code needs to be run against the layer renderer's GraphicsContext3D and all of
the child layers' contexts:
if (m_context->getExtensions()->supports("GL_CHROMIUM_latch"))
m_context->getExtensions()->ensureEnabled("GL_CHROMIUM_latch");
and the code which uses getChildToParentLatchCHROMIUM and waitLatchCHROMIUM
needs to be guarded by a test:
if (extensions->supports("GL_CHROMIUM_latch"))
See e.g. Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:278
> + // The multithreaded compositor case does not work as long as
Change this to "FIXME: ... case will not work".
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:281
> + // potentially clobbering the parent texture that is beibng
renderered
typo: beibng
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:303
> + // After drawLayers:
This comment also seems useless.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:353
> + // Before updateCompositorResourcesRecursive, when the compositor runs
in
Please add a FIXME here.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:967
> -
> +
This whitespace-only change doesn't belong in this patch. I suggest you try to
configure your editor.
> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1125
> +// TODO(jbates): when compositor is multithreaded and copyTexImage2D bug is
fixed,
WebKit uses FIXME rather than TODO().
> Source/WebKit/chromium/ChangeLog:3
> + Reviewed by Ken Russell.
Per above, leave the OOPS line.
> Source/WebKit/chromium/public/WebGraphicsContext3D.h:175
> + virtual void getParentToChildLatchCHROMIUM(WGC3Duint* latchId) {} //
TODO = 0;
> + virtual void getChildToParentLatchCHROMIUM(WGC3Duint* latchId) {} //
TODO = 0;
> + virtual void waitLatchCHROMIUM(WGC3Dint shmId, WGC3Duint latchId) {} //
TODO = 0;
> + virtual void setLatchCHROMIUM(WGC3Dint shmId, WGC3Duint latchId) {} //
TODO = 0;
TODO -> FIXME:
Also, per offline discussion, let's remove the shmId argument, as it's being
removed from the implementation.
> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:254
> -
> +
Whitespace-only change.
> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:806
> +DELEGATE_TO_IMPL_2(waitLatchCHROMIUM, GC3Dint, GC3Duint)
> +DELEGATE_TO_IMPL_2(setLatchCHROMIUM, GC3Dint, GC3Duint)
Per offline discussion, remove first argument.
> Source/WebKit/chromium/src/GraphicsContext3DInternal.h:277
> + void waitLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId);
> + void setLatchCHROMIUM(GC3Dint shmId, GC3Duint latchId);
Remove shmId argument.
More information about the webkit-reviews
mailing list