Is the SurfaceId valid if only one of the two fields is set? Should FrameSinkId::is_null() and LocalFrameId::is_null() also be changed to is_valid()? This might make my debugging the invalid UnguessableToken much easier.

Another opened ended question, I guess for danakj@ and enne@:

I want to unify all things that are service-side for CompositorFrameSinks: ui::Compositor, DelegatedFrameHost, RenderWidgetHostViewChildFrame, etc because at that the end of the day, I'd like all this code to live in the Mus-gpu process and not the browser process.

Service-side, the implementation of CompositorFrameSink will always have a SurfaceFactory, right? SurfaceFactory's API and CompositorFrameSink's API, and SurfaceFactoryClient and CompositorFrameSinkClient's APIs seem to be converging. It seems like they can become one thing one day? WDYT? Is there any reason why SurfaceFactory != service-side CompositorFrameSink?

I think that sounds reasonable to me, re: SurfaceFactory and service-side CompositorFrameSink (is there a better name for this?).  I mostly just can't think if there's ever going to be something that receives compositor frames but then does something else with them instead of giving them to the SurfaceManager directly.  I don't think so, though.

> I mostly just can't think if there's ever going to be something that receives
> compositor frames but then does something else with them instead of giving them
> to the SurfaceManager directly.  I don't think so, though.

If there is, it could be a different implementation of the mojo api? Or does that not work.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/067f5824a3d561b9a16774fed995c23b6854405f

commit 067f5824a3d561b9a16774fed995c23b6854405f
Author: staraz <staraz@chromium.org>
Date: Mon Nov 07 21:06:41 2016

Added a command line switch kRenderClientId. It holds the process ID of a browser process and is passed to a renderer process when the render process is created. RenderWidget stores the client ID into its client_id_ field and use it to construct a FrameSinkID when RenderWidgetCompositor is created in RenderWidget::InitializeLayerTreeView.

This change should fix a bug where SurfaceSequence::frame_sink_id is empty in SurfaceLayer::SatisfyDestroySequence().

BUG= 656639 

Review-Url: https://codereview.chromium.org/2447143003
Cr-Commit-Position: refs/heads/master@{#430375}

[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/common/view_messages.h
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/public/common/content_switches.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/public/common/content_switches.h
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/public/renderer/render_thread.h
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/public/test/mock_render_thread.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/public/test/mock_render_thread.h
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/renderer/render_thread_impl.h
[modify] https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f/content/renderer/render_widget.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f907282ab306ec48f3392350b4269ce8f71257b2

commit f907282ab306ec48f3392350b4269ce8f71257b2
Author: staraz <staraz@chromium.org>
Date: Wed Nov 09 21:06:58 2016

This CL changes is_null() in SurfaceId, FrameSinkId and LocalFrameId
(IDs in the following paragraphs) to is_valid() with slightly different
logic.

is_null() returns false if any field of the class is set to a non-
default value. We used to assume that an ID being not null is
equivalent to being valid.

is_valid() returns true if all fields of an ID are set (FrameSinkId is
a special case where FrameSinkId::is_valid() returns true if either of
its fields is non-zero). Thus, a valid ID is always not null but not
the other way around. is_valid() poses a more strict check than
!is_null() and would help us catch bugs where a class is only partially
initialized.

For example, CL 2447143003 fixed a bug where we were not waiting long
enough for LayerTreeHostInProcess::SetFrameSinkId() to be called by an
IPC. The bug resulted in SurfaceLayer::destroy_sequence_ having an
empty FrameSinkId when SurfaceLayer::SatisfyDestroySequence() is
called. destroy_sequence_ had non-zero sequence_ and thus is not null.
Using is_valid() instead of is_null() would help us catch bugs like
this.

Moreover, a LocalFrameId with a 0 (uninitialized) nonce is invalid but
it may pass the !is_null() check if the local_id is set. We do not want
to deal with invalid LocalFrameIds in the browser or window server.

BUG= 656639 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

TBR=dtrainor@chromium.org;torne@chromium.org

Review-Url: https://codereview.chromium.org/2425923003
Cr-Commit-Position: refs/heads/master@{#431029}

[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/blimp/client/core/compositor/blimp_compositor.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/layers/surface_layer.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/layers/surface_layer_impl.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/layers/surface_layer_impl_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/layers/surface_layer_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/direct_compositor_frame_sink.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/display.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/frame_sink_id.h
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/local_frame_id.h
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/surface_aggregator_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/surface_id.h
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/surfaces/surface_sequence.h
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/cc/test/test_compositor_frame_sink.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/components/exo/surface.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/renderer/render_widget.h
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/content/test/content_browser_test_utils_internal.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/services/ui/public/cpp/window.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/services/ui/ws/frame_generator.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/services/ui/ws/server_window_compositor_frame_sink.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/ui/android/delegated_frame_host_android.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/ui/aura/mus/window_port_mus.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/ui/compositor/compositor.cc
[modify] https://crrev.com/f907282ab306ec48f3392350b4269ce8f71257b2/ui/compositor/layer.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9bab2153937afee58c5e9f467c1c4160e575cb6

commit c9bab2153937afee58c5e9f467c1c4160e575cb6
Author: staraz <staraz@chromium.org>
Date: Thu Nov 10 19:27:39 2016

Replaced cc::Display::SetSurfaceId() with SetLocalFrameId()

Display's FrameSinkId should never change. Therefore, the field is set to const
and is now set in the constructor rather than Initialize().

BUG= 656639 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2468633002
Cr-Commit-Position: refs/heads/master@{#431315}

[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/blimp/client/support/compositor/blimp_embedder_compositor.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/cc/surfaces/direct_compositor_frame_sink.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/cc/surfaces/direct_compositor_frame_sink_unittest.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/cc/surfaces/display.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/cc/surfaces/display.h
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/cc/surfaces/display_unittest.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/cc/test/test_compositor_frame_sink.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/services/ui/ws/server_window_compositor_frame_sink.cc
[modify] https://crrev.com/c9bab2153937afee58c5e9f467c1c4160e575cb6/ui/compositor/test/in_process_context_factory.cc

3 is not done, but it's already captured in another bug so I'm calling this one FIXED.