Bug 37872 – Add isGLES2Compliant to GraphicsContext3D

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Some GraphicsContext3D implementations will be built on top of desktop OpenGL, and some will be built on top of compliant OpenGL ES 2.0 implementations. In order to enforce strict OpenGL ES 2.0 and WebGL semantics on all platforms, it is convenient to have some checks in the shared code in WebGLRenderingContext. However, these checks need to be disabled when WebGLRenderingContext is delegating to an OpenGL ES 2.0 compliant GraphicsContext3D. We should add a bool isGLES2Compliant() to GraphicsContext3D to be able to conditionalize some of these checks.

[Zhenyao Mo]

Created attachment 53871 [details]
patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 53871 [details]
patch

This looks good to me, but this patch alone just adds unused code.  It is in general not a good idea to add unused code.  If this could be combined with at least a single user or if the patch that will use this is already pending review, then no problem.

[Zhenyao Mo]

(In reply to comment #2)
> (From update of attachment 53871 [details])
> This looks good to me, but this patch alone just adds unused code.  It is in
> general not a good idea to add unused code.  If this could be combined with at
> least a single user or if the patch that will use this is already pending
> review, then no problem.

We are going to use this piece code in the following bug (and probably a few more)

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

[Kenneth Russell]

For what it's worth, I suggested separating this out into its own patch because I've received feedback on earlier patches that they should address independent issues. This particular entry point will shortly be used in several places throughout the WebKit WebGL code.

We do need to land the Chromium portion of this change first. Mo, perhaps remove the review? flag for the time being?

[Zhenyao Mo]

Chrome side patch already landed.

[WebKit Commit Bot]

Comment on attachment 53871 [details]
patch

Clearing flags on attachment: 53871

Committed r58140: <http://trac.webkit.org/changeset/58140>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

The isGLES2Compliant() method should be |const|

[Darin Fisher (:fishd, Google)]

(In reply to comment #8)
> The isGLES2Compliant() method should be |const|

Indeed.  Good catch, Simon!  @zmo: can you post a patch to fix that?

[Zhenyao Mo]

(In reply to comment #9)
> (In reply to comment #8)
> > The isGLES2Compliant() method should be |const|
> 
> Indeed.  Good catch, Simon!  @zmo: can you post a patch to fix that?

Will do.

[Zhenyao Mo]

Created attachment 54173 [details]
patch: make isGLES2Compliant() const

[Kenneth Russell]

Are you sure this compiles? Doesn't WebGraphicsContext3D::isGLES2Compliant need to become const as a consequence of this change?

[Zhenyao Mo]

(In reply to comment #12)
> Are you sure this compiles? Doesn't WebGraphicsContext3D::isGLES2Compliant need
> to become const as a consequence of this change?

It compiles fine on my Mac machine.  I was going to change it in WebGraphicsContext3D too, but noticed that methods there like width() and height() are not made const, so I felt it would be better to be consistent.

[Kenneth Russell]

It looks fine to me as long as it compiles, but I personally feel that there are so many methods on GraphicsContext3D that should otherwise be const that they should all be cleaned up in one shot.

[Darin Fisher (:fishd, Google)]

Comment on attachment 54173 [details]
patch: make isGLES2Compliant() const

If there are more that should be const, then let's convert those as well, but this change R=me.

[WebKit Commit Bot]

Comment on attachment 54173 [details]
patch: make isGLES2Compliant() const

Clearing flags on attachment: 54173

Committed r58209: <http://trac.webkit.org/changeset/58209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58209 might have broken SnowLeopard Intel Release (Tests)