Bug 37872 - Add isGLES2Compliant to GraphicsContext3D
Summary: Add isGLES2Compliant to GraphicsContext3D
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-20 11:07 PDT by Kenneth Russell
Modified: 2010-04-23 22:52 PDT (History)
10 users (show)

See Also:


Attachments
patch (6.58 KB, patch)
2010-04-20 13:21 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
patch: make isGLES2Compliant() const (3.90 KB, patch)
2010-04-23 10:43 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-04-20 11:07:48 PDT
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.
Comment 1 Zhenyao Mo 2010-04-20 13:21:12 PDT
Created attachment 53871 [details]
patch
Comment 2 Darin Fisher (:fishd, Google) 2010-04-21 14:55:54 PDT
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.
Comment 3 Zhenyao Mo 2010-04-21 15:00:04 PDT
(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
Comment 4 Kenneth Russell 2010-04-21 18:00:08 PDT
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?
Comment 5 Zhenyao Mo 2010-04-22 10:27:53 PDT
Chrome side patch already landed.
Comment 6 WebKit Commit Bot 2010-04-22 19:07:16 PDT
Comment on attachment 53871 [details]
patch

Clearing flags on attachment: 53871

Committed r58140: <http://trac.webkit.org/changeset/58140>
Comment 7 WebKit Commit Bot 2010-04-22 19:07:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Simon Fraser (smfr) 2010-04-22 19:10:05 PDT
The isGLES2Compliant() method should be |const|
Comment 9 Darin Fisher (:fishd, Google) 2010-04-23 00:07:49 PDT
(In reply to comment #8)
> The isGLES2Compliant() method should be |const|

Indeed.  Good catch, Simon!  @zmo: can you post a patch to fix that?
Comment 10 Zhenyao Mo 2010-04-23 10:13:29 PDT
(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.
Comment 11 Zhenyao Mo 2010-04-23 10:43:22 PDT
Created attachment 54173 [details]
patch: make isGLES2Compliant() const
Comment 12 Kenneth Russell 2010-04-23 10:54:25 PDT
Are you sure this compiles? Doesn't WebGraphicsContext3D::isGLES2Compliant need to become const as a consequence of this change?
Comment 13 Zhenyao Mo 2010-04-23 11:01:17 PDT
(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.
Comment 14 Kenneth Russell 2010-04-23 11:09:42 PDT
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.
Comment 15 Darin Fisher (:fishd, Google) 2010-04-23 22:01:36 PDT
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.
Comment 16 WebKit Commit Bot 2010-04-23 22:17:27 PDT
Comment on attachment 54173 [details]
patch: make isGLES2Compliant() const

Clearing flags on attachment: 54173

Committed r58209: <http://trac.webkit.org/changeset/58209>
Comment 17 WebKit Commit Bot 2010-04-23 22:17:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2010-04-23 22:52:27 PDT
http://trac.webkit.org/changeset/58209 might have broken SnowLeopard Intel Release (Tests)