Bug 67832

Summary: [chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread
Product: WebKit Reporter: Nat Duca <nduca>
Component: New BugsAssignee: Nat Duca <nduca>
Severity: Normal CC: dglazkov, husky, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
now with comments.
ThreadUsage enum
Make it work, plus tests. kbr: review+

Description Nat Duca 2011-09-08 21:01:47 PDT
[chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread
Comment 1 Nat Duca 2011-09-08 21:22:17 PDT
Created attachment 106838 [details]
Comment 2 Iain Merrick 2011-09-09 03:40:20 PDT
Very nice overall.

> Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:60
> +    // Used to create a GraphicsContext3D for use on a thread. Will skip making

Super picky: this first sentence is actually less useful than the name of the method.

Maybe ditch it, but add a comment to create() to emphasise that the new context must be used on *this* thread?
Comment 3 Nat Duca 2011-09-09 08:50:55 PDT
Created attachment 106877 [details]
now with comments.
Comment 4 Kenneth Russell 2011-09-09 18:22:23 PDT
Comment on attachment 106877 [details]
now with comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=106877&action=review

Overall this looks good but one small thing needs cleanup before landing.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:122
> +PassRefPtr<GraphicsContext3D> GraphicsContext3DPrivate::createGraphicsContextFromWebContext(PassOwnPtr<WebKit::WebGraphicsContext3D> webContext, GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, GraphicsContext3D::RenderStyle renderStyle, bool forUseOnAnotherThread)

WebKit style strongly discourages the use of bool arguments because they aren't self-describing at the call site. Please define a little enum in GraphicsContext3DPrivate.h like ThreadUsage { ForUseOnThisThread, ForUseOnAnotherThread } and pass that instead.
Comment 5 Kenneth Russell 2011-09-09 18:28:51 PDT
Comment on attachment 106877 [details]
now with comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=106877&action=review

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:165
> +    return GraphicsContext3DPrivate::createGraphicsContextFromWebContext(webContext.release(), attrs, hostWindow, renderStyle, false);

Did you mean to pass "forUseOnAnotherThread" instead of "false" here? That would be yet another reason to use an enum.
Comment 6 Nat Duca 2011-09-09 18:38:35 PDT
Created attachment 106950 [details]
ThreadUsage enum
Comment 7 Kenneth Russell 2011-09-09 18:56:25 PDT
Comment on attachment 106950 [details]
ThreadUsage enum

View in context: https://bugs.webkit.org/attachment.cgi?id=106950&action=review

Thanks, looks good but does this actually compile?

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1026
> +    return createGraphicsContext(attrs, hostWindow, renderStyle, ForUseOnThisThread);

Does this compile or do you need to say GraphicsContext3DPrivate::ForUseOnThisThread?

> Source/WebKit/chromium/tests/MockGraphicsContext3DTest.cpp:56
> +    RefPtr<GraphicsContext3D> context = GraphicsContext3DPrivate::createGraphicsContextFromWebContext(adoptPtr(new FrameCountingContext()), attrs, 0, GraphicsContext3D::RenderDirectlyToHostWindow, false);

Does this compile?

> Source/WebKit/chromium/tests/MockGraphicsContext3DTest.cpp:78
> +    RefPtr<GraphicsContext3D> context = GraphicsContext3DPrivate::createGraphicsContextFromWebContext(adoptPtr(new GMockContext()), attrs, 0, GraphicsContext3D::RenderDirectlyToHostWindow, false);

Same question here and below.
Comment 8 WebKit Review Bot 2011-09-09 19:53:51 PDT
Comment on attachment 106950 [details]
ThreadUsage enum

Attachment 106950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9625903
Comment 9 Nat Duca 2011-09-09 23:46:10 PDT
Created attachment 106968 [details]
Make it work, plus tests.
Comment 10 Kenneth Russell 2011-09-10 14:15:11 PDT
Comment on attachment 106968 [details]
Make it work, plus tests.

Fantastic. r=me
Comment 11 Nat Duca 2011-09-12 12:52:49 PDT
Committed r94971: <http://trac.webkit.org/changeset/94971>