Provide shared context to Platform API in renderer.

Provide shared context to Platform API in renderer.

Implements the WebKit::Platform::sharedOffscreenGraphicsContext3D API
in the renderer, and provide the same shared context to WebKit and the
compositor when the compositor is not threaded.

RenderWidgetCompositor and RendererWebKitPlatformSupport both access
the shared context through RenderThreadImpl who creates them.

R=piman,jamesr
BUG=177768
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186342

[danakj, 2013-02-09 06:16:57]

Here's the renderer follow up for https://codereview.chromium.org/12212007/.

It is built on top of that as well as https://codereview.chromium.org/12217099/
(the webkit TestWKPlatformSupport stuff).

If we want, we could land this first, but then compositor's shared context would
not be the same as WebKit's until the WebKit side landed, but it would avoid the
3-sidedness we'll have to do otherwise. If we land the webkit/ and then the
WebKit stuff first, we can drop the #ifdef stuff from this CL.

[piman, 2013-02-11 19:15:30]

https://codereview.chromium.org/12212100/diff/4001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/12212100/diff/4001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:287: : shared_context_main_thread_(this),
nit: you'll need the ALLOW_THIS_IN_INITIALIZER_LIST wrapper for MSVC. (here and
below)

https://codereview.chromium.org/12212100/diff/4001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:1168:
scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context(
use WebGraphicsContext3DCommandBufferImpl::CreateOffscreenContext ?

[danakj, 2013-02-11 20:53:36]

https://codereview.chromium.org/12212100/diff/4001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/12212100/diff/4001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:287: : shared_context_main_thread_(this),
On 2013/02/11 19:15:30, piman wrote:
> nit: you'll need the ALLOW_THIS_IN_INITIALIZER_LIST wrapper for MSVC. (here
and
> below)

Done.

