Initial commit of the blimp/ folder and target

blimp/client/compositor/blimp_compositor.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BLIMP_CLIENT_COMPOSITOR_BLIMP_COMPOSITOR_H_
+#define BLIMP_CLIENT_COMPOSITOR_BLIMP_COMPOSITOR_H_
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "cc/trees/layer_tree_host_client.h"
+#include "cc/trees/layer_tree_host_single_thread_client.h"
+#include "cc/trees/layer_tree_settings.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/native_widget_types.h"
+
+namespace base {
+class CommandLine;
+class SingleThreadTaskRunner;
+class Thread;
+}
+
+namespace cc {
+class LayerTreeHost;
+}
+
+namespace blimp {
+
+// BlimpCompositor provides the basic framework and setup to host a
+// LayerTreeHost (compositor).  The rendering surface this compositor draws to
+// is defined by the gfx::AcceleratedWidget returned by
+// BlimpCompositor::GetWindow().
+class BlimpCompositor : public cc::LayerTreeHostClient,
+                        public cc::LayerTreeHostSingleThreadClient {
+ public:
+  ~BlimpCompositor() override;
+
+  void SetVisible(bool visible);
+  void SetSize(const gfx::Size& size);
+
+  // LayerTreeHostClient Implementation ----------------------------------------

[Wez, 2015/08/27 02:01:51]

n00b nit: See my comment in the .java re trailing ----

nit: Elsewhere in Chromium we have comments like:

// WibbleFlanger interface.

rather than "implementation" - since this isn't actually the implementation.

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

Be gone ---!  I was looking at the cc files and they seem to do "<ClassName>
implementation", which makes sense to me.  This is the implementation though,
right?  We're implementating the interface here?

[Wez, 2015/09/03 00:49:27]

Sadly the C++ is not very consistent; typically in C++ this comment is in the
header, which is where you are declaring the bits of the interface that you're
overriding, so I prefer "Foo interface." so the comment indicates which
interface the stuff you're overriding belongs to.

In this case there's no separation between declaration and definition, so "Foo
implementation." would be reasonable - I'd personally still argue that "Foo
interface." is still valid, though, since it explains which interface the
overrides implement, and is then consistent with what I'd expect in C++.

This is super nit-picky, though! So long as we're consistent, it's all good. :)

p.s. You're missing . here too, at the end of the comment. :P

[David Trainor- moved to gerrit, 2015/09/03 06:33:21]

Crap!  Adding . to all "X implementation" lines.

+  void WillBeginMainFrame() override {}

[Wez, 2015/08/27 02:01:51]

Chromium style guide requires that virtual functions _never_ be defined in-line.

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

Ah my bad!  I saw this in ui/compositor/compositor.h and
compositor_impl_android.h when I was looking for the right way to implement
this.  Are these all incorrect?  I can look into fixing those in another patch
if so.

[Wez, 2015/09/03 00:49:27]

Strictly speaking, yes; I'm pretty sure the guidance used to be in favour of
inline {} for empty overrides, which is probably why those classes do it. I
think it later transpired that inline overrides impacts link times or something,
hence the Chromium style-guide dictat.

[David Trainor- moved to gerrit, 2015/09/03 06:33:21]

Moved these all to the cc file.  I'm fine with that :).

+  void DidBeginMainFrame() override {}
+  void BeginMainFrame(const cc::BeginFrameArgs& args) override {}
+  void BeginMainFrameNotExpectedSoon() override {}
+  void Layout() override;
+  void ApplyViewportDeltas(const gfx::Vector2dF& inner_delta,
+                           const gfx::Vector2dF& outer_delta,
+                           const gfx::Vector2dF& elastic_overscroll_delta,
+                           float page_scale,
+                           float top_controls_delta) override {}
+  void RequestNewOutputSurface() override;
+  void DidInitializeOutputSurface() override;
+  void DidFailToInitializeOutputSurface() override;
+  void WillCommit() override {}
+  void DidCommit() override;
+  void DidCommitAndDrawFrame() override {}
+  void DidCompleteSwapBuffers() override;
+  void DidCompletePageScaleAnimation() override {}
+  void RecordFrameTimingEvents(
+      scoped_ptr<cc::FrameTimingTracker::CompositeTimingSet> composite_events,
+      scoped_ptr<cc::FrameTimingTracker::MainFrameTimingSet> main_frame_events)
+      override {}
+
+  // LayerTreeHostSingleThreadClient Implementation ----------------------------
+  void ScheduleComposite() override;
+  void ScheduleAnimation() override;
+  void DidPostSwapBuffers() override;
+  void DidAbortSwapBuffers() override;
+
+ protected:
+  explicit BlimpCompositor(float device_scale_factor);
+
+  // Used by this class when building the OutputSurface that this compositor

[Wez, 2015/08/27 02:01:51]

"by this class" seems redundant?

Suggest wording like: "Returns a widget for use in constructing this
compositor's OutputSurface. Will only ever be called while this compositor is
visible."

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

Much better thanks :).

+  // draws to.  This needs to return a valid gfx::AcceleratedWidget by the time
+  // BlimpCompositor::SetVisible(true) is called.
+  virtual gfx::AcceleratedWidget GetWindow() = 0;
+
+  // Populates the cc::LayerTreeSettings used by the cc::LayerTreeHost.  Can be
+  // overridden to provide custom settings parameters.
+  virtual void GenerateLayerTreeSettings(cc::LayerTreeSettings& settings,

[Wez, 2015/08/27 02:01:51]

If this is an out-parameter then it should be cc::LayerTreeSettings*, not &.

I'm unclear on what cc::LayerTreeSettings intended usage is; it is declared as a
class but in all respects looks like a struct. All of it's members are PoD or
PoD-equivalent AFAICT, so can you just have this be:

virtual cc::LayerTreeSettings GenerateLayerTreeSettings(const base::CommandLine&
cmd);

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

I changed it to LayerTreeSettings*.

[Wez, 2015/09/03 00:49:27]

Acknowledged.

+                                         const base::CommandLine& cmd);
+
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> GetCompositorTaskRunner();

[Wez, 2015/08/27 02:01:51]

nit: Comment to clarify what this does - what is the compositor task runner?

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

Done.

+
+  gfx::Size viewport_size_;

[Wez, 2015/08/27 02:01:51]

nit: Comments to explain the less obvious members, e.g. device_scale_factor_,
host_ and compositor_thread_?

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

Done.

+  float device_scale_factor_;
+  scoped_ptr<cc::LayerTreeHost> host_;
+  scoped_ptr<cc::LayerTreeSettings> settings_;

[Wez, 2015/08/27 02:01:51]

nit: Does this need to be a scoped_ptr<>, rather than a member?

[David Trainor- moved to gerrit, 2015/08/28 01:23:46]

I wanted to easily know when it wasn't initialized yet (I thought about adding a
boolean but this seemed easier).  Like the other comment above about just
returning the reference, if you think the copy constructor and rebuilding the
settings when the app comes to the foreground costs are negligible I can make
these changes.

+  scoped_ptr<base::Thread> compositor_thread_;
+
+  base::WeakPtrFactory<BlimpCompositor> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlimpCompositor);
+};
+
+}  // namespace blimp
+
+#endif  // BLIMP_CLIENT_COMPOSITOR_BLIMP_COMPOSITOR_H_