Introducing SurfaceReferenceFactory

Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot tell if the
underlying implementation is SurfaceSequence or SurfaceReference, which paves the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/72b2a28cb2ffa0dabe4505e14ba65c41c20495c5
Cr-Commit-Position: refs/heads/master@{#439299}

[Saman Sami, 2016-11-18 18:09:57]

Description was changed from

==========
SurfaceInfo

BUG=659598
==========

to

==========
SurfaceInfo

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-11-18 18:10:25]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 18:11:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 21:33:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 21:33:44]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-11-21 15:59:02]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-11-21 15:59:03]

We want this thing to be polymorphic and move-only.

https://codereview.chromium.org/2514033002/diff/1/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/1/cc/layers/surface_layer.h#n...
cc/layers/surface_layer.h:36: void SetSurfaceInfo(SurfaceInfo surface_info);
void SetSurfaceInfo(std::unique_ptr<SurfaceInfo> surface_info)

If we want to make this thing polymorphic and move-only then just use a
unique_ptr.

https://codereview.chromium.org/2514033002/diff/1/cc/layers/surface_layer.h#n...
cc/layers/surface_layer.h:43: SurfaceInfo surface_info() { return surface_info_;
}
This should be readonly.

const SurfaceInfo& surface_info() const { return surface_info_; }

[Saman Sami, 2016-11-22 22:05:29]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2016-11-22 22:05:34]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-22 22:05:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 22:08:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 22:08:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2016-11-22 22:14:01]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2016-11-22 22:14:08]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-22 22:14:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 00:38:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 00:38:21]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-11-24 17:10:09]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 17:10:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 17:13:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 17:13:56]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2016-11-24 17:19:00]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2016-11-24 17:19:04]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-11-24 17:19:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 17:46:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 17:46:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-11-24 18:08:21]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 18:08:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-24 18:17:17]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 18:17:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 18:32:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 18:32:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2016-11-24 18:50:06]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 18:50:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-24 19:14:26]

Patchset #3 (id:100001) has been deleted

[Saman Sami, 2016-11-24 19:14:35]

Patchset #3 (id:120001) has been deleted

[Saman Sami, 2016-11-24 19:14:44]

Patchset #3 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-11-24 19:24:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 19:24:06]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-11-24 21:15:52]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 21:16:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 21:38:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 21:38:26]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Saman Sami, 2016-11-28 19:34:34]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 19:35:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 21:50:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 21:50:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Saman Sami, 2016-11-30 00:18:28]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 00:20:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 00:51:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 00:51:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Saman Sami, 2016-11-30 02:29:02]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2016-11-30 02:29:14]

Patchset #5 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-11-30 02:29:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-30 03:32:16]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2016-11-30 03:32:30]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 03:33:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 04:22:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 04:22:38]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Saman Sami, 2016-11-30 04:47:01]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 04:47:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 05:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 05:28:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Saman Sami, 2016-11-30 15:07:41]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 15:07:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-30 15:49:16]

Patchset #7 (id:260001) has been deleted

[Saman Sami, 2016-11-30 15:49:25]

Patchset #5 (id:220001) has been deleted

[Saman Sami, 2016-11-30 15:49:40]

Patchset #5 (id:240001) has been deleted

[Saman Sami, 2016-11-30 15:58:12]

Patchset #5 (id:280001) has been deleted

[Saman Sami, 2016-11-30 15:58:18]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 15:58:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-30 16:52:33]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 16:53:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-30 16:57:09]

Patchset #5 (id:300001) has been deleted

[Saman Sami, 2016-11-30 17:31:02]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 17:31:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-30 17:48:31]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 17:48:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-11-30 18:05:52]

https://codereview.chromium.org/2514033002/diff/360001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:100: layer_impl->SetSurfaceId(surface_ref_->id());
Send clone maybe

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/BUILD.gn
File cc/surfaces/BUILD.gn (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/BUILD.gn#n...
cc/surfaces/BUILD.gn:20: "surfaces_export.h",
Move back to surfaces:surfaces

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/BUILD.gn#n...
cc/surfaces/BUILD.gn:37: "//ui/gfx/geometry",
Necessary?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_ma...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_ma...
cc/surfaces/surface_manager.cc:21: class SurfaceRef final : public
SurfaceRefWithSequence<SurfaceRef> {
Move to a separate file and rename?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
File cc/surfaces/surface_ref_base.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.cc:9: SurfaceRefInternal::~SurfaceRefInternal() {
Move more stuff to this file or move this to header (I think there might be a
compile error though)

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
File cc/surfaces/surface_ref_base.h (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:88: bool is_active_ = false;
rename?

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:63: class PluginSurfaceRef :
public cc::SurfaceRefWithSequence<PluginSurfaceRef> {
Should I merge these two classes and also pass render_frame_proxy_ from below to
decide what to do?

https://codereview.chromium.org/2514033002/diff/360001/ui/compositor/layer_un...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/ui/compositor/layer_un...
ui/compositor/layer_unittest.cc:1816: SetId(id);
SetMetadata

[Fady Samuel, 2016-11-30 19:51:30]

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_ma...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_ma...
cc/surfaces/surface_manager.cc:42: SetMetadata(surface_id, scale, size);
Call the base class' constructor instead?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_ref.h
File cc/surfaces/surface_ref.h (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref.h:19: void BlockImpl() override {
Why is this called block?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref.h:26: void AllowImpl() override {
Why is this called allow?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref.h:34: void DelegateLockImpl(CompositorFrameMetadata*
metadata) override {
Why is this called DelegateLockImpl? I don't understand what this is supposed to
do.

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
File cc/surfaces/surface_ref_base.h (right):

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:23: class SurfaceRefInternal {
kylechar@ and I were chatting. My current thinking is what you built really is a
SurfaceEmbedding that has "SurfaceInfo" and SurfaceSequence/SurfaceReference.

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:29: SurfaceRefInternal* SetId(const SurfaceId&
id) {
Can we get rid of these setters? I'd prefer if id, scale, and size are all
const.

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:44: void SetHost(LayerTreeHost* host) {
I'd prefer if the base class didn't know about LayerTreeHost. Not all Mus
clients will have a LayerTreeHost.

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:48: void AllowSurfaceDestruction() {
I'm super confused about these things. What does AllowSurfaceDestruction, and
BlockSurfaceDestruction do and why do we ned them?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:61: bool is_active() { return is_active_; }
What does this mean?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:63: void SetMetadata(const SurfaceId& id, float
scale, const gfx::Size& size) {
Do these quantities change? Maybe make this in the constructor's initializer
list instead?

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:77: LayerTreeHost* host() const { return host_; }
SurfaceRef/SurfaceEmbedding shouldn't know about LayerTreeHost.

https://codereview.chromium.org/2514033002/diff/360001/cc/surfaces/surface_re...
cc/surfaces/surface_ref_base.h:98: SurfaceRefPtr res(new T(*(T*)this));
Use static_cast

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:60: int routing_id_;
const

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:63: class PluginSurfaceRef :
public cc::SurfaceRefWithSequence<PluginSurfaceRef> {
On 2016/11/30 18:05:52, Saman Sami wrote:
> Should I merge these two classes and also pass render_frame_proxy_ from below
to
> decide what to do?

No, that's not thread safe.

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:82: int routing_id_;
const int.

https://codereview.chromium.org/2514033002/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:83: int plugin_id_;
const int

https://codereview.chromium.org/2514033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:52:
ref->SetMetadata(m_surfaceId, 1.f, gfx::Size(canvasWidth, canvasHeight));
I really don't like the idea of calling this metadata and make it happen later.
How about making that part of the constructor instead to SurfaceRef?

https://codereview.chromium.org/2514033002/diff/360001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:35:
surface_manager->NewSurfaceRef(surface_id, 1.f, surface_size);
SurfaceManager will live in mus gpu and this SurfaceRef will live in the cilent
process. I don't think it should be surfacemanager dispensing this thing. I
think instead, we should have a type of SurfaceRef that takes in a
SurfaceManager. That might be OK for now.

https://codereview.chromium.org/2514033002/diff/360001/ui/compositor/layer_un...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/2514033002/diff/360001/ui/compositor/layer_un...
ui/compositor/layer_unittest.cc:1816: SetId(id);
On 2016/11/30 18:05:52, Saman Sami wrote:
> SetMetadata

call base constructor instead?

https://codereview.chromium.org/2514033002/diff/360001/ui/compositor/layer_un...
ui/compositor/layer_unittest.cc:1847: gfx::Size(10, 10));
Do we need this extra parameter? What does it do?

[commit-bot: I haz the power, 2016-11-30 20:26:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 20:26:24]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-01 18:59:28]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 19:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 19:10:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 19:10:03]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[Saman Sami, 2016-12-01 20:28:12]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:28:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 20:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 20:33:49]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-01 20:40:02]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:40:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-01 20:41:04]

Patchset #9 (id:400001) has been deleted

[commit-bot: I haz the power, 2016-12-01 20:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 20:51:41]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[Saman Sami, 2016-12-02 20:54:22]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 20:54:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 20:59:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 20:59:06]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2016-12-02 21:25:00]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 21:25:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 22:04:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 22:04:52]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Saman Sami, 2016-12-02 22:10:47]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 22:11:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-02 22:36:42]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 22:37:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 23:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 23:33:49]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Saman Sami, 2016-12-02 23:43:28]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 23:44:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 00:59:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 00:59:50]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Saman Sami, 2016-12-05 18:10:32]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 18:10:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-05 19:01:31]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 19:02:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 19:06:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 19:06:58]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Saman Sami, 2016-12-05 19:10:56]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 19:11:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 21:10:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 21:10:10]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-12-06 15:52:25]

