Summary: | Add isGLES2Compliant to GraphicsContext3D | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, cmarrin, commit-queue, dglazkov, eric, fishd, kbr, oliver, simon.fraser, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Kenneth Russell
2010-04-20 11:07:48 PDT
Created attachment 53871 [details]
patch
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.
(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 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? Chrome side patch already landed. Comment on attachment 53871 [details] patch Clearing flags on attachment: 53871 Committed r58140: <http://trac.webkit.org/changeset/58140> All reviewed patches have been landed. Closing bug. The isGLES2Compliant() method should be |const| (In reply to comment #8) > The isGLES2Compliant() method should be |const| Indeed. Good catch, Simon! @zmo: can you post a patch to fix that? (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. Created attachment 54173 [details]
patch: make isGLES2Compliant() const
Are you sure this compiles? Doesn't WebGraphicsContext3D::isGLES2Compliant need to become const as a consequence of this change? (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. 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 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 on attachment 54173 [details] patch: make isGLES2Compliant() const Clearing flags on attachment: 54173 Committed r58209: <http://trac.webkit.org/changeset/58209> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/58209 might have broken SnowLeopard Intel Release (Tests) |