https://codereview.chromium.org/12212100/diff/4001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:1168:
scoped_ptr<WebGraphicsContext3DCommandBufferImpl> context(
On 2013/02/11 19:15:30, piman wrote:
> use WebGraphicsContext3DCommandBufferImpl::CreateOffscreenContext ?

Oh, nice. Done.

[piman, 2013-02-11 21:03:24]

lgtm

[jamesr, 2013-02-12 03:10:58]

lgtm2

https://codereview.chromium.org/12212100/diff/5010/content/browser/in_process...
File content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc
(right):

https://codereview.chromium.org/12212100/diff/5010/content/browser/in_process...
content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc:127:
NOTREACHED();
do you need to override these?  Why not just use the base class implementation?

[danakj, 2013-02-12 03:15:46]

https://codereview.chromium.org/12212100/diff/5010/content/browser/in_process...
File content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc
(right):

https://codereview.chromium.org/12212100/diff/5010/content/browser/in_process...
content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc:127:
NOTREACHED();
On 2013/02/12 03:10:59, jamesr wrote:
> do you need to override these?  Why not just use the base class
implementation?

I deleted it there so this is the only implementation, since the browser doesn't
do WebGL.

[danakj, 2013-02-13 08:28:28]

I've reworked the RenderThreadImpl class without the use of the OffscreenContext
class, to match what ImageTransportFactory is doing, and verified that it can
recover from context loss and all.

Moved the Memory(Allocation)CallbackProxy out from ImageTransportFactory's
implementation so that we could share it here.

[danakj, 2013-03-05 21:15:52]

PTAL.

This implements the cc::ContextProvider interface stuff for the renderer process
in RenderThreadImpl. With this, WebSharedContext3D can be removed.

[jamesr, 2013-03-06 00:26:10]

lgtm

https://codereview.chromium.org/12212100/diff/33002/content/browser/in_proces...
File content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc
(right):

https://codereview.chromium.org/12212100/diff/33002/content/browser/in_proces...
content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc:127:
return NULL;
Why bother overriding this?  Isn't there a default null-returning implementation
somewhere further up the inheritance chain?

[danakj, 2013-03-06 00:27:10]

https://codereview.chromium.org/12212100/diff/33002/content/browser/in_proces...
File content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc
(right):

https://codereview.chromium.org/12212100/diff/33002/content/browser/in_proces...
content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc:127:
return NULL;
On 2013/03/06 00:26:10, jamesr wrote:
> Why bother overriding this?  Isn't there a default null-returning
implementation
> somewhere further up the inheritance chain?

There is, in the Platform class. This just adds a NOTREACHED() so we know
browser-side shinanigans are happening. I can remove it if you like?

[jamesr, 2013-03-06 00:30:40]

On 2013/03/06 00:27:10, danakj wrote:
>
https://codereview.chromium.org/12212100/diff/33002/content/browser/in_proces...
> File content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc
> (right):
> 
>
https://codereview.chromium.org/12212100/diff/33002/content/browser/in_proces...
> content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc:127:
> return NULL;
> On 2013/03/06 00:26:10, jamesr wrote:
> > Why bother overriding this?  Isn't there a default null-returning
> implementation
> > somewhere further up the inheritance chain?
> 
> There is, in the Platform class. This just adds a NOTREACHED() so we know
> browser-side shinanigans are happening. I can remove it if you like?

Yeah, I'd prefer to not clutter up the different overrides of
WebKitPlatformSupportImpl with functions that we don't expect to be called.  The
interface is huge and large portions aren't relevant for many implementations. 
Otherwise every subclass ends up being a huge block of NOTREACHED()s

[danakj, 2013-03-06 00:32:24]

On Tue, Mar 5, 2013 at 7:30 PM, <jamesr@chromium.org> wrote:

> On 2013/03/06 00:27:10, danakj wrote:
>
> https://codereview.chromium.**org/12212100/diff/33002/**
> content/browser/in_process_**webkit/browser_**
>
webkitplatformsupport_impl.cc<https://codereview.chromium.org/12212100/diff/33002/content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc>
>
>> File content/browser/in_process_**webkit/browser_**
>> webkitplatformsupport_impl.cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/12212100/diff/33002/**
> content/browser/in_process_**webkit/browser_**
>
webkitplatformsupport_impl.cc#**newcode127<https://codereview.chromium.org/12212100/diff/33002/content/browser/in_process_webkit/browser_webkitplatformsupport_impl.cc#newcode127>
>
>> content/browser/in_process_**webkit/browser_**
>> webkitplatformsupport_impl.cc:**127:
>> return NULL;
>> On 2013/03/06 00:26:10, jamesr wrote:
>> > Why bother overriding this?  Isn't there a default null-returning
>> implementation
>> > somewhere further up the inheritance chain?
>>
>
>  There is, in the Platform class. This just adds a NOTREACHED() so we know
>> browser-side shinanigans are happening. I can remove it if you like?
>>
>
> Yeah, I'd prefer to not clutter up the different overrides of
> WebKitPlatformSupportImpl with functions that we don't expect to be
> called.  The
> interface is huge and large portions aren't relevant for many
> implementations.
> Otherwise every subclass ends up being a huge block of NOTREACHED()s
>

k, thanks. done!


>
>
https://codereview.chromium.**org/12212100/<https://codereview.chromium.org/1...
>

[piman, 2013-03-06 01:08:59]

lgtm

https://codereview.chromium.org/12212100/diff/49001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212100/diff/49001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:337: protected:
nit: blank line before 'protected:'

[danakj, 2013-03-06 01:41:32]

Thanks!

https://codereview.chromium.org/12212100/diff/49001/content/browser/renderer_...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12212100/diff/49001/content/browser/renderer_...
content/browser/renderer_host/compositor_impl_android.cc:337: protected:
On 2013/03/06 01:08:59, piman wrote:
> nit: blank line before 'protected:'

Done.

[commit-bot: I haz the power, 2013-03-06 01:44:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12212100/55001

[commit-bot: I haz the power, 2013-03-06 01:44:44]

Failed to apply patch for
content/renderer/renderer_webkitplatformsupport_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/renderer_webkitplatformsupport_impl.h
  Hunk #2 succeeded at 105 (offset 1 line).
  Hunk #3 succeeded at 122 (offset 1 line).
  Hunk #4 FAILED at 159.
  1 out of 4 hunks FAILED -- saving rejects to file
content/renderer/renderer_webkitplatformsupport_impl.h.rej

Patch:       content/renderer/renderer_webkitplatformsupport_impl.h
Index: content/renderer/renderer_webkitplatformsupport_impl.h
diff --git a/content/renderer/renderer_webkitplatformsupport_impl.h
b/content/renderer/renderer_webkitplatformsupport_impl.h
index
e6f1eef5ce817be3ccdaa25273bbddb1acc7aa82..530e1a0d3e0d1de22a7b381529c5f76b85a0eb30
100644
--- a/content/renderer/renderer_webkitplatformsupport_impl.h
+++ b/content/renderer/renderer_webkitplatformsupport_impl.h
@@ -14,6 +14,10 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebIDBFactory.h"
 #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSharedWorkerRepository.h"
 
+namespace cc {
+class ContextProvider;
+}
+
 namespace webkit_glue {
 class WebClipboardImpl;
 }
@@ -100,6 +104,10 @@ class CONTENT_EXPORT RendererWebKitPlatformSupportImpl
       WebKit::WebMediaStreamCenterClient* client);
   virtual bool processMemorySizesInBytes(
       size_t* private_bytes, size_t* shared_bytes);
+  virtual WebKit::WebGraphicsContext3D* createOffscreenGraphicsContext3D(
+      const WebKit::WebGraphicsContext3D::Attributes& attributes);
+  virtual WebKit::WebGraphicsContext3D* sharedOffscreenGraphicsContext3D();
+  virtual GrContext* sharedOffscreenGrContext();
 
   // Disables the WebSandboxSupport implementation for testing.
   // Tests that do not set up a full sandbox environment should call
@@ -113,9 +121,6 @@ class CONTENT_EXPORT RendererWebKitPlatformSupportImpl
   // Set WebGamepads to return when sampleGamepads() is invoked.
   static void SetMockGamepadsForTesting(const WebKit::WebGamepads& pads);
 
- protected:
-  virtual GpuChannelHostFactory* GetGpuChannelHostFactory() OVERRIDE;
-
  private:
   bool CheckPreparsedJsCachingEnabled() const;
 
@@ -154,6 +159,8 @@ class CONTENT_EXPORT RendererWebKitPlatformSupportImpl
   scoped_ptr<WebKit::WebBlobRegistry> blob_registry_;
 
   scoped_ptr<GamepadSharedMemoryReader> gamepad_shared_memory_reader_;
+
+  scoped_refptr<cc::ContextProvider> shared_offscreen_context_;
 };
 
 }  // namespace content

[commit-bot: I haz the power, 2013-03-06 01:48:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12212100/58001

[commit-bot: I haz the power, 2013-03-06 01:48:22]

Failed to apply patch for
content/renderer/renderer_webkitplatformsupport_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/renderer_webkitplatformsupport_impl.h
  Hunk #2 succeeded at 105 (offset 1 line).
  Hunk #3 succeeded at 122 (offset 1 line).
  Hunk #4 FAILED at 159.
  1 out of 4 hunks FAILED -- saving rejects to file
content/renderer/renderer_webkitplatformsupport_impl.h.rej

Patch:       content/renderer/renderer_webkitplatformsupport_impl.h
Index: content/renderer/renderer_webkitplatformsupport_impl.h
diff --git a/content/renderer/renderer_webkitplatformsupport_impl.h
b/content/renderer/renderer_webkitplatformsupport_impl.h
index
e6f1eef5ce817be3ccdaa25273bbddb1acc7aa82..530e1a0d3e0d1de22a7b381529c5f76b85a0eb30
100644
--- a/content/renderer/renderer_webkitplatformsupport_impl.h
+++ b/content/renderer/renderer_webkitplatformsupport_impl.h
@@ -14,6 +14,10 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebIDBFactory.h"
 #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSharedWorkerRepository.h"
 
+namespace cc {
+class ContextProvider;
+}
+
 namespace webkit_glue {
 class WebClipboardImpl;
 }
@@ -100,6 +104,10 @@ class CONTENT_EXPORT RendererWebKitPlatformSupportImpl
       WebKit::WebMediaStreamCenterClient* client);
   virtual bool processMemorySizesInBytes(
       size_t* private_bytes, size_t* shared_bytes);
+  virtual WebKit::WebGraphicsContext3D* createOffscreenGraphicsContext3D(
+      const WebKit::WebGraphicsContext3D::Attributes& attributes);
+  virtual WebKit::WebGraphicsContext3D* sharedOffscreenGraphicsContext3D();
+  virtual GrContext* sharedOffscreenGrContext();
 
   // Disables the WebSandboxSupport implementation for testing.
   // Tests that do not set up a full sandbox environment should call
@@ -113,9 +121,6 @@ class CONTENT_EXPORT RendererWebKitPlatformSupportImpl
   // Set WebGamepads to return when sampleGamepads() is invoked.
   static void SetMockGamepadsForTesting(const WebKit::WebGamepads& pads);
 
- protected:
-  virtual GpuChannelHostFactory* GetGpuChannelHostFactory() OVERRIDE;
-
  private:
   bool CheckPreparsedJsCachingEnabled() const;
 
@@ -154,6 +159,8 @@ class CONTENT_EXPORT RendererWebKitPlatformSupportImpl
   scoped_ptr<WebKit::WebBlobRegistry> blob_registry_;
 
   scoped_ptr<GamepadSharedMemoryReader> gamepad_shared_memory_reader_;
+
+  scoped_refptr<cc::ContextProvider> shared_offscreen_context_;
 };
 
 }  // namespace content

[commit-bot: I haz the power, 2013-03-06 01:54:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12212100/59001

[commit-bot: I haz the power, 2013-03-06 03:51:07]

Change committed as 186342