Bug 67832 - [chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread
Summary: [chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nat Duca
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-08 21:01 PDT by Nat Duca
Modified: 2011-09-12 12:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.76 KB, patch)
2011-09-08 21:22 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
now with comments. (13.75 KB, patch)
2011-09-09 08:50 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
ThreadUsage enum (13.90 KB, patch)
2011-09-09 18:38 PDT, Nat Duca
no flags Details | Formatted Diff | Diff
Make it work, plus tests. (15.70 KB, patch)
2011-09-09 23:46 PDT, Nat Duca
kbr: review+
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-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]
Patch
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>