Bug 67832 – [chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread

[Nat Duca]

Created attachment 106838 [details]
Patch

[Iain Merrick]

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?

[Nat Duca]

Created attachment 106877 [details]
now with comments.

[Kenneth Russell]

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.

[Kenneth Russell]

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.

[Nat Duca]

Created attachment 106950 [details]
ThreadUsage enum

[Kenneth Russell]

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.

[WebKit Review Bot]

Comment on attachment 106950 [details]
ThreadUsage enum

Attachment 106950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9625903

[Nat Duca]

Created attachment 106968 [details]
Make it work, plus tests.

[Kenneth Russell]

Comment on attachment 106968 [details]
Make it work, plus tests.

Fantastic. r=me

[Nat Duca]

Committed r94971: <http://trac.webkit.org/changeset/94971>