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

gfx/layers/RenderThread.h

@@ -22,6 +22,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:
@@ -29,35 +33,62 @@ public:
   virtual void Run(RenderThread& aRenderThread, wr::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(wr::WindowId aWindowId, UniquePtr<RendererOGL> aRenderer);
 
+  /// Can only be called from the render thread.
   void RemoveRenderer(wr::WindowId aWindowId);
 
-  // Automatically forwarded to the render thread.
+  // RenderNotifier implementation
+
+  /// Automatically forwarded to the render thread.
   void NewFrameReady(wr::WindowId aWindowId);
 
-  // Automatically forwarded to the render thread.
+  /// Automatically forwarded to the render thread.
   void NewScrollFrameReady(wr::WindowId aWindowId, bool aCompositeNeeded);
 
-  // Automatically forwarded to the render thread.
+  /// Automatically forwarded to the render thread.
   void PipelineSizeChanged(wr::WindowId aWindowId, uint64_t aPipelineId, float aWidth, float aHeight);
 
-  // Automatically forwarded to the render thread.
+  /// Automatically forwarded to the render thread.
   void RunEvent(wr::WindowId aWindowId, UniquePtr<RendererEvent> aCallBack);
 
 private:
@@ -65,6 +96,7 @@ private:
 
   ~RenderThread();
 
+  /// Can only be called from the render thread.
   void UpdateAndRender(wr::WindowId aWindowId);
 
   base::Thread* const mThread;

gfx/layers/opengl/RendererOGL.cpp

@@ -18,7 +18,7 @@ RendererOGL*
 RendererOGL::Create(already_AddRefed<RenderThread> aThread,
                     already_AddRefed<widget::CompositorWidget> aWidget,
                     WrRenderer* aWrRenderer,
-                    uint64_t aPipelineId,
+                    wr::WindowId aWindowId,
                     WebRenderBridgeParent* aBridge)
 {
   RefPtr<widget::CompositorWidget> widget = aWidget;
@@ -45,7 +45,7 @@ RendererOGL::Create(already_AddRefed<RenderThread> aThread,
   return new RendererOGL(thread.forget(),
                          gl.forget(),
                          widget.forget(),
-                         aPipelineId,
+                         aWindowId,
                          aWrRenderer,
                          aBridge);
 }
@@ -53,7 +53,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,
+                         wr::WindowId aWindowId,
                          WrRenderer* aWrRenderer,
                          WebRenderBridgeParent* aBridge)
 : mThread(aThread)
@@ -61,7 +61,7 @@ RendererOGL::RendererOGL(already_AddRefed<RenderThread> aThread,
 , mWidget(aWidget)
 , mWrRenderer(aWrRenderer)
 , mBridge(aBridge)
-, mPipelineId(aPipelineId)
+, mWindowId(aWindowId)
 {
 }
 
@@ -79,6 +79,11 @@ RendererOGL::Update()
 bool
 RendererOGL::Render(uint64_t aTransactionId)
 {
+  // 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.
+
   TimeStamp start = TimeStamp::Now();
 
   if (!mGL->MakeCurrent()) {

gfx/layers/opengl/RendererOGL.h

@@ -28,29 +28,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,
+                             wr::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,
+              wr::WindowId aWindowId,
               WrRenderer* aWrRenderer,
               WebRenderBridgeParent* aBridge);
 
@@ -59,7 +69,7 @@ protected:
   RefPtr<widget::CompositorWidget> mWidget;
   WrRenderer* mWrRenderer;
   WebRenderBridgeParent* mBridge;
-  uint64_t mPipelineId;
+  wr::WindowId mWindowId;
 };
 
 }

gfx/webrender_bindings/src/bindings.rs

@@ -194,6 +194,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,
@@ -208,6 +219,7 @@ impl WebRenderFrameBuilder {
     }
 }
 
+// XXX (bug 1328602) - This will be removed soon-ish.
 struct Notifier {
     render_notifier: Arc<(Mutex<bool>, Condvar)>,
 }
@@ -229,6 +241,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,