Summary: | [chromium] Add swapBuffersCompleteCallback to Extensions3DChromium | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nat Duca <nduca> | ||||||||||
Component: | New Bugs | Assignee: | Nat Duca <nduca> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, eric, kbr, levin, nduca, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Nat Duca
2011-04-27 12:37:50 PDT
Created attachment 91325 [details]
Patch
Please provide more description in the bug report about why this functionality is being added. Comment on attachment 91325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91325&action=review A few questions. Not touching the review bit yet. > Source/WebKit/chromium/public/WebGraphicsContext3D.h:65 > +#define WEBGRAPHICSCONTEXT3D_HAS_SWAPBUFFERS_COMPLETE_EXTENSION Is this being added just so that you can land the Chromium side patch before this one? If so, please remove it -- we wait for the WebKit change to land and roll before committing the downstream code. Otherwise the code would be littered with such #ifdefs. > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1213 > + m_impl->setSwapBuffersCompleteCallbackCHROMIUM(m_swapBuffersCompleteCallbackAdapter.get()); For safety during teardown, should we explicitly set m_impl's swap buffers callback to 0 in the destructor of GraphicsContext3DInternal? > Source/WebKit/chromium/src/WebViewImpl.cpp:56 > +#include "Extensions3DChromium.h" Why is this needed if there are no other code changes in this file? > A few questions. Not touching the review bit yet. > > > Source/WebKit/chromium/public/WebGraphicsContext3D.h:65 > > +#define WEBGRAPHICSCONTEXT3D_HAS_SWAPBUFFERS_COMPLETE_EXTENSION Yanked. >setSwapBuffersCompleteCallbackCHROMIUM(m_swapBuffersCompleteCallbackAdapter.get(> For safety during teardown, should we explicitly set m_impl's swap buffers callback to 0 in the destructor of GraphicsContext3DInternal? I'm fine doing that, but e.g., ContextLostCallback, doesn't do that. I tend to agree with your suggestion so my upcoming patch reflects that. Created attachment 91426 [details]
Patch
Attachment 91426 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1193: Missing space inside { }. [whitespace/braces] [5]
Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1195: Missing space inside { }. [whitespace/braces] [5]
Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:84: Missing space inside { }. [whitespace/braces] [5]
Source/WebKit/chromium/public/WebGraphicsContext3D.h:108: Missing space inside { }. [whitespace/braces] [5]
Source/WebKit/chromium/public/WebGraphicsContext3D.h:184: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 5 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 91426 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8516315 Created attachment 91427 [details]
Ah, thats why extensions3dchromium was included.
Attachment 91427 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1193: Missing space inside { }. [whitespace/braces] [5]
Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1195: Missing space inside { }. [whitespace/braces] [5]
Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:84: Missing space inside { }. [whitespace/braces] [5]
Source/WebKit/chromium/public/WebGraphicsContext3D.h:108: Missing space inside { }. [whitespace/braces] [5]
Source/WebKit/chromium/public/WebGraphicsContext3D.h:184: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 5 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 91427 [details]
Ah, thats why extensions3dchromium was included.
Looks good but could you please clean up the style issues? Note that "webkit-patch upload" will run the style checker before upload and catch these issues before the bots do.
Created attachment 91573 [details]
Patch
Le sigh... nduca:~/Local/chrome/src/third_party/WebKit $ check-webkit-style Total errors found: 0 in 7 files Git issue? Ugh. Comment on attachment 91573 [details]
Patch
Fantastic. Thanks.
Comment on attachment 91573 [details] Patch Clearing flags on attachment: 91573 Committed r85292: <http://trac.webkit.org/changeset/85292> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/85292 might have broken Windows 7 Release (Tests) (In reply to comment #12) > Le sigh... > nduca:~/Local/chrome/src/third_party/WebKit $ check-webkit-style > Total errors found: 0 in 7 files > > Git issue? Ugh. (Very) recently added check. (You may not have had it.) |