[Webkit-unassigned] [Bug 76248] Initial implementation of GraphicsContext3DOpenGLES.cpp

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76248





--- Comment #4 from ChangSeok Oh <kevin.cs.oh at gmail.com>  2012-01-17 01:27:52 PST ---
(From update of attachment 122537)
View in context: https://bugs.webkit.org/attachment.cgi?id=122537&action=review

Thank you for review. :)

>> Source/WebCore/ChangeLog:13
>> +        No new tests required.
> 
> Might want to explain why there are no new tests required.

Done.

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::GraphicsContext3D::texImage2D):
> 
> Here it might be good to briefly explain how each method differs from it's desktop GL counterpart.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:103
>> +    // resize multisample FBO
> 
> Nit. Comments should begin with a capital letter and end with a period.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:107
>> +    // resize regular FBO
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:225
>> +    // FIXME : We don't care about multisampleFBO currently.
> 
> Extra space after FIXME. Might want to open a bug about this now.

Done. I filed a new bug to support anti-aliasing on GLES backend.
https://bugs.webkit.org/show_bug.cgi?id=76420

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:240
>> +        notImplemented();
> 
> Why do you do this check twice? Perhaps you should return early if multisampling is needed?

Removed two checking. I think it's better to deal with supporting multisampleing in an another bug.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:250
>> +        notImplemented();
> 
> Ditto.

Removed two checking, too.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:281
>> +        info.name.append("[0]");
> 
> Extra space after the FIXME. Might want to investigate this a bit now to avoid code churn.

Removed lines.
Though I opened a new bug76427 to fix it, the issue is not reproduced after updating webgl conformance test.
Let me keep track of it. If there is no issue, then I'll close the bug without any change.

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:289
>> +    // all previous rendering calls should be done before reading pixels.
> 
> Certainly the driver bug wouldn't affect both OpenGLES drivers and for whatever driver this work-around was added for. Can you just remove it?

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.