Description was changed from

==========
SurfaceInfo

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
SurfaceInfo

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-06 20:10:13]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 20:11:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 20:15:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 20:15:28]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Saman Sami, 2016-12-06 21:13:24]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:13:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-06 21:51:54]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:52:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 01:58:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 01:58:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-07 04:17:34]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 04:17:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 05:43:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 05:43:19]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-07 18:44:23]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 18:44:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 19:04:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 19:04:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-07 19:13:55]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 19:14:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 19:27:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 19:27:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-07 19:35:06]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 19:35:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-07 19:40:47]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 19:41:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 21:48:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 21:48:36]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-07 22:44:59]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:45:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-07 23:07:41]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 23:08:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-12-07 23:29:04]

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer.h:23: static scoped_refptr<SurfaceLayer>
Create(SurfaceEmbeddingPtr);
TBH, I'm not a fan of this typedef.

How about just std::unique_ptr<SurfaceEmbedding>. It's more characters but it's
more explicit.

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer.h:32: const SurfaceEmbeddingPtr& surface_emb() const {
return surface_emb_; }
surface_embedding()

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer.h:47: SurfaceEmbeddingPtr current_ref_;
I don't understand the difference between these two?

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer_impl.h:27: void SetSurfaceInfo(const SurfaceInfo&);
const SurfaceInfo& surface_info

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:14: class CC_SURFACES_EXPORT
SurfaceEmbeddingUsingSequence
How about SurfaceSequenceSurfaceEmbedding?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:18: struct SurfaceInfo {
Put this in a separate file?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:53: SurfaceEmbeddingPtr
CreateReference(SurfaceEmbeddingOwner* owner) {
How about simply making this a pure virtual function?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:60: void
DelegateReference(CompositorFrameMetadata* metadata) {
DelegateReference is a confusing name: How about just
RemoveReference(CompsoitorFrameMetadata* metdata)?

https://codereview.chromium.org/2514033002/diff/740001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/740001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:44: class SurfaceEmbedding :
public cc::SurfaceEmbeddingUsingSequence {
ChildFrameSurfaceEmbedding?

https://codereview.chromium.org/2514033002/diff/740001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:68: class
PluginSurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {
BrowserPluginSurfaceEmbedding

https://codereview.chromium.org/2514033002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:23:
class SurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {
OffscreenCanvasSurfaceEmbedding

https://codereview.chromium.org/2514033002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:25:
SurfaceEmbedding(cc::SurfaceInfo info,
const cc::SurfaceInfo& info

https://codereview.chromium.org/2514033002/diff/740001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2514033002/diff/740001/ui/compositor/layer.cc...
ui/compositor/layer.cc:185:
clone->SetShowSurface(surface_layer_->surface_emb()->Clone(),
This is wrong I think. You want to create a new reference here.

https://codereview.chromium.org/2514033002/diff/740001/ui/compositor/layer.cc...
ui/compositor/layer.cc:660: cc::SurfaceLayer::Create(surface_ref->Clone());
Why are you cloning here?

[Fady Samuel, 2016-12-07 23:29:05]

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer.h:23: static scoped_refptr<SurfaceLayer>
Create(SurfaceEmbeddingPtr);
TBH, I'm not a fan of this typedef.

How about just std::unique_ptr<SurfaceEmbedding>. It's more characters but it's
more explicit.

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer.h:32: const SurfaceEmbeddingPtr& surface_emb() const {
return surface_emb_; }
surface_embedding()

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer.h:47: SurfaceEmbeddingPtr current_ref_;
I don't understand the difference between these two?

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/layers/surface_laye...
cc/layers/surface_layer_impl.h:27: void SetSurfaceInfo(const SurfaceInfo&);
const SurfaceInfo& surface_info

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:14: class CC_SURFACES_EXPORT
SurfaceEmbeddingUsingSequence
How about SurfaceSequenceSurfaceEmbedding?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:18: struct SurfaceInfo {
Put this in a separate file?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:53: SurfaceEmbeddingPtr
CreateReference(SurfaceEmbeddingOwner* owner) {
How about simply making this a pure virtual function?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:60: void
DelegateReference(CompositorFrameMetadata* metadata) {
DelegateReference is a confusing name: How about just
RemoveReference(CompsoitorFrameMetadata* metdata)?

https://codereview.chromium.org/2514033002/diff/740001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/740001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:44: class SurfaceEmbedding :
public cc::SurfaceEmbeddingUsingSequence {
ChildFrameSurfaceEmbedding?

https://codereview.chromium.org/2514033002/diff/740001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:68: class
PluginSurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {
BrowserPluginSurfaceEmbedding

https://codereview.chromium.org/2514033002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:23:
class SurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {
OffscreenCanvasSurfaceEmbedding

https://codereview.chromium.org/2514033002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:25:
SurfaceEmbedding(cc::SurfaceInfo info,
const cc::SurfaceInfo& info

https://codereview.chromium.org/2514033002/diff/740001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2514033002/diff/740001/ui/compositor/layer.cc...
ui/compositor/layer.cc:185:
clone->SetShowSurface(surface_layer_->surface_emb()->Clone(),
This is wrong I think. You want to create a new reference here.

https://codereview.chromium.org/2514033002/diff/740001/ui/compositor/layer.cc...
ui/compositor/layer.cc:660: cc::SurfaceLayer::Create(surface_ref->Clone());
Why are you cloning here?

[commit-bot: I haz the power, 2016-12-08 00:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 00:46:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[kylechar, 2016-12-08 02:02:46]

Heres some styleguide nits I noticed while looking over things.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
File cc/surfaces/direct_surface_embedding.cc (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
cc/surfaces/direct_surface_embedding.cc:5: #include
"cc/surfaces/direct_surface_embedding.h"
Newline between associated head file and other headers. Missing vetor and
SurfaceSequence headers.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
File cc/surfaces/direct_surface_embedding.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
cc/surfaces/direct_surface_embedding.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
cc/surfaces/direct_surface_embedding.h:14: : public
NON_EXPORTED_BASE(SurfaceEmbeddingUsingSequence) {
SurfaceEmbeddingUsingSequence is exported though?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
cc/surfaces/direct_surface_embedding.h:21: void SatisfySequence(const
SurfaceSequence&) override;
const SurfafeSequence& sequence

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/direct_sur...
cc/surfaces/direct_surface_embedding.h:25: };
Should this be copyable?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
No (c) in new copyright headers.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
File should have same name as class.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:22: void
AddReferenceImpl(SurfaceEmbeddingOwner*) override;
SurfaceEmbeddingOwners* owner

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:24: virtual void RequireSequence(const
SurfaceSequence&) = 0;
You should have a virtual destructor for an abstract class.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.h:27: };
Again, should this be copyable?

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.cc (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.cc:12: }
// namespace cc

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:26: bool operator==(const SurfaceInfo&
info) const {
You should define != too if you define ==.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:40: class SurfaceEmbedding {
One class per header and file should be named after that class.

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:43: ~SurfaceEmbedding();
virtual ~SurfaceEmbedding();

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:45: void RemoveReference() {
Function definitions should be in the .cc file unless they're trivial, eg. int
foo() const { return foo_; }

[Fady Samuel, 2016-12-08 12:49:24]

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.h (right):

https://codereview.chromium.org/2514033002/diff/740001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:70: SurfaceEmbeddingPtr Clone() {
If you want a clone then just make Clone() a pure virtual in the base class. I'm
not a fan of this * / *Impl template pattern used all over the place.

[Saman Sami, 2016-12-09 17:35:16]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 17:35:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-09 22:04:38]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 22:05:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 23:26:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 23:26:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Saman Sami, 2016-12-10 00:24:28]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 00:25:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 01:37:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:37:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Saman Sami, 2016-12-10 03:01:09]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 03:02:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 04:05:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 04:05:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Saman Sami, 2016-12-10 05:01:01]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 05:01:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 07:28:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 07:28:28]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-10 17:05:50]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 17:06:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 18:55:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 18:55:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Saman Sami, 2016-12-10 22:22:47]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 22:23:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 00:22:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 00:23:00]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-11 03:42:20]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 03:42:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 05:38:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 05:38:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Saman Sami, 2016-12-11 18:25:30]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 18:25:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-11 20:06:18]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 20:06:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 20:09:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 20:09:55]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Saman Sami, 2016-12-11 20:24:24]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 20:24:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 22:14:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 22:14:07]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-12 06:07:20]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 06:07:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 08:07:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 08:07:19]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-12-12 16:12:01]

https://codereview.chromium.org/2514033002/diff/760001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2514033002/diff/760001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:66: current_ref_ =
surface_emb_->CreateReference(layer_tree_host());
if (layer_tree_host())
  surface_embedding_->Bind(layer_tree_host());

https://codereview.chromium.org/2514033002/diff/760001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:88: current_ref_ =
surface_emb_->CreateReference(layer_tree_host());
surface_embedding_ = surface_embedding_->CloneSurfaceAndBind(layer_tree_host());

Creates a new SurfaceEmbedding bound to a new owner with a new reference.

https://codereview.chromium.org/2514033002/diff/760001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:102: new
SatisfySwapPromise(std::move(current_ref_)));
Have this take in a SurfaceEmbedding.

https://codereview.chromium.org/2514033002/diff/760001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.h (right):

https://codereview.chromium.org/2514033002/diff/760001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:43: class SurfaceRef final {
Make this move only and create on stack. Call it ScopedSurfaceReference

https://codereview.chromium.org/2514033002/diff/760001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:52: bool is_deleted_ = false;
What's the point of this? Set embedding_ = nullptr instead?

https://codereview.chromium.org/2514033002/diff/760001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:56: class SurfaceEmbedding {
Make this refcounted.

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
cc/layers/surface_layer.cc:21: explicit SatisfySwapPromise(SurfaceRefPtr
surface_ref)
scoped_ptr<SurfaceReference> surface_reference?

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
cc/layers/surface_layer.h:22: static scoped_refptr<SurfaceLayer>
Create(SurfaceEmbeddingPtr);
nit: scoped_refptr<SurfaceEmbedding> surface_embedding? Also, is this now a
SurfaceReferenceFactory?

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
cc/layers/surface_layer.h:23: void SetSurfaceInfo(const SurfaceId& id, float
scale, const gfx::Size& size);
I'd rather have just one way to set surface info. Can you get rid of this
method?

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl.h (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
cc/layers/surface_layer_impl.h:27: void SetSurfaceInfo(const SurfaceInfo&);
const SurfaceInfo& surface_info

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
cc/layers/surface_layer_impl.h:28: const SurfaceId& surface_id() const { return
surface_info_.id; }
nit: is this necessary?

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
File cc/layers/surface_layer_impl_unittest.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/layers/surface_laye...
cc/layers/surface_layer_impl_unittest.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Please don't change the copyright on an existing file.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.cc:12: SequenceSurfaceRef* ref =
static_cast<SequenceSurfaceRef*>(surface_ref);
nit: Comment explaining why this is safe.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.cc:18: SequenceSurfaceRef* ref =
static_cast<SequenceSurfaceRef*>(surface_ref);
nit: comment explaining why this is safe.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.cc:19: SatisfySequence(ref->seq());
Does this need to be a separate method?

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.cc:27: RequireSequence(id, seq);
Does this need to be a separate method? This just confuses things, I think.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding_base.h (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:19: struct SurfaceInfo {
Move to a separate file.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:24: SurfaceInfo() {}
SurfaceInfo() = default;

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:27: bool operator==(const SurfaceInfo&
info) const {
Implement !=

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:34: typedef scoped_refptr<SurfaceEmbedding>
SurfaceEmbeddingPtr;
This typedef isn't adding value. Please be explicit.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:43: class SurfaceRef {
Please try to have one class per header / cc file pair. Also, the name of the
class should match the name of the file. This is very hard to follow.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:45: explicit SurfaceRef(const
SurfaceEmbedding* factory);
I'd also give this a move only constructor and move assignment operator.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding_base.h:62: SequenceSurfaceRef(const
SurfaceEmbedding* factory,
Move constructor and move assignment operator.

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_re...
File cc/surfaces/surface_reference_owner.h (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_re...
cc/surfaces/surface_reference_owner.h:12: class SurfaceReferenceOwner {
Comment.

[Fady Samuel, 2016-12-12 16:25:08]

https://codereview.chromium.org/2514033002/diff/980001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:496: new
cc::DirectSurfaceEmbedding(manager);
We likely don't need to construct a new one of these things every time.

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:44: class SurfaceEmbedding :
public cc::SurfaceEmbeddingUsingSequence {
IframeSurfaceEmbedding

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:62: int routing_id_;
DISALLOW_COPY_AND_ASSIGN(IframeSurfaceEmbedding)

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:62: int routing_id_;
const int routing_id_

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:65: class
PluginSurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {
nit: BrowserPluginSurfaceEmbedding

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:86: int routing_id_;
const int routing_id_;

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:87: int plugin_id_;
DISALLOW_COPY_AND_ASSIGN(BrowserPluginSurfaceEmbedding)

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:87: int plugin_id_;
const int browser_plugin_instance_id_; Longer, more descriptive names demystify
code in the future.

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:196: if (render_frame_proxy_)
Braces.

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:197: surface_ref = new
SurfaceEmbedding(sender, host_routing_id_);
It feels like we should have a single instance of this thing, rather than
creating a new one on every OnSetSetSurface

https://codereview.chromium.org/2514033002/diff/980001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:203:
cc::SurfaceLayer::Create(std::move(surface_ref));
A surface embedding is ref counted, right? Do we actually need a copy of this
thing for every surfacelayer?

https://codereview.chromium.org/2514033002/diff/980001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/980001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:39:
const mojom::blink::OffscreenCanvasSurfacePtr& m_service;
This is likely going to have lifetime issues.

https://codereview.chromium.org/2514033002/diff/980001/ui/android/delegated_f...
File ui/android/delegated_frame_host_android.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/ui/android/delegated_f...
ui/android/delegated_frame_host_android.cc:36: cc::SurfaceLayer::Create(new
cc::DirectSurfaceEmbedding(surface_manager));
It almost feels like SurfaceManager should have a DirectSurfaceEmbedding hanging
off of it.

https://codereview.chromium.org/2514033002/diff/980001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/ui/compositor/layer.cc...
ui/compositor/layer.cc:186: surface_layer_->ref_factory(), frame_size_in_dip_);
Being explicit seems useful here: surface_reference_factory()

[Saman Sami, 2016-12-12 16:42:39]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 16:43:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-12 17:46:23]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 17:46:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-12 18:52:40]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 18:53:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-12 19:58:14]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:59:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-12 20:15:29]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 20:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-12 20:51:16]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 20:52:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-12 21:07:09]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 21:08:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-12-12 22:35:26]

Super cool so far! Much more readable.

https://codereview.chromium.org/2514033002/diff/1120001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1120001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:28: explicit SurfaceReferenceBase(const
SurfaceReferenceFactory* factory);
If you're holding onto SurfaceReferenceFactory as a scoped_refptr, then take it
in as a scoped_refptr. So:

SurfaceReferenceBase(scoped_refptr<SurfaceReferenceFactory> factory);

https://codereview.chromium.org/2514033002/diff/1120001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:33: bool IsDestroyed() const;
bool destroyed() const { return !!factory_; }

[Saman Sami, 2016-12-12 23:24:32]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2016-12-12 23:25:30]

