[chromium] Add GraphicsContext3DPrivate:createGraphicsContextForAnotherThread
Created attachment 106838 [details] Patch
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?
Created attachment 106877 [details] now with comments.
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 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.
Created attachment 106950 [details] ThreadUsage enum
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 on attachment 106950 [details] ThreadUsage enum Attachment 106950 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9625903
Created attachment 106968 [details] Make it work, plus tests.
Comment on attachment 106968 [details] Make it work, plus tests. Fantastic. r=me
Committed r94971: <http://trac.webkit.org/changeset/94971>