Bug 59626 – [chromium] Add swapBuffersCompleteCallback to Extensions3DChromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Add swapBuffersCompleteCallback to Extensions3DChromium

[Nat Duca]

Created attachment 91325 [details]
Patch

[Kenneth Russell]

Please provide more description in the bug report about why this functionality is being added.

[Kenneth Russell]

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?

[Nat Duca]

> 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.

[Nat Duca]

Created attachment 91426 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 91426 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8516315

[Nat Duca]

Created attachment 91427 [details]
Ah, thats why extensions3dchromium was included.

[WebKit Review Bot]

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.

[Kenneth Russell]

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.

[Nat Duca]

Created attachment 91573 [details]
Patch

[Nat Duca]

Le sigh...
nduca:~/Local/chrome/src/third_party/WebKit $ check-webkit-style 
Total errors found: 0 in 7 files

Git issue? Ugh.

[Kenneth Russell]

Comment on attachment 91573 [details]
Patch

Fantastic. Thanks.

[WebKit Commit Bot]

Comment on attachment 91573 [details]
Patch

Clearing flags on attachment: 91573

Committed r85292: <http://trac.webkit.org/changeset/85292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85292 might have broken Windows 7 Release (Tests)

[David Levin]

(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.)