PTAL

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
File cc/surfaces/surface_embedding.cc (right):

https://codereview.chromium.org/2514033002/diff/980001/cc/surfaces/surface_em...
cc/surfaces/surface_embedding.cc:19: SatisfySequence(ref->seq());
On 2016/12/12 16:12:00, Fady Samuel wrote:
> Does this need to be a separate method?

It's nicer when it is. Someone has already done the static cast and extracted
the sequence number when you're in a subclass.

[commit-bot: I haz the power, 2016-12-12 23:26:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-12-12 23:35:46]

https://codereview.chromium.org/2514033002/diff/1140001/cc/layers/surface_lay...
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/1140001/cc/layers/surface_lay...
cc/layers/surface_layer.h:26: void SetSurfaceInfo(const SurfaceId& id, float
scale, const gfx::Size& size);
Delete this or does that cause too much churn?

https://codereview.chromium.org/2514033002/diff/1140001/components/exo/surfac...
File components/exo/surface.cc (right):

https://codereview.chromium.org/2514033002/diff/1140001/components/exo/surfac...
components/exo/surface.cc:49: class ExoSurfaceReferenceFactory : public
cc::SequenceSurfaceReferenceFactory {
Drop Exo, it's redundant in this case. For example exo::CompositorFrameSink has
no Exo prefix because it's in the exo namespace.

https://codereview.chromium.org/2514033002/diff/1140001/components/exo/surfac...
components/exo/surface.cc:69: };
nit: new line after

