Establish mojo service between Canvas (blink) and SurfaceManager (browser)

third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp

Index: third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp b/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
index b13d9ad7c36c2260ff386af7f2fdae63c4b4564e..ec00cb99338919da6c8dc5d3eaac7c7dd3558258 100644
--- a/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
+++ b/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
@@ -4,24 +4,70 @@
 
 #include "platform/graphics/CanvasSurfaceLayerBridge.h"
 
-#include "cc/layers/solid_color_layer.h"
+#include "cc/layers/surface_layer.h"
+#include "cc/surfaces/surface_id.h"
+#include "cc/surfaces/surface_sequence.h"
 #include "platform/graphics/GraphicsLayer.h"
+#include "platform/mojo/MojoHelper.h"
 #include "public/platform/Platform.h"
+#include "public/platform/ServiceRegistry.h"
 #include "public/platform/WebCompositorSupport.h"
 #include "public/platform/WebLayer.h"
+#include "ui/gfx/geometry/size.h"
+#include "wtf/Functional.h"
 
 namespace blink {
 
 CanvasSurfaceLayerBridge::CanvasSurfaceLayerBridge()
 {
-    m_solidColorLayer = cc::SolidColorLayer::Create();
-    m_solidColorLayer->SetBackgroundColor(SK_ColorBLUE);
-    m_webLayer = adoptPtr(Platform::current()->compositorSupport()->createLayerFromCCLayer(m_solidColorLayer.get()));
-    GraphicsLayer::registerContentsLayer(m_webLayer.get());
+    DCHECK(!m_service.is_bound());
+    Platform::current()->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));

[danakj, 2016/06/13 21:30:40]

It occured to me another way to deal with testing would be to have this service
be implemented by the tests, since this is the "virtual interface" in this case.
Then have tests do test-only things in the calls to this interface, instead of
overriding methods in this class to not use mojo.

I think that is how we intend to test things in cc, and leaves you without
ForTesting-type stuff in the code here.

 }
 
 CanvasSurfaceLayerBridge::~CanvasSurfaceLayerBridge()
 {
 }
 
+bool CanvasSurfaceLayerBridge::createSurfaceLayer(int canvasWidth, int canvasHeight)
+{
+    // Get SurfaceId from browser.
+    bool success = this->syncGetServiceId();
+    if (!success)
+        return false;
+
+    // Immediately send an Async message to browser to request Surface creation.
+    this->asyncRequestSurfaceCreation();
+
+    // Create SurfaceLayer.
+    const cc::SurfaceLayer::SatisfyCallback& satisfyCallback = createBaseCallback(bind<cc::SurfaceSequence>(&CanvasSurfaceLayerBridge::satisfyCallback, this));

[danakj, 2016/06/13 20:53:39]

don't use a const& here for the lvalue, you're storing and owning the lifetime
of the thing when you create it.

[xlai (Olivia), 2016/06/14 21:10:17]

Done.

+    const cc::SurfaceLayer::RequireCallback& requireCallback = createBaseCallback(bind<cc::SurfaceId, cc::SurfaceSequence>(&CanvasSurfaceLayerBridge::requireCallback, this));
+    m_surfaceLayer = cc::SurfaceLayer::Create(satisfyCallback, requireCallback);

[danakj, 2016/06/13 20:53:39]

you can std::move the callbacks here, to drop the local ownership

[xlai (Olivia), 2016/06/14 21:10:17]

Done.

+    m_surfaceLayer->SetSurfaceId(m_surfaceId, 1.f, gfx::Size(canvasWidth, canvasHeight));

[danakj, 2016/06/13 20:53:39]

How are you going to handle resizing (do you need to handle it)?

+
+    // Convert SurfaceLayer to WebLayer and register it.

[danakj, 2016/06/13 20:53:39]

style: This type of comment that just says what the code says is discouraged.
Comments should always explain "why" something is done. If all they say is
"what" it does, drop the comment and let the code speak for itself.

[xlai (Olivia), 2016/06/14 21:10:17]

Done. Removed superficial comments.

+    m_webLayer = adoptPtr(Platform::current()->compositorSupport()->createLayerFromCCLayer(m_surfaceLayer.get()));

[danakj, 2016/06/13 20:53:38]

Maybe you don't need to go through Platform etc anymore, and just do like this
and construct a WebLayerImpl:
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...

[xlai (Olivia), 2016/06/14 21:10:17]

I tried this and find that it can only be done by including the whole cc_blink
target into WebKit/Source/platform. Right now, everybody in WebKit access
functions in cc_blink via this WebKit/public/platform/WebCompositorSupport.h; so
I'd rather not to break this dependency in this patch. If we want to change this
dependency, I would rather do it on a separate patch and then change everyone
else simultaneously as well.

+    GraphicsLayer::registerContentsLayer(m_webLayer.get());
+    return true;
+}
+
+void CanvasSurfaceLayerBridge::satisfyCallback(cc::SurfaceSequence sequence)
+{
+    m_service->Satisfy(sequence);
 }
+
+void CanvasSurfaceLayerBridge::requireCallback(cc::SurfaceId surfaceId, cc::SurfaceSequence sequence)
+{
+    m_service->Require(surfaceId, sequence);
+}
+
+bool CanvasSurfaceLayerBridge::syncGetServiceId()

[danakj, 2016/06/13 20:53:39]

do you mean GetSurfaceId?

[xlai (Olivia), 2016/06/14 21:10:17]

The function name style in Blink starts with small letter. To distinguish it
from the mojo functions itself, I put sync as a prefix.

[danakj, 2016/06/14 22:07:00]

Oh oops, I was referring to Service vs Surface.

+{
+    return m_service->GetSurfaceId(&m_surfaceId);
+}
+
+void CanvasSurfaceLayerBridge::asyncRequestSurfaceCreation()
+{
+    m_service->RequestSurfaceCreation(m_surfaceId);
+}
+
+} // namespace blink