[webkit-reviews] review denied: [Bug 35388] [Qt] GraphicsLayer: support webGL : [Attachment 51826] Compilation fix

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Jarkko Sakkinen
<jarkko.j.sakkinen at gmail.com>'s request for review:
Bug 35388: [Qt] GraphicsLayer: support webGL
https://bugs.webkit.org/show_bug.cgi?id=35388

Attachment 51826: Compilation fix
https://bugs.webkit.org/attachment.cgi?id=51826&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
we don't name arguments that don't add clarity:
 656	     void paint(QPainter* painter) const;

Why the double-spacing?
496 {
 497	 m_internal->m_glWidget->makeCurrent();
 498 
 499	 m_internal->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER,
m_internal->m_mainFbo);
 500 
 501	 glReadPixels(/* x */ 0, /* y */ 0, m_currentWidth, m_currentHeight,
GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE,
m_internal->m_pixels.bits());
 502	 painter->drawImage(/* x */ 0, /* y */ 0,
m_internal->m_pixels.transformed(QMatrix().rotate(180)));
 503 
 504	 m_internal->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER,
m_internal->m_currentFbo);
 505 }

This should need { } to have local variables, no?
   case Canvas3DContentType:
 417	     m_gc3D->paint(painter);
 418	     GraphicsContext gc(painter);
 419	     m_layer->paintGraphicsLayerContents(gc,
option->exposedRect.toAlignedRect());
 420	     break;
 421 #endif

Why the extra new line?
	     break;
 565 
 566 #endif


Double spacing again?

 void GraphicsLayerQt::setContentsToGraphicsContext3D(const GraphicsContext3D*
ctx)
 922 {
 923	 if (ctx == m_impl->m_gc3D)
 924	     return;
 925 
 926	 m_impl->m_pendingContent.contentType =
GraphicsLayerQtImpl::Canvas3DContentType;
 927 
 928	 m_impl->m_gc3D = ctx;
 929 
 930	 m_impl->notifyChange(GraphicsLayerQtImpl::ContentChange);
 931 }
 932 

Besides the nits this change looks good.  r- for the nits.