https://codereview.chromium.org/2514033002/diff/1140001/components/exo/surfac...
components/exo/surface.cc:443: scoped_refptr<cc::SurfaceReferenceFactory>
ref_factory =
Can we put this as a member variable of exo::Surface so we don't recreate it
every time?

https://codereview.chromium.org/2514033002/diff/1140001/content/renderer/chil...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/1140001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:206:
scoped_refptr<cc::SurfaceReferenceFactory> ref_factory;
What if ChildFrameCompositingHelper has-a SurfaceReferenceFactory so you don't
keep recreating it.

https://codereview.chromium.org/2514033002/diff/1140001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/1140001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:61: new
OffscreenCanvasSurfaceReferenceFactory(m_service));
Can we try to avoid continuously creating a factory? Can this have-a factory?

[Saman Sami, 2016-12-12 23:36:06]

Description was changed from

==========
SurfaceInfo

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which replaces satisfy and
require callbacks. This class hides the implementation details of
surface references from its clients and reduces direct usages of
SurfaceSequence in our codebase which makes the transition to SurfaceReference
smoother.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-12-12 23:50:59]

https://codereview.chromium.org/2514033002/diff/1140001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1140001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:14: struct SurfaceInfo {
I think it would be nice if everything in this struct was read-only to external
accessors.

Basically make this is a class. Make id, scale, and size private with read-only
accessors.

