[chromium] Add swapBuffersCompleteCallback to Extensions3DChromium
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.)