Merge WebGraphicsContext3D creation and initialization
Created attachment 123045 [details] Patch
webkit side of https://chromiumcodereview.appspot.com/9249048/ (chrome side needs to land first).
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 123045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123045&action=review This looks fine overall. Please request review again when you're ready to land it. You might roll Source/WebKit/chromium/DEPS in the same patch. > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:326 > virtual WebGraphicsContext3D* createGraphicsContext3D() { return 0; } Add a comment indicating that this variant is obsolete and is being removed.
Comment on attachment 123045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123045&action=review > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:322 > + virtual WebGraphicsContext3D* createGraphicsContext3D( WebKitPlatformSupport is not supposed to know about WebView. please don't add this parameter here. for that matter, WebGraphicsContext3D should not know about WebView either. we have a layering violation here :( one way to get around this problem is to introduce a creation function on WebViewClient. that way, you are creating a WebGraphicsContext3D at a higher level, and yet the WebGraphicsContext3D interface would not need to know about WebView directly.
Created attachment 123167 [details] Patch
PTAL, chrome side is there: https://chromiumcodereview.appspot.com/9254035/
Comment on attachment 123167 [details] Patch Looks fine to me, but as before, will need the Chromium side to land and Chromium DEPS rolled forward. Suggest rolling DEPS in the same patch. Leaving r? for Darin's comments.
Comment on attachment 123167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123167&action=review > Source/WebKit/chromium/public/WebViewClient.h:114 > + virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, WebView*, bool renderDirectlyToWebView) { return 0; } nit: no need for the WebView parameter since the WebViewClient knows what WebView it is associated with. (I presume you don't need to ask a RenderWidget to create a WGC3D for some other RenderWidget.) What does renderDirectlyToWebView mean? Maybe you could document that a bit? > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:159 > + OwnPtr<WebKit::WebGraphicsContext3D> webContext = adoptPtr(webViewImpl->client()->createGraphicsContext3D(webAttributes, webViewImpl, renderDirectlyToHostWindow)); In the future, we'll probably want to bridge up through an interface like HostWindow, but that doesn't have to happen now. You see, GraphicsContext3DChromium.cpp is technically part of WebCore/platform/, and as such it should not have direct access to the WebViewImpl either. But, please don't try to fix this in this patch :-) Otherwise, LGTM
Created attachment 123361 [details] Patch
(In reply to comment #9) > (From update of attachment 123167 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123167&action=review > > > Source/WebKit/chromium/public/WebViewClient.h:114 > > + virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, WebView*, bool renderDirectlyToWebView) { return 0; } > > nit: no need for the WebView parameter since the WebViewClient knows what WebView it is associated with. (I presume you don't need to ask a RenderWidget to create a WGC3D for some other RenderWidget.) Done. > > What does renderDirectlyToWebView mean? Maybe you could document that a bit? Done.
Comment on attachment 123361 [details] Patch LGTM for what it's worth. Still suggest waiting for the Chromium side to land and rolling forward Chromium DEPS in this patch.
Comment on attachment 123361 [details] Patch Attachment 123361 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11173709
Comment on attachment 123361 [details] Patch Rejecting attachment 123361 [details] from commit-queue. piman@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 123361 [details] Patch Rejecting attachment 123361 [details] from commit-queue. New failing tests: compositing/layer-creation/spanOverlapsCanvas.html Full output: http://queues.webkit.org/results/11177422
Comment on attachment 123361 [details] Patch I doubt that CQ failure is related to this patch. Let's try again.
Comment on attachment 123361 [details] Patch Rejecting attachment 123361 [details] from commit-queue. New failing tests: compositing/layer-creation/spanOverlapsCanvas.html Full output: http://queues.webkit.org/results/11314293
I definitely repro the failure without the patch (accelerated compositing doesn't turn on, when the expectation is that it does).
(In reply to comment #18) > I definitely repro the failure without the patch (accelerated compositing doesn't turn on, when the expectation is that it does). Oh, never mind, operator error. It does seem to be coming from this patch, I'll track it down.
Well, it turns out we're trying to create a graphics context without a HostWindow/WebView #0 WebCore::(anonymous namespace)::createGraphicsContext (attrs=..., hostWindow=0x0, renderStyle=WebCore::GraphicsContext3D::RenderOffscreen, threadUsage=WebCore::GraphicsContext3DPrivate::ForUseOnThisThread) at ../../../third_party/WebKit/Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:158 #1 0x00000000004f9efe in WebCore::GraphicsContext3D::create (attrs=..., hostWindow=0x0, renderStyle=WebCore::GraphicsContext3D::RenderOffscreen) at ../../../third_party/WebKit/Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:1044 #2 0x00000000007a68b1 in WebCore::SharedGraphicsContext3D::get () at ../../../third_party/WebKit/Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:41 #3 0x0000000000723a6d in WebCore::createAcceleratedCanvas (size=..., data=0x7fffea361a80) at ../../../third_party/WebKit/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:70 #4 0x0000000000724073 in WebCore::ImageBuffer::ImageBuffer (this=0x7fffea361a80, size=..., renderingMode=WebCore::Accelerated, success=@0x7fffffffaf5f) at ../../../third_party/WebKit/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:110 #5 0x00000000004cea58 in WebCore::ImageBuffer::create (size=..., colorSpace=WebCore::ColorSpaceDeviceRGB, renderingMode=WebCore::Accelerated) at ../../../third_party/WebKit/Source/WebCore/platform/graphics/ImageBuffer.h:78 #6 0x0000000001811e4d in WebCore::HTMLCanvasElement::createImageBuffer (this=0x7fffea1ddb40) at ../../../third_party/WebKit/Source/WebCore/html/HTMLCanvasElement.cpp:474 #7 0x00000000018120b0 in WebCore::HTMLCanvasElement::buffer (this=0x7fffea1ddb40) at ../../../third_party/WebKit/Source/WebCore/html/HTMLCanvasElement.cpp:510 #8 0x000000000181202a in WebCore::HTMLCanvasElement::drawingContext (this=0x7fffea1ddb40) at ../../../third_party/WebKit/Source/WebCore/html/HTMLCanvasElement.cpp:496 #9 0x00000000018a33f8 in WebCore::CanvasRenderingContext2D::drawingContext (this=0x7fffea227a80) at ../../../third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1828 #10 0x000000000189d26f in WebCore::CanvasRenderingContext2D::setFillStyle (this=0x7fffea227a80, prpStyle=...) at ../../../third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:378 #11 0x000000000189ea42 in WebCore::CanvasRenderingContext2D::setFillColor (this=0x7fffea227a80, color=...) at ../../../third_party/WebKit/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:727 #12 0x00000000011704e6 in WebCore::V8CanvasRenderingContext2D::fillStyleAccessorSetter (name=..., value=..., info=...) at ../../../third_party/WebKit/Source/WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp:99 #13 0x0000000000eaa338 in v8::internal::JSObject::SetPropertyWithCallback (this=0x1c75fcf9daa1, structure=0x1194d6b5221, name=0x2d2041e1f031, value=0x2d2041e1f059, holder=0x1c75fcf9daa1, strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:2038 #14 0x0000000000eadcae in v8::internal::JSObject::SetPropertyForResult (this=0x1c75fcf9daa1, result=0x7fffffffb4d0, name=0x2d2041e1f031, value=0x2d2041e1f059, attributes=NONE, strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:3011 #15 0x0000000000eac279 in v8::internal::JSReceiver::SetProperty (this=0x1c75fcf9daa1, result=0x7fffffffb4d0, key=0x2d2041e1f031, value=0x2d2041e1f059, attributes=NONE, strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:2655 #16 0x0000000000eaa044 in v8::internal::JSReceiver::SetProperty (this=0x1c75fcf9daa1, name=0x2d2041e1f031, value=0x2d2041e1f059, attributes=NONE, strict_mode=v8::internal::kNonStrictMode) at ../../../v8/src/objects.cc:1993 #17 0x00000000010d271c in v8::internal::StoreIC::Store (this=0x7fffffffb640, state=v8::internal::UNINITIALIZED, strict_mode=v8::internal::kNonStrictMode, object=..., name=..., value=...) at ../../../v8/src/ic.cc:1329 #18 0x00000000010d4f9e in v8::internal::StoreIC_Miss (args=..., isolate=0x7ffff7e42000) at ../../../v8/src/ic.cc:1881 Any idea?
jamesr or senorblanco should know that code path.
(In reply to comment #21) > jamesr or senorblanco should know that code path. Yeah, I made that work back in http://trac.webkit.org/changeset/93157. This was necessary in order to unify our accelerated ImageBuffer creation with the CoreGraphics path, where it only needs to pass down an enum, rather than the platform-specific crud we were doing before. In order to do that, I had to avoid passing down the Page-related stuff (WebViewImpl I think) so I allowed it to be NULL. This works fine for offscreen contexts like <canvas>, just not for onscreen ones (where the WebViewImpl must not be NULL). I haven't looked at this patch in detail, but It would be really unfortunate to lose this ability and to have to go back to platform-specific hacks.
Darin: what do you think? 2D Canvas needs a shared offscreen context that isn't tied to a given WebView, so we don't have a WebViewClient to use as a factory. We can add back a createGraphicsContext3D on the WebKitPlatformSupport, that would only be used to create/initialize an offscreen context, that we would go to when we don't have a WebView (hence, no need for it as a parameter). Would that solution be acceptable to you?
(In reply to comment #23) > Darin: what do you think? 2D Canvas needs a shared offscreen context that isn't tied to a given WebView, so we don't have a WebViewClient to use as a factory. We can add back a createGraphicsContext3D on the WebKitPlatformSupport, that would only be used to create/initialize an offscreen context, that we would go to when we don't have a WebView (hence, no need for it as a parameter). Would that solution be acceptable to you? Yes. I was going to only suggest that you name it createOffscreenGraphicsContext3D to emphasize the point.
Created attachment 124416 [details] Patch
I implemented that. Chrome side at https://chromiumcodereview.appspot.com/9297046
Comment on attachment 124416 [details] Patch Attachment 124416 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11358387 New failing tests: compositing/layer-creation/spanOverlapsCanvas.html
Comment on attachment 124416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124416&action=review > Source/WebKit/chromium/public/WebViewClient.h:117 > + virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, bool renderDirectlyToWebView) { return 0; } when would you pass false as the second parameter here instead of just calling createOffscreen...() ?
(In reply to comment #28) > (From update of attachment 124416 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124416&action=review > > > Source/WebKit/chromium/public/WebViewClient.h:117 > > + virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes, bool renderDirectlyToWebView) { return 0; } > > when would you pass false as the second parameter here instead of just calling createOffscreen...() ? Today, it's called for WebGL contexts, which do have a WebView. createOffscreenGraphicsContext3D is only called for the hackish "shared context", and relies on implicit (rather than explicit) context sharing, that actually shares cross-pages. I wish at some point we can have the shared context be page-specific and go through WebViewClient::createGraphicsContext3D instead, but that seems unlikely in the short term. We could make WebGL contexts go through createOffscreenGraphicsContext3D and it'd probably work, but unless you feel strongly about this, I'd rather not go through yet another round of 2-sided changes to remove the bool: this API looks like what you'd want long term anyway.
But why do WebGL contexts have to be associated with a WebView anyway? A given WebGL context might render to any number of different WebViews over its lifetime (if its owning <canvas> is moved into a popup window by JS, for example). I don't think that conceptually any offscreen context should be associated with a given Page or WebView since that association can change at any time.
Created attachment 124601 [details] Patch
New patch up that removes the "renderDirectlyToWebView parameter". offscreen contexts for webgl go through the WebKitPlatformSupport path.
Comment on attachment 124601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124601&action=review > Source/WebKit/chromium/public/WebViewClient.h:117 > + virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes) { return 0; } nit: "const ref" like the one on WebKitPlatformSupport? > Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:132 > + virtual bool initialize(Attributes, WebView*, bool renderDirectlyToWebView) { return false; } nit: this takes Attributes too? is it needed? if needed, should it also be pass by "const ref"? > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:158 > + Chrome* chrome = static_cast<Chrome*>(hostWindow); indentation
Comment on attachment 124601 [details] Patch Attachment 124601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11373026
Created attachment 124617 [details] Patch
Note: As discussed by chat, today some implementations rely on having the WebView even for offscreen contexts (e.g. WebGraphicsContext3DCommandBufferImpl to grab the URL to tell the GPU process, WebGraphicsContext3DInProcessImpl to properly share contexts), and this CL isn't intended to change semantics there, so I reverted to the previous version with the explicit "renderDirectlyToWebView" (aka !offscreen) parameter. A follow-up CL can clean this up. (In reply to comment #33) > (From update of attachment 124601 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124601&action=review > > > Source/WebKit/chromium/public/WebViewClient.h:117 > > + virtual WebGraphicsContext3D* createGraphicsContext3D(WebGraphicsContext3D::Attributes) { return 0; } > > nit: "const ref" like the one on WebKitPlatformSupport? Done. > > > Source/WebKit/chromium/public/platform/WebGraphicsContext3D.h:132 > > + virtual bool initialize(Attributes, WebView*, bool renderDirectlyToWebView) { return false; } > > nit: this takes Attributes too? is it needed? if needed, should it also be pass by "const ref"? This function will go away once the chrome side refactoring is done. I noted in the doc as such. There's no point in fixing this now (it'd require extra 2-sided changes). > > > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:158 > > + Chrome* chrome = static_cast<Chrome*>(hostWindow); > > indentation Because of the revert to previous version, the problem is gone.
Comment on attachment 124617 [details] Patch Attachment 124617 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11367741 New failing tests: compositing/layer-creation/spanOverlapsCanvas.html
LGTM
LGTM too. Per offline discussion, sounds like another patch is coming which will roll Chromium DEPS too to fix the build failure.
Comment on attachment 124617 [details] Patch Clearing flags on attachment: 124617 Committed r106371: <http://trac.webkit.org/changeset/106371>
All reviewed patches have been landed. Closing bug.
This patch broke Chromium builds: http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Perf%29/builds/132/steps/compile-webkit/logs/stdio
This needs a roll of Source/WebKit/chromium/DEPS
Not a DEPS roll issue, just I removed a method that is OVERRIDE chrome-side. I temporarily put it back in https://bugs.webkit.org/show_bug.cgi?id=77467 and it should have fixed the build.
(In reply to comment #44) > Not a DEPS roll issue, just I removed a method that is OVERRIDE chrome-side. I temporarily put it back in https://bugs.webkit.org/show_bug.cgi?id=77467 and it should have fixed the build. Gah, this is why we should never OVERRIDE implementations of WebKit APIs. I'll look in to having the style checker enforce that OVERRIDE is banned for these.