[commit-bot: I haz the power, 2016-12-13 01:35:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 01:35:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-13 19:37:56]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 19:38:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-13 21:01:17]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 21:02:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-13 21:28:17]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 21:28:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-13 21:48:49]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 21:49:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-13 22:04:28]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which replaces satisfy and
require callbacks. This class hides the implementation details of
surface references from its clients and reduces direct usages of
SurfaceSequence in our codebase which makes the transition to SurfaceReference
smoother.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface refrences
while hiding the implementation details from its client. So 
the client cannot tell if the underlying implementation
is SurfaceSequence or SurfaceReference, which paves the way for phasing 
out SurfaceSequence and replacing it with SurfaceReference. All require 
/ satisfy callbacks are now gone and are replaces with different
implementations of SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2016-12-13 22:55:06]

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
cc/layers/surface_layer.h:33: const scoped_refptr<SurfaceReferenceFactory>
surface_reference_factory()
nit: remove the const

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
File cc/layers/surface_layer_unittest.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
cc/layers/surface_layer_unittest.cc:84: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference.h:20: const SurfaceSequence& sequence()
const { return sequence_; }
If you don't need it, remove it.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference_factory.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.cc:25: // to the type we returned
in CreateReference.
nit; You should mention that SurfaceReference is a friend

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.cc:33: // to the type we returned
in CreateReference.
nit; You should mention that SurfaceReference is a friend

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.h:30: void
DestroyReference(SurfaceReferenceBase* surface_ref,
// SurfaceReferenceFactory implementation:

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.h:33: virtual void
RequireSequence(const SurfaceId& surface_id,
nit Move these to the top of the private section.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.cc:13: return;
Move this to the destructor:

if (factory_)
  factory_->DestroyReference(this);

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.cc:35: : factory_(factory) {}
factory_(std::move(factory))

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:32: void Destroy();
Can you make this private?

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:33: void Destroy(CompositorFrameMetadata*
metadata);
// TODO(fsamuel, samans, kylechar): Remove this.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:34: bool IsDestroyed() const;
Remove this if it's not used.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:36: const SurfaceReferenceFactory*
factory() const { return factory_.get(); }
Do we need this?

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_factory.h:23: : public
base::RefCounted<SurfaceReferenceFactory> {
RefCountedThreadSafe

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_factory.h:39: friend class SurfaceReferenceBase;
Move friend statements above.

https://codereview.chromium.org/2514033002/diff/1200001/components/exo/surfac...
File components/exo/surface.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/components/exo/surfac...
components/exo/surface.cc:61: }
nit: blank line.

https://codereview.chromium.org/2514033002/diff/1200001/components/exo/surfac...
components/exo/surface.cc:65: }
nit: blank line.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:51: protected:
Can you mark this as private?

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:59: }
nit: leave blank line.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:80: protected:
Mark it as private?

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:87: }
nit: leave blank line.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:94: const
scoped_refptr<ThreadSafeSender> sender_;
Weird to make a scoped_refptr const.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
File content/renderer/child_frame_compositing_helper.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.h:89: const
base::WeakPtr<BrowserPlugin> browser_plugin_;
It's weird to make a WeakPtr const.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:29: :
m_service(m_service) {}
nit: new line.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:33: }
nit: new line

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:42:
const mojom::blink::OffscreenCanvasSurfacePtr& m_service;
This isn't safe because OffscreenCanvasSurfaceReferenceFactory can outlive
CanvasSurfaceLayerBridge. I'd instead hold a weakptr to
CanvasSurfaceLayerBridge.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:36: const
scoped_refptr<cc::SurfaceReferenceFactory> m_refFactory;
Weird to make this const.

https://codereview.chromium.org/2514033002/diff/1200001/ui/compositor/layer.h
File ui/compositor/layer.h (left):

https://codereview.chromium.org/2514033002/diff/1200001/ui/compositor/layer.h...
ui/compositor/layer.h:301: gfx::Size frame_size_in_dip);
Can we get rid of this?

[commit-bot: I haz the power, 2016-12-14 00:09:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 00:09:37]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2016-12-14 04:54:23]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface refrences
while hiding the implementation details from its client. So 
the client cannot tell if the underlying implementation
is SurfaceSequence or SurfaceReference, which paves the way for phasing 
out SurfaceSequence and replacing it with SurfaceReference. All require 
/ satisfy callbacks are now gone and are replaces with different
implementations of SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the underlying implementation is SurfaceSequence or SurfaceReference,
which paves the way for replacing SurfaceSequence with SurfaceReference. All
require / satisfy callbacks are now gone and are replaces with different
implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-14 04:54:57]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the underlying implementation is SurfaceSequence or SurfaceReference,
which paves the way for replacing SurfaceSequence with SurfaceReference. All
require / satisfy callbacks are now gone and are replaces with different
implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaces with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-14 17:39:41]

PTAL

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference.h:20: const SurfaceSequence& sequence()
const { return sequence_; }
On 2016/12/13 22:55:05, Fady Samuel wrote:
> If you don't need it, remove it.

Sorry. Turns out I do need it in SequenceSurfaceReferenceFactory's
implementation.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
File content/renderer/child_frame_compositing_helper.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.h:89: const
base::WeakPtr<BrowserPlugin> browser_plugin_;
Since it's used in the construction of surface_reference_factory_ in the
constructor, I need to make sure its value won't change.

[Saman Sami, 2016-12-14 17:50:02]

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
cc/layers/surface_layer.h:33: const scoped_refptr<SurfaceReferenceFactory>
surface_reference_factory()
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit: remove the const

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
File cc/layers/surface_layer_unittest.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/layers/surface_lay...
cc/layers/surface_layer_unittest.cc:84: };
On 2016/12/13 22:55:05, Fady Samuel wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference_factory.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.cc:25: // to the type we returned
in CreateReference.
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit; You should mention that SurfaceReference is a friend 

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.cc:33: // to the type we returned
in CreateReference.
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit; You should mention that SurfaceReference is a friend 

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.h:30: void
DestroyReference(SurfaceReferenceBase* surface_ref,
On 2016/12/13 22:55:05, Fady Samuel wrote:
> // SurfaceReferenceFactory implementation:

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.h:33: virtual void
RequireSequence(const SurfaceId& surface_id,
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit Move these to the top of the private section.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.cc:13: return;
On 2016/12/13 22:55:05, Fady Samuel wrote:
> Move this to the destructor:
> 
> if (factory_)
>   factory_->DestroyReference(this);

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.cc:35: : factory_(factory) {}
On 2016/12/13 22:55:05, Fady Samuel wrote:
> factory_(std::move(factory))

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:32: void Destroy();
On 2016/12/13 22:55:05, Fady Samuel wrote:
> Can you make this private?

Moved the implementation to the destructor

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:33: void Destroy(CompositorFrameMetadata*
metadata);
On 2016/12/13 22:55:05, Fady Samuel wrote:
> // TODO(fsamuel, samans, kylechar): Remove this.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:34: bool IsDestroyed() const;
On 2016/12/13 22:55:05, Fady Samuel wrote:
> Remove this if it's not used.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:36: const SurfaceReferenceFactory*
factory() const { return factory_.get(); }
On 2016/12/13 22:55:05, Fady Samuel wrote:
> Do we need this?

Nope. Removed.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_factory.h:23: : public
base::RefCounted<SurfaceReferenceFactory> {
On 2016/12/13 22:55:05, Fady Samuel wrote:
> RefCountedThreadSafe

Done.

https://codereview.chromium.org/2514033002/diff/1200001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_factory.h:39: friend class SurfaceReferenceBase;
On 2016/12/13 22:55:05, Fady Samuel wrote:
> Move friend statements above.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/components/exo/surfac...
File components/exo/surface.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/components/exo/surfac...
components/exo/surface.cc:61: }
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit: blank line.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/components/exo/surfac...
components/exo/surface.cc:65: }
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit: blank line.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:51: protected:
On 2016/12/13 22:55:06, Fady Samuel wrote:
> Can you mark this as private?

Done.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:59: }
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit: leave blank line.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:80: protected:
On 2016/12/13 22:55:05, Fady Samuel wrote:
> Mark it as private?

Done.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:87: }
On 2016/12/13 22:55:05, Fady Samuel wrote:
> nit: leave blank line.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/content/renderer/chil...
content/renderer/child_frame_compositing_helper.cc:94: const
scoped_refptr<ThreadSafeSender> sender_;
On 2016/12/13 22:55:06, Fady Samuel wrote:
> Weird to make a scoped_refptr const.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:29: :
m_service(m_service) {}
On 2016/12/13 22:55:06, Fady Samuel wrote:
> nit: new line.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:33: }
On 2016/12/13 22:55:06, Fady Samuel wrote:
> nit: new line

Done.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:42:
const mojom::blink::OffscreenCanvasSurfacePtr& m_service;
On 2016/12/13 22:55:06, Fady Samuel wrote:
> This isn't safe because OffscreenCanvasSurfaceReferenceFactory can outlive
> CanvasSurfaceLayerBridge. I'd instead hold a weakptr to
> CanvasSurfaceLayerBridge.

