Bug 1328602 - Add a few comments explaining where some of the pieces are meant to fit. r=gfx?

gfx/layers/wr/RenderThread.cpp

@@ -157,7 +157,11 @@ RenderThread::UpdateAndRender(gfx::WindowId aWindowId)
 
   auto& renderer = it->second;
 
-  auto transactionId = 0; // TODO!
+  // TODO: WebRender has the notion of epoch and gecko has transaction ids.
+  // They mostly mean the same thing but I'm not sure they are produced the same
+  // way. We need to merge the two or have a way to associate transaction ids with
+  // epochs to wire everything up properly.
+  auto transactionId = 0;
 
   renderer->Update();
   renderer->Render(transactionId);

gfx/layers/wr/RenderThread.h

@@ -23,6 +23,10 @@ namespace layers {
 class RendererOGL;
 class RenderThread;
 
+/// Base class for an event that can bes cheduled to run on the render thread.
+///
+/// The event can be passed through the same channels as regular WebRender messages
+/// to preserve ordering.
 class RendererEvent
 {
 public:
@@ -30,35 +34,62 @@ public:
   virtual void Run(RenderThread& aRenderThread, gfx::WindowId aWindow);
 };
 
+/// The render thread is where WebRender issues all of its GPU work, and as much
+/// as possible this thread should only serve this purpose.
+///
+/// The render thread owns the different RendererOGLs (one per window) and implements
+/// the RenderNotifier api exposed by the WebRender bindings.
+///
+/// We should generally avoid posting tasks to the render thhread's event loop directly
+/// and instead use the RendererEvent mechanism which avoids races between the events
+/// and WebRender's own messages.
+///
+/// The GL context(s) should be created and used on this thread only.
+/// XXX - I've tried to organize code so that we can potentially avoid making
+/// this a singleton since this bad habbit has a tendency to bite us later, but
+/// I haven't gotten all the way there either in order to focus on the more
+/// important pieces first. So we are a bit in-between (this is totally a singleton
+/// but in some places we pretend it's not). Hopefully we can evolve this in a way
+/// that keeps the door open to removing the singleton bits.
 class RenderThread final
 {
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(RenderThread)
 
 public:
+  /// Can be called from any thread.
   static RenderThread* Get();
 
   static void Start();
 
   static void ShutDown();
 
+  /// Can be called from any thread.
+  /// In most cases it is best to use RendererEvents through WebRender's api instead
+  /// of scheduling directly to this message loop (so as to preserve the ordering
+  /// of the messages).
   static MessageLoop* Loop();
 
+  /// Can be called from any thread.
   static bool IsInRenderThread();
 
+  /// Can only be called from the render thread.
   void AddRenderer(gfx::WindowId aWindowId, UniquePtr<RendererOGL> aRenderer);
 
+  /// Can only be called from the render thread.
   void RemoveRenderer(gfx::WindowId aWindowId);
 
-  // Automatically forwarded to the render thread.
+  // RenderNotifier implementation
+
+  /// Automatically forwarded to the render thread.
   void NewFrameReady(gfx::WindowId aWindowId);
 
-  // Automatically forwarded to the render thread.
+  /// Automatically forwarded to the render thread.
   void NewScrollFrameReady(gfx::WindowId aWindowId, bool aCompositeNeeded);
 
-  // Automatically forwarded to the render thread.
+  /// Automatically forwarded to the render thread.
   void PipelineSizeChanged(gfx::WindowId aWindowId, uint64_t aPipelineId, float aWidth, float aHeight);
 
-  // Automatically forwarded to the render thread.
+  /// Automatically forwarded to the render thread.
   void RunEvent(gfx::WindowId aWindowId, UniquePtr<RendererEvent> aCallBack);
 
 private:
@@ -66,6 +97,7 @@ private:
 
   ~RenderThread();
 
+  /// Can only be called from the render thread.
   void UpdateAndRender(gfx::WindowId aWindowId);
 
   base::Thread* const mThread;

gfx/layers/wr/RendererOGL.cpp

@@ -19,7 +19,7 @@ RendererOGL*
 RendererOGL::Create(already_AddRefed<RenderThread> aThread,
                     already_AddRefed<widget::CompositorWidget> aWidget,
                     WrRenderer* aWrRenderer,
-                    uint64_t aPipelineId,
+                    gfx::WindowId aWindowId,
                     WebRenderBridgeParent* aBridge)
 {
   RefPtr<widget::CompositorWidget> widget = aWidget;
@@ -46,7 +46,7 @@ RendererOGL::Create(already_AddRefed<RenderThread> aThread,
   return new RendererOGL(thread.forget(),
                          gl.forget(),
                          widget.forget(),
-                         aPipelineId,
+                         aWindowId,
                          aWrRenderer,
                          aBridge);
 }
@@ -54,7 +54,7 @@ RendererOGL::Create(already_AddRefed<RenderThread> aThread,
 RendererOGL::RendererOGL(already_AddRefed<RenderThread> aThread,
                          already_AddRefed<gl::GLContext> aGL,
                          already_AddRefed<widget::CompositorWidget> aWidget,
-                         uint64_t aPipelineId,
+                         gfx::WindowId aWindowId,
                          WrRenderer* aWrRenderer,
                          WebRenderBridgeParent* aBridge)
 : mThread(aThread)
@@ -62,7 +62,7 @@ RendererOGL::RendererOGL(already_AddRefed<RenderThread> aThread,
 , mWidget(aWidget)
 , mWrRenderer(aWrRenderer)
 , mBridge(aBridge)
-, mPipelineId(aPipelineId)
+, mWindowId(aWindowId)
 {
 }
 

gfx/layers/wr/RendererOGL.h

@@ -8,6 +8,7 @@
 #define MOZILLA_LAYERS_RENDEREROGL_H
 
 #include "mozilla/layers/RenderThread.h"
+#include "mozilla/layers/WebRenderTypes.h"
 #include "mozilla/gfx/webrender.h"
 
 namespace mozilla {
@@ -28,29 +29,39 @@ namespace layers {
 
 class WebRenderBridgeParent;
 
+/// Owns the WebRender renderer and GL context.
+///
+/// There is one renderer per window, all owned by the render thread.
+/// This class is a similar abstraction to CompositorOGL except that it is used
+/// on the render thread instead of the compositor thread.
 class RendererOGL
 {
 public:
 
+  /// Render thread only.
   static RendererOGL* Create(already_AddRefed<RenderThread> aThread,
                              already_AddRefed<widget::CompositorWidget> aWidget,
                              WrRenderer* aWrRenderer,
-                             uint64_t aPipelineId,
+                             gfx::WindowId aWindowId,
                              WebRenderBridgeParent* aBridge);
 
+  /// Render thread only.
   void Update();
 
+  /// Render thread only.
   bool Render(uint64_t aTransactionId);
 
+  /// Render thread only.
   bool RenderToTarget(gfx::DrawTarget& aTarget);
 
+  /// Render thread only.
   ~RendererOGL();
 
 protected:
   RendererOGL(already_AddRefed<RenderThread> aThread,
               already_AddRefed<gl::GLContext> aGL,
               already_AddRefed<widget::CompositorWidget>,
-              uint64_t aPipelineId,
+              gfx::WindowId aWindowId,
               WrRenderer* aWrRenderer,
               WebRenderBridgeParent* aBridge);
 
@@ -59,7 +70,7 @@ protected:
   RefPtr<widget::CompositorWidget> mWidget;
   WrRenderer* mWrRenderer;
   WebRenderBridgeParent* mBridge;
-  uint64_t mPipelineId;
+  gfx::WindowId mWindowId;
 };
 
 }

gfx/webrender_bindings/src/bindings.rs

@@ -196,6 +196,17 @@ impl webrender_traits::RenderNotifier for CppNotifier {
     }
 }
 
+// RenderThread WIP notes:
+// In order to separate the compositor thread (or ipc receiver) and the render
+// thread, some of the logic below needs to be rewritten. In particular
+// the WrWindowState and Notifier implementations aren't designed to work with
+// a separate render thread.
+// As part of that I am moving the bindings closer to WebRender's API boundary,
+// and moving more of the logic in C++ land.
+// This work is tracked by bug 1328602.
+//
+// See RenderThread.h for some notes about how the pieces fit together.
+
 pub struct WebRenderFrameBuilder {
     pub root_pipeline_id: PipelineId,
     pub dl_builder: webrender_traits::DisplayListBuilder,
@@ -210,6 +221,7 @@ impl WebRenderFrameBuilder {
     }
 }
 
+// XXX (bug 1328602) - This will be removed soon-ish.
 struct Notifier {
     render_notifier: Arc<(Mutex<bool>, Condvar)>,
 }
@@ -231,6 +243,7 @@ impl webrender_traits::RenderNotifier for Notifier {
     }
 }
 
+// XXX (bug 1328602) - This will be removed soon-ish.
 pub struct WrWindowState {
     renderer: Renderer,
     api: webrender_traits::RenderApi,