Bug 59626 - [chromium] Add swapBuffersCompleteCallback to Extensions3DChromium
Summary: [chromium] Add swapBuffersCompleteCallback to Extensions3DChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-27 12:37 PDT by Nat Duca
Modified: 2011-04-28 22:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.83 KB, patch)
2011-04-27 12:38 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2011-04-27 22:00 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Ah, thats why extensions3dchromium was included. (9.67 KB, patch)
2011-04-27 22:06 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2011-04-28 15:54 PDT, Nat Duca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-04-27 12:37:50 PDT
[chromium] Add swapBuffersCompleteCallback to Extensions3DChromium
Comment 1 Nat Duca 2011-04-27 12:38:34 PDT
Created attachment 91325 [details]
Patch
Comment 2 Kenneth Russell 2011-04-27 18:53:11 PDT
Please provide more description in the bug report about why this functionality is being added.
Comment 3 Kenneth Russell 2011-04-27 19:07:22 PDT
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?
Comment 4 Nat Duca 2011-04-27 21:59:22 PDT
> 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.
Comment 5 Nat Duca 2011-04-27 22:00:53 PDT
Created attachment 91426 [details]
Patch
Comment 6 WebKit Review Bot 2011-04-27 22:03:38 PDT
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.
Comment 7 WebKit Review Bot 2011-04-27 22:05:47 PDT
Attachment 91426 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8516315
Comment 8 Nat Duca 2011-04-27 22:06:00 PDT
Created attachment 91427 [details]
Ah, thats why extensions3dchromium was included.
Comment 9 WebKit Review Bot 2011-04-27 22:08:22 PDT
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 10 Kenneth Russell 2011-04-28 15:24:34 PDT
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.
Comment 11 Nat Duca 2011-04-28 15:54:51 PDT
Created attachment 91573 [details]
Patch
Comment 12 Nat Duca 2011-04-28 15:55:11 PDT
Le sigh...
nduca:~/Local/chrome/src/third_party/WebKit $ check-webkit-style 
Total errors found: 0 in 7 files

Git issue? Ugh.
Comment 13 Kenneth Russell 2011-04-28 15:57:27 PDT
Comment on attachment 91573 [details]
Patch

Fantastic. Thanks.
Comment 14 WebKit Commit Bot 2011-04-28 20:19:43 PDT
Comment on attachment 91573 [details]
Patch

Clearing flags on attachment: 91573

Committed r85292: <http://trac.webkit.org/changeset/85292>
Comment 15 WebKit Commit Bot 2011-04-28 20:19:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2011-04-28 20:52:51 PDT
http://trac.webkit.org/changeset/85292 might have broken Windows 7 Release (Tests)
Comment 17 David Levin 2011-04-28 22:24:20 PDT
(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.)