Will do in a future patch.

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2514033002/diff/1200001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:36: const
scoped_refptr<cc::SurfaceReferenceFactory> m_refFactory;
On 2016/12/13 22:55:06, Fady Samuel wrote:
> Weird to make this const.

Done.

https://codereview.chromium.org/2514033002/diff/1200001/ui/compositor/layer.h
File ui/compositor/layer.h (left):

https://codereview.chromium.org/2514033002/diff/1200001/ui/compositor/layer.h...
ui/compositor/layer.h:301: gfx::Size frame_size_in_dip);
On 2016/12/13 22:55:06, Fady Samuel wrote:
> Can we get rid of this?

I'll do it in a separate CL.

[Saman Sami, 2016-12-14 18:53:32]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 18:54:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-12-14 20:11:39]

a few more comments but otherwise this looks really good!

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/direct_su...
File cc/surfaces/direct_surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/direct_su...
cc/surfaces/direct_surface_reference_factory.h:24: : manager_(manager) {}
nit: move to cc file.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/direct_su...
cc/surfaces/direct_surface_reference_factory.h:34: SurfaceManager* manager_ =
nullptr;
no reason to do this if you're always initializing in the constructor.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:16: SurfaceInfo() = default;
Do you need this default constructor?

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:23: // To keep things as lightweight as
possible, the base class only keeps a
I wouldn't say it's to keep things lightweight but to keep the representation of
a reference flexible you only keep a pointer to the factory in the base and
subclasses decide what other info they need to keep.

https://codereview.chromium.org/2514033002/diff/1240001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/1240001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:44:
const mojom::blink::OffscreenCanvasSurfacePtr& m_service;
Hold a weakptr to CanvasSurfaceLayerBridge instead?

[Saman Sami, 2016-12-14 20:46:16]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 20:46:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-14 20:47:52]

PTAL

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/direct_su...
File cc/surfaces/direct_surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/direct_su...
cc/surfaces/direct_surface_reference_factory.h:24: : manager_(manager) {}
On 2016/12/14 20:11:39, Fady Samuel wrote:
> nit: move to cc file.

Done.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/direct_su...
cc/surfaces/direct_surface_reference_factory.h:34: SurfaceManager* manager_ =
nullptr;
On 2016/12/14 20:11:39, Fady Samuel wrote:
> no reason to do this if you're always initializing in the constructor.

Done.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:16: SurfaceInfo() = default;
On 2016/12/14 20:11:39, Fady Samuel wrote:
> Do you need this default constructor?

We might need it for mojo.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:23: // To keep things as lightweight as
possible, the base class only keeps a
On 2016/12/14 20:11:39, Fady Samuel wrote:
> I wouldn't say it's to keep things lightweight but to keep the representation
of
> a reference flexible you only keep a pointer to the factory in the base and
> subclasses decide what other info they need to keep.

But more information never hurts flexibility. I can keep surface id and owner,
but I don't because not all subclasses might want to use them and it will be an
unnecessary overhead.

[Fady Samuel, 2016-12-14 20:50:49]

fsamuel@chromium.org changed reviewers:
+ kylechar@chromium.org

[Fady Samuel, 2016-12-14 20:50:50]

fsamuel@chromium.org changed reviewers:
+ kylechar@chromium.org

[Fady Samuel, 2016-12-14 20:50:52]

Awesome! This LGTM at this point, but I'd like kylechar@ to take a look too to
make sure we can make this fit in with his work too. +kylechar@

[Fady Samuel, 2016-12-14 20:50:52]

Awesome! This LGTM at this point, but I'd like kylechar@ to take a look too to
make sure we can make this fit in with his work too. +kylechar@

[kylechar, 2016-12-14 20:51:53]

I've looked through it quickly, mind giving me a quick run down of how it's
supposed to fit together on the whiteboard?

https://codereview.chromium.org/2514033002/diff/1240001/cc/layers/surface_lay...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/layers/surface_lay...
cc/layers/surface_layer.cc:49: ref_factory_ = std::move(ref_factory);
ref_factory_(std::move(ref_factory))

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.h:24:
SequenceSurfaceReferenceFactory() = default;
Constructor is usually the first method.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:17: SurfaceInfo(const SurfaceId& id, float scale,
gfx::Size size)
const gfx::Size& size

https://codereview.chromium.org/2514033002/diff/1240001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2514033002/diff/1240001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:36:
scoped_refptr<cc::SurfaceReferenceFactory> m_refFactory;
ref_factory_

[Saman Sami, 2016-12-14 21:54:22]

PTAL

https://codereview.chromium.org/2514033002/diff/1240001/cc/layers/surface_lay...
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/layers/surface_lay...
cc/layers/surface_layer.cc:49: ref_factory_ = std::move(ref_factory);
On 2016/12/14 20:51:53, kylechar wrote:
> ref_factory_(std::move(ref_factory))

Done.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/sequence_...
File cc/surfaces/sequence_surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/sequence_...
cc/surfaces/sequence_surface_reference_factory.h:24:
SequenceSurfaceReferenceFactory() = default;
On 2016/12/14 20:51:53, kylechar wrote:
> Constructor is usually the first method.

Done.

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1240001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:17: SurfaceInfo(const SurfaceId& id, float scale,
gfx::Size size)
On 2016/12/14 20:51:53, kylechar wrote:
> const gfx::Size& size

Done.

https://codereview.chromium.org/2514033002/diff/1240001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2514033002/diff/1240001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:36:
scoped_refptr<cc::SurfaceReferenceFactory> m_refFactory;
On 2016/12/14 20:51:53, kylechar wrote:
> ref_factory_

I think third parties have their own naming convention.

[Saman Sami, 2016-12-15 00:05:22]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 00:06:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 00:09:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:09:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Saman Sami, 2016-12-15 17:12:43]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 17:13:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-15 18:43:39]

samans@chromium.org changed reviewers:
+ reveman@chromium.org

[Saman Sami, 2016-12-15 18:48:21]

samans@chromium.org changed reviewers:
+ jbauman@chromium.org

[Saman Sami, 2016-12-15 18:59:50]

samans@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-12-15 19:03:07]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaces with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-15 19:03:18]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaces with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2016-12-15 19:03:18]

samans@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2016-12-15 19:04:23]

blimp/ lgtm

[kylechar, 2016-12-15 19:08:45]

lgtm with nits.

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/direct_su...
File cc/surfaces/direct_surface_reference_factory.cc (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/direct_su...
cc/surfaces/direct_surface_reference_factory.cc:27: auto surface =
manager_->GetSurfaceForId(surface_id);
This is a Surface* right? The convention is to use auto* to make it clear it's a
pointer in that case.

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:32: float scale_ = 1.f;
Should this be device_scale_factor_? There are other scale factors that it could
be confused with, which might be confusing. Also update scale() ->
device_scale_factor() if you update this.

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_m...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_m...
cc/surfaces/surface_manager.h:20: #include
"cc/surfaces/direct_surface_reference_factory.h"
DirectSurfaceReferenceFactory isn't used in the header. You can import (or
forward declare) SurfaceReferenceFactory then import this in the cc.

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_factory.h:10: #include
"cc/surfaces/surface_reference_base.h"
If you're forward declaring SurfaceReferenceBase this isn't needed (or vice
versa).

[Saman Sami, 2016-12-15 19:10:40]

samans@chromium.org changed reviewers:
+ jbroman@chromium.org

[sky, 2016-12-15 19:11:24]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-12-15 19:11:27]

