Bug 66523 – [chromium] RenderView should register for WebGraphics3D callbacks

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

[Iain Merrick]

WebGraphics3DCommandBufferImpl has a WebView member variable. On certain events (context lost, swapBuffers started and finished) it extracts a RenderView from the WebView and calls methods on it. We'd like to get rid of that WebView. Instead, RenderView should register for callbacks with the WebGraphicsContext3D -- the hooks for this mostly exist already.

[Iain Merrick]

Created attachment 104442 [details]
First draft.

[Iain Merrick]

Chromium code is in http://codereview.chromium.org/7685001

[WebKit Review Bot]

Attachment 104442 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

I think we can do this with a 100% Chromium-side patch to render_view.cc... lets talk when you get in, and sorry in advance for any confusion that I caused on my part in underspecifying this change in the first place.

[Nat Duca]

Oh, snap, now I get it. We do need the posted callback, you're totally right and I'm majorly wrong. /me goes and drinks more coffee.

Can we get by without the changes to WebViewClient? For example, what if we make WebGraphicsContext3DCommandBufferImpl::initialize call through to RenderView directly with a "hey, you just had a context attached to you" sort of way? That would be safe in the threaded case too since ::initialize will be called on the main thread too and the RenderView can ignore it.

[Iain Merrick]

Created attachment 104516 [details]
Added bug URL to ChangeLog.

[Iain Merrick]

Created attachment 104518 [details]
Address Nat's comments

We don't need the extra WebViewClient method, so this is a lot simpler now -- just the extra callback.

[James Robinson]

Comment on attachment 104518 [details]
Address Nat's comments

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

Seems reasonable.  Where's the chromium side?

Given how many callbacks we have now (lost context, swapbuffers complete, swapbuffers posted, etc etc) I wonder if it's time to move to a more general interface for this sort of thing.

> WebKit/chromium/ChangeLog:18
> +        * public/WebGraphicsContext3D.h:
> +        (WebKit::WebGraphicsContext3D::WebGraphicsSwapBuffersPostedCallbackCHROMIUM::~WebGraphicsSwapBuffersPostedCallbackCHROMIUM):
> +        (WebKit::WebGraphicsContext3D::setSwapBuffersPostedCallbackCHROMIUM):
> +        * public/WebViewClient.h:
> +        (WebKit::WebViewClient::initCompositorContext):
> +        * src/WebViewImpl.cpp:
> +        (WebKit::WebViewImpl::createLayerTreeHostContext3D):
> +        (WebKit::WebViewImpl::graphicsContext3D):

This is out of date, please regenerate (or just edit).

[Nat Duca]

Iirw, I think we figured out a way to not need this patch at all. The "so many callbacks" was smelling funny to Iain and I as well, so we switched to making #ifdef'd calls from WGC3D to RenderView .

The long term fix for this is to move main-thread scheduling into the compositor as well. The main challenge I see there is plugin window moves and updateRects...

[Iain Merrick]

As Nat said, this bug is obsolete, but I forgot to close it out, sorry.