LGTM - but you need owners for the webkit and android changes (I'm not familiar
with those platforms).

https://codereview.chromium.org/2514033002/diff/1300001/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (right):

https://codereview.chromium.org/2514033002/diff/1300001/ui/compositor/BUILD.g...
ui/compositor/BUILD.gn:84: "//cc/surfaces:surfaces",
optional: you don't need ':surfaces' here and on line 197.

[Saman Sami, 2016-12-15 19:18:08]

samans@chromium.org changed reviewers:
+ junov@chromium.org
- jbauman@chromium.org

[Saman Sami, 2016-12-15 19:19:01]

samans@chromium.org changed reviewers:
+ jbauman@chromium.org

[Saman Sami, 2016-12-15 19:19:41]

samans@chromium.org changed reviewers:
- jbroman@chromium.org

[David Trainor- moved to gerrit, 2016-12-15 19:34:16]

ui/android lgtm

[Justin Novosad, 2016-12-15 19:40:13]

Webkit lgtm

[Justin Novosad, 2016-12-15 19:40:27]

Webkit lgtm

[commit-bot: I haz the power, 2016-12-15 19:45:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 19:45:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2016-12-15 19:52:37]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 19:53:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-15 19:53:52]

samans@chromium.org changed reviewers:
+ jam@chromium.org

[reveman, 2016-12-15 19:59:58]

lgtm % nits

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
File components/exo/surface.cc (right):

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.cc:49: class SurfaceReferenceFactory : public
cc::SequenceSurfaceReferenceFactory {
Please move this below the CustomWindowTargeter class. How about
SurfaceReferenceFactoryImpl? or follow the pattern below and call it
CustomSurfaceReferenceFactory.

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.cc:52: scoped_refptr<CompositorFrameSinkHolder>
sink_holder)
s/scoped_refptr<CompositorFrameSinkHolder>/CompositorFrameSinkHolder */ and
remove std::move below

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.cc:58: // cc::SequenceSurfaceReferenceFactory
implementation:
nit: "// Overridden from cc::SequenceSurfaceReferenceFactory:" for consistency

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.h:383: scoped_refptr<cc::SurfaceReferenceFactory>
ref_factory_;
nit: s/ref_factory_/surface_reference_factory_/ or reference_factory_ to avoid
abbreviations

Maybe also add a comment that explains what this is used for.

[jam, 2016-12-15 20:09:54]

content lgtm as long as someone else reviewed child_frame_compositing_helper.cc

[Fady Samuel, 2016-12-15 20:30:55]

On 2016/12/15 20:09:54, jam wrote:
> content lgtm as long as someone else reviewed
child_frame_compositing_helper.cc

I'm not an owner of this file, but I'm very familiar with it as it's used by
BrowserPlugin. LGTM child_frame_compositing_helper.cc FWIW

[Saman Sami, 2016-12-15 20:40:28]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 20:41:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-15 20:41:37]

Fixed nits

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/direct_su...
File cc/surfaces/direct_surface_reference_factory.cc (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/direct_su...
cc/surfaces/direct_surface_reference_factory.cc:27: auto surface =
manager_->GetSurfaceForId(surface_id);
On 2016/12/15 19:08:45, kylechar wrote:
> This is a Surface* right? The convention is to use auto* to make it clear it's
a
> pointer in that case.

Done.

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_m...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_m...
cc/surfaces/surface_manager.h:20: #include
"cc/surfaces/direct_surface_reference_factory.h"
On 2016/12/15 19:08:45, kylechar wrote:
> DirectSurfaceReferenceFactory isn't used in the header. You can import (or
> forward declare) SurfaceReferenceFactory then import this in the cc.

Done.

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_factory.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_factory.h:10: #include
"cc/surfaces/surface_reference_base.h"
On 2016/12/15 19:08:45, kylechar wrote:
> If you're forward declaring SurfaceReferenceBase this isn't needed (or vice
> versa).

Done.

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
File components/exo/surface.cc (right):

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.cc:49: class SurfaceReferenceFactory : public
cc::SequenceSurfaceReferenceFactory {
On 2016/12/15 19:59:58, reveman wrote:
> Please move this below the CustomWindowTargeter class. How about
> SurfaceReferenceFactoryImpl? or follow the pattern below and call it
> CustomSurfaceReferenceFactory.

Done.

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.cc:52: scoped_refptr<CompositorFrameSinkHolder>
sink_holder)
On 2016/12/15 19:59:58, reveman wrote:
> s/scoped_refptr<CompositorFrameSinkHolder>/CompositorFrameSinkHolder */ and
> remove std::move below

Done.

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.cc:58: // cc::SequenceSurfaceReferenceFactory
implementation:
On 2016/12/15 19:59:58, reveman wrote:
> nit: "// Overridden from cc::SequenceSurfaceReferenceFactory:" for consistency

Done.

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surface.h
File components/exo/surface.h (right):

https://codereview.chromium.org/2514033002/diff/1300001/components/exo/surfac...
components/exo/surface.h:383: scoped_refptr<cc::SurfaceReferenceFactory>
ref_factory_;
On 2016/12/15 19:59:58, reveman wrote:
> nit: s/ref_factory_/surface_reference_factory_/ or reference_factory_ to avoid
> abbreviations
> 
> Maybe also add a comment that explains what this is used for.

Done.

https://codereview.chromium.org/2514033002/diff/1300001/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (right):

https://codereview.chromium.org/2514033002/diff/1300001/ui/compositor/BUILD.g...
ui/compositor/BUILD.gn:84: "//cc/surfaces:surfaces",
On 2016/12/15 19:11:26, sky wrote:
> optional: you don't need ':surfaces' here and on line 197.

Done.

[jbauman, 2016-12-15 20:55:49]

lgtm

https://codereview.chromium.org/2514033002/diff/1320001/third_party/WebKit/So...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2514033002/diff/1320001/third_party/WebKit/So...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:46:
base::WeakPtr<CanvasSurfaceLayerBridge> m_bridge;
We really need to fix the satisfy swap promise soon, as using a weakptr from a
different thread really isn't thread-safe.

[commit-bot: I haz the power, 2016-12-16 00:16:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 00:16:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2016-12-16 12:24:10]

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.cc (right):

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.cc:25: void
SurfaceReferenceBase::Destroy(CompositorFrameMetadata* metadata) {
Get rid of this?

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:16: class CompositorFrameMetadata;
Get rid of this?

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:22: // until destroy is called.
nit: Destroy

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:35: void Destroy(CompositorFrameMetadata*
metadata);
I think you can get rid of this now?

[Saman Sami, 2016-12-16 15:59:56]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 16:00:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-16 16:05:34]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 16:05:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2016-12-16 16:14:35]

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.cc (right):

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.cc:25: void
SurfaceReferenceBase::Destroy(CompositorFrameMetadata* metadata) {
On 2016/12/16 12:24:09, Fady Samuel wrote:
> Get rid of this?

Done.

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
File cc/surfaces/surface_reference_base.h (right):

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:16: class CompositorFrameMetadata;
On 2016/12/16 12:24:09, Fady Samuel wrote:
> Get rid of this?

Done.

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:22: // until destroy is called.
On 2016/12/16 12:24:09, Fady Samuel wrote:
> nit: Destroy

Done.

https://codereview.chromium.org/2514033002/diff/1320001/cc/surfaces/surface_r...
cc/surfaces/surface_reference_base.h:35: void Destroy(CompositorFrameMetadata*
metadata);
On 2016/12/16 12:24:09, Fady Samuel wrote:
> I think you can get rid of this now?

Done.

[commit-bot: I haz the power, 2016-12-16 16:43:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 16:43:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2016-12-16 17:15:28]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 17:16:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-12-16 17:26:37]

still lgtm

[Saman Sami, 2016-12-16 17:37:35]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2016-12-16 17:37:40]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2016-12-16 17:39:16]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2016-12-16 17:39:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
kylechar@chromium.org, jam@chromium.org, junov@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2514033002/#ps1370001
(title: "up")

[commit-bot: I haz the power, 2016-12-16 17:39:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mfomitchev, 2016-12-16 19:18:11]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2016-12-16 19:18:14]

https://codereview.chromium.org/2514033002/diff/1370001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1370001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:14: class SurfaceInfo {
This is really similar to aura::SurfaceInfo. We should probably get rid of the
Aura one?

[Fady Samuel, 2016-12-16 19:18:54]

https://codereview.chromium.org/2514033002/diff/1370001/cc/surfaces/surface_i...
File cc/surfaces/surface_info.h (right):

https://codereview.chromium.org/2514033002/diff/1370001/cc/surfaces/surface_i...
cc/surfaces/surface_info.h:14: class SurfaceInfo {
On 2016/12/16 19:18:14, mfomitchev wrote:
> This is really similar to aura::SurfaceInfo. We should probably get rid of the
> Aura one?

+1 That is the intention. There should be a mojom in cc/ipc too.

[Fady Samuel, 2016-12-16 19:21:45]

On 2016/12/16 19:18:54, Fady Samuel wrote:
>
https://codereview.chromium.org/2514033002/diff/1370001/cc/surfaces/surface_i...
> File cc/surfaces/surface_info.h (right):
> 
>
https://codereview.chromium.org/2514033002/diff/1370001/cc/surfaces/surface_i...
> cc/surfaces/surface_info.h:14: class SurfaceInfo {
> On 2016/12/16 19:18:14, mfomitchev wrote:
> > This is really similar to aura::SurfaceInfo. We should probably get rid of
the
> > Aura one?
> 
> +1 That is the intention. There should be a mojom in cc/ipc too.

Not in this CL though :-)

[Saman Sami, 2016-12-16 21:28:44]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaces with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains the information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2016-12-16 22:23:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 22:23:47]

Failed to apply patch for blimp/client/core/compositor/blimp_compositor.cc:
While running git apply --index -p1;
  error: patch failed: blimp/client/core/compositor/blimp_compositor.cc:331
  error: blimp/client/core/compositor/blimp_compositor.cc: patch does not apply

Patch:       blimp/client/core/compositor/blimp_compositor.cc
Index: blimp/client/core/compositor/blimp_compositor.cc
diff --git a/blimp/client/core/compositor/blimp_compositor.cc
b/blimp/client/core/compositor/blimp_compositor.cc
index
c1e5c329d7d5ea43649b1ca4ca741003aed4750e..185742c154ff7376fbbc7364ebb689d42d062b69
100644
--- a/blimp/client/core/compositor/blimp_compositor.cc
+++ b/blimp/client/core/compositor/blimp_compositor.cc
@@ -38,30 +38,6 @@
 namespace blimp {
 namespace client {
 
-namespace {
-
-void SatisfyCallback(base::WeakPtr<cc::SurfaceManager> manager,
-                     const cc::SurfaceSequence& sequence) {
-  if (!manager)
-    return;
-  std::vector<uint32_t> sequences;
-  sequences.push_back(sequence.sequence);
-  manager->DidSatisfySequences(sequence.frame_sink_id, &sequences);
-}
-
-void RequireCallback(base::WeakPtr<cc::SurfaceManager> manager,
-                     const cc::SurfaceId& id,
-                     const cc::SurfaceSequence& sequence) {
-  cc::Surface* surface = manager->GetSurfaceForId(id);
-  if (!surface) {
-    LOG(ERROR) << "Attempting to require callback on nonexistent surface";
-    return;
-  }
-  surface->AddDestructionDependency(sequence);
-}
-
-}  // namespace
-
 class BlimpCompositor::FrameTrackingSwapPromise : public cc::SwapPromise {
  public:
   FrameTrackingSwapPromise(
@@ -331,12 +307,13 @@ void
BlimpCompositor::SubmitCompositorFrame(cc::CompositorFrame frame) {
     // manager must outlive compositors using it.
     cc::SurfaceManager* surface_manager =
         GetEmbedderDeps()->GetSurfaceManager();
-    scoped_refptr<cc::SurfaceLayer> content_layer = cc::SurfaceLayer::Create(
-        base::Bind(&SatisfyCallback, surface_manager->GetWeakPtr()),
-        base::Bind(&RequireCallback, surface_manager->GetWeakPtr()));
-    content_layer->SetSurfaceId(
-        cc::SurfaceId(surface_factory_->frame_sink_id(), local_frame_id_), 1.f,
-        surface_size, false /* strecth_content_to_fill_bounds */);
+    auto content_layer =
+        cc::SurfaceLayer::Create(surface_manager->reference_factory());
+    cc::SurfaceId surface_id(surface_factory_->frame_sink_id(),
+                             local_frame_id_);
+    content_layer->SetSurfaceInfo(
+        cc::SurfaceInfo(surface_id, 1.f, surface_size),
+        false /* stretch_content_to_fill_bounds */);
     content_layer->SetBounds(current_surface_size_);
     content_layer->SetIsDrawable(true);
     content_layer->SetContentsOpaque(true);

[Saman Sami, 2016-12-16 22:38:00]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2016-12-16 22:38:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, reveman@chromium.org, sky@chromium.org,
kylechar@chromium.org, jam@chromium.org, junov@chromium.org,
jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2514033002/#ps1390001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-16 22:39:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 00:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-17 00:41:20]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2016-12-17 00:45:58]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-12-17 00:46:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 02:44:20]

CQ is committing da patch.

Bot data: {"patchset_id": 1390001, "attempt_start_ts": 1481935558026280,
"parent_rev": "b50d4f13507cd79b0f96198da23fce753901b1de", "commit_rev":
"afc2917600c17d76bd70a61ed3040fc3fbc91676"}

[commit-bot: I haz the power, 2016-12-17 02:44:51]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2514033002
==========

[commit-bot: I haz the power, 2016-12-17 02:44:56]

Committed patchset #58 (id:1390001)

[commit-bot: I haz the power, 2016-12-17 02:49:02]

Description was changed from

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the 
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of 
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2514033002
==========

to

==========
Introducing SurfaceReferenceFactory

This CL introduces SurfaceReferenceFactory which can create surface references
while hiding the implementation details from its client. So the client cannot
tell if the
underlying implementation is SurfaceSequence or SurfaceReference, which paves
the
way for replacing SurfaceSequence with SurfaceReference. All require / satisfy
callbacks are now gone and are replaced with different implementations of
SurfaceReferenceFactory.

This CL also adds SurfaceInfo, which contains information
about an embedded surface that are usually passed around together.

BUG=659598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/72b2a28cb2ffa0dabe4505e14ba65c41c20495c5
Cr-Commit-Position: refs/heads/master@{#439299}
==========

[commit-bot: I haz the power, 2016-12-17 02:49:07]

Patchset 58 (id:??) landed as
https://crrev.com/72b2a28cb2ffa0dabe4505e14ba65c41c20495c5
Cr-Commit-Position: refs/heads/master@{#439299}