Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

BUG=401331
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4e3c9d5b507f8a7cfb99ef3cc855184943a1dbd8
Cr-Commit-Position: refs/heads/master@{#379988}

[enne (OOO), 2016-02-06 00:24:03]

Description was changed from

==========
[wip] surface factory client <-> begin frame source hookup
==========

to

==========
[wip] surface factory client <-> begin frame source hookup
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[enne (OOO), 2016-02-06 00:24:04]

Description was changed from

==========
[wip] surface factory client <-> begin frame source hookup
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[wip] surface factory client <-> begin frame source hookup
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-02-12 22:40:09]

Description was changed from

==========
[wip] surface factory client <-> begin frame source hookup
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[wip] surface factory client <-> begin frame source hookup

SurfaceManager now maintains a dag of surface id namespaces.  Optionally, a
single BeginFrameSource input can be attached to a single namespace node.  Every
namespace node also has a SurfaceFactoryClient.  This client is informed of a
current BeginFrameSource, which is chosen from any BeginFrameSource attached to
it or a parent of that node.

SurfaceManager is responsible for picking which source to use, of which it
currently just picks the first one until that source goes is removed after which
it arbitrarily picks another valid one.  In practice, this means that a window
moved to another display in ChromeOS will switch its BeginFrameSource after the
window is dropped onto the new window.

Because the users of this dag all have very different requirements, the ordering
of SurfaceFactoryClient registration, namespace hierarchy registration, and
BeginFrameSource attaching are not particularly strict.

* BeginFrameSources can only be attached to namespaces with a
SurfaceFactoryClient, and must be detached before unregistering that client.
* Namespace hierarchy may be registered before or after the SurfaceFactoryClient
for the parent or child has been registered.
* Namespace hierarchy may be unregistered before or after the
SurfaceFactoryClient for the parent or child has been unregistered.
* Any registered client, BeginFrameSource, or namespace hierarchy should be
unregistered and is not implicitly cleaned up.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-02-12 22:43:29]

Description was changed from

==========
[wip] surface factory client <-> begin frame source hookup

SurfaceManager now maintains a dag of surface id namespaces.  Optionally, a
single BeginFrameSource input can be attached to a single namespace node.  Every
namespace node also has a SurfaceFactoryClient.  This client is informed of a
current BeginFrameSource, which is chosen from any BeginFrameSource attached to
it or a parent of that node.

SurfaceManager is responsible for picking which source to use, of which it
currently just picks the first one until that source goes is removed after which
it arbitrarily picks another valid one.  In practice, this means that a window
moved to another display in ChromeOS will switch its BeginFrameSource after the
window is dropped onto the new window.

Because the users of this dag all have very different requirements, the ordering
of SurfaceFactoryClient registration, namespace hierarchy registration, and
BeginFrameSource attaching are not particularly strict.

* BeginFrameSources can only be attached to namespaces with a
SurfaceFactoryClient, and must be detached before unregistering that client.
* Namespace hierarchy may be registered before or after the SurfaceFactoryClient
for the parent or child has been registered.
* Namespace hierarchy may be unregistered before or after the
SurfaceFactoryClient for the parent or child has been unregistered.
* Any registered client, BeginFrameSource, or namespace hierarchy should be
unregistered and is not implicitly cleaned up.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.

* BeginFrameSources can only be attached to namespaces with a
  SurfaceFactoryClient, and must be detached before unregistering that
  client.

* Namespace hierarchy may be registered before or after the
  SurfaceFactoryClient for the parent or child has been registered.

* Namespace hierarchy may be unregistered before or after the
  SurfaceFactoryClient for the parent or child has been unregistered.

* Any registered client, BeginFrameSource, or namespace hierarchy
  should be unregistered and is not implicitly cleaned up.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-02-12 22:45:13]

enne@chromium.org changed reviewers:
+ jbauman@chromium.org

[enne (OOO), 2016-02-12 22:45:13]

enne@chromium.org changed reviewers:
+ jbauman@chromium.org

[enne (OOO), 2016-02-12 22:53:10]

Wanted to send this out and get some initial feedback.  Lots of files are
changed here, but SurfaceManager and its unit tests are the most important
thing.

This implements the SurfaceManager portion of what I describe here:
https://docs.google.com/document/d/1QK0FAu2qRUsAvyuXJyFn9hesP4y8BSN7ew50qr_Bd...

Thing that I know need more work: SurfaceManager needs more unit tests for sure.
 The SurfaceManager API is a bit wordy and maybe RegisterSurfaceFactoryClient
should have an explicit unregister pair like the rest.  (It also doesn't really
interact with anything else in SurfaceManager, which makes me wonder if it
should live elsewhere?)  Happy also to split this up in any way that makes
things simpler.

Next steps after this are to hook up the SetBeginFrameSource for
DelegatedFrameHost / SurfaceDisplayOutputSurface.  Then, hook up namespace
hierarchies for OOPIF/mus.  Then hook up their SetBeginFrameSources.

[brianderson, 2016-02-18 23:01:06]

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

[brianderson, 2016-02-18 23:01:08]

Haven't looked at it all, but it's looking good. All comments I left are just
nits.

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:136: SurfaceFactoryClient* client) {
DCHECK namespace is valid?

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:140: if (BeginFrameSource* source =
namespace_client_map_[id_namespace].source) {
Cache map lookup?

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:152: for (auto iter : registered_sources_)
Maybe use a name other than "iter".

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:156: namespace_client_map_.erase(iter);
Does it make a difference whether this is erased here vs. in
UnregisterSurfaceIdNamespace? If it were there, could you DCHECK is_empty()
instead?

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:182: for (auto iter : registered_sources_)
Nice.

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:190: if (!data.source) {
Can you add a comment regarding the purpose of this if? It's not immediately
obvious without understanding how UnregisterBeginFrameSource works.

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:202: BeginFrameSource* source) {
Maybe add some DCHECKS like you have in the other methods.

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.h:78: void RegisterSurfaceFactoryClient(uint32_t
id_namespace,
Is it possible to fold this into RegisterSurfaceIdNamespace? Or will the
SurfaceFactoryClient not always be known by that point?

[enne (OOO), 2016-02-19 21:26:57]

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.h:78: void RegisterSurfaceFactoryClient(uint32_t
id_namespace,
On 2016/02/18 at 23:01:07, brianderson wrote:
> Is it possible to fold this into RegisterSurfaceIdNamespace? Or will the
SurfaceFactoryClient not always be known by that point?

Sadly, no.  SurfaceIdNamespace is about the allocator, which does not have the
same lifetime as the SurfaceFactoryClient.  I thought about refactoring this so
that surface factories owned their allocators and this could be combined.  To
that end, I talked about this with jbauman some, who said that there were goals
of moving the allocator off even farther away than it is now, so I think it's
important to keep them decoupled to enable that future refactoring.

[jbauman, 2016-02-24 19:52:34]

Sorry my review is so late. This lgtm with Brian's comments addressed.

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:189: auto& data =
namespace_client_map_[id_namespace];
One thing we'll have to be careful of is to make sure we don't have infinite
recursion here, either by keeping a set of namespaces that we've already looked
at in the current recursion, or by just being certain
RegisterSurfaceNamespaceHierarchy is only ever called by trusted code.

[enne (OOO), 2016-02-24 21:19:03]

On 2016/02/24 at 19:52:34, jbauman wrote:
> Sorry my review is so late. This lgtm with Brian's comments addressed.

No worries, I was just going to get back to it today, so this is perfect.  I'll
put up a final version with a few more tests and address all these comments.

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/1673783004/diff/20001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:189: auto& data =
namespace_client_map_[id_namespace];
On 2016/02/24 at 19:52:34, jbauman wrote:
> One thing we'll have to be careful of is to make sure we don't have infinite
recursion here, either by keeping a set of namespaces that we've already looked
at in the current recursion, or by just being certain
RegisterSurfaceNamespaceHierarchy is only ever called by trusted code.

Mmm, that's a really good point.  Rather than keeping a list, I *think* it would
be sufficient to just pass along the parent of the newly added hierarchy into
the recursion, and if that parent is not seen in the recursion, then no new
loops have been added.

[enne (OOO), 2016-02-25 23:43:28]

Description was changed from

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.

* BeginFrameSources can only be attached to namespaces with a
  SurfaceFactoryClient, and must be detached before unregistering that
  client.

* Namespace hierarchy may be registered before or after the
  SurfaceFactoryClient for the parent or child has been registered.

* Namespace hierarchy may be unregistered before or after the
  SurfaceFactoryClient for the parent or child has been unregistered.

* Any registered client, BeginFrameSource, or namespace hierarchy
  should be unregistered and is not implicitly cleaned up.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-02-25 23:45:17]

* addressed brianderson's comments
* added a recursion check
* renamed api to all be register/unregister for consistency
* added test for all the permutations of register/unregister ordering to make
sure SurfaceManager data structures behave sanely

[enne (OOO), 2016-02-25 23:45:32]

If you don't mind taking one more look, I'd appreciate it.  :)

[jbauman, 2016-02-26 23:58:49]

Great! lgtm.

[enne (OOO), 2016-02-27 00:01:14]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-02-27 00:02:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/40001

[commit-bot: I haz the power, 2016-02-27 00:19:09]

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

[commit-bot: I haz the power, 2016-02-27 00:19:11]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2016-02-29 19:59:39]

enne@chromium.org changed reviewers:
+ boliu@chromium.org, erg@chromium.org, sievers@chromium.org

[enne (OOO), 2016-02-29 19:59:40]

OWNERS:

erg:
  components/mus/surfaces/top_level_display_client.cc
  components/mus/surfaces/top_level_display_client.h
  components/mus/ws/server_window_surface.cc
  components/mus/ws/server_window_surface.h
  components/mus/ws/server_window_surface_manager.cc
  components/mus/ws/server_window_surface_manager.h
sievers:
  content/browser/compositor/delegated_frame_host.cc
  content/browser/compositor/delegated_frame_host.h
  content/browser/frame_host/render_widget_host_view_child_frame.cc
  content/browser/frame_host/render_widget_host_view_child_frame.h
  content/browser/renderer_host/render_widget_host_view_android.cc
  content/browser/renderer_host/render_widget_host_view_android.h
boliu:
  android_webview/browser/hardware_renderer.cc
  android_webview/browser/hardware_renderer.h

[no sievers, 2016-02-29 20:23:18]

lgtm

[Elliot Glaysher, 2016-02-29 20:28:23]

Description was changed from

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Elliot Glaysher, 2016-02-29 20:28:23]

erg@chromium.org changed reviewers:
+ sky@chromium.org
- erg@chromium.org

[Elliot Glaysher, 2016-02-29 20:28:59]

On 2016/02/29 19:59:40, enne wrote:
> OWNERS:
> 
> erg:
>   components/mus/surfaces/top_level_display_client.cc
>   components/mus/surfaces/top_level_display_client.h
>   components/mus/ws/server_window_surface.cc
>   components/mus/ws/server_window_surface.h
>   components/mus/ws/server_window_surface_manager.cc
>   components/mus/ws/server_window_surface_manager.h

erg -> sky

(I'm not an expert in the interaction with cc here.)

[boliu, 2016-02-29 20:45:48]

rs lgtm % compile

[sky, 2016-03-01 03:41:44]

https://codereview.chromium.org/1673783004/diff/120001/components/mus/ws/serv...
File components/mus/ws/server_window_surface.cc (right):

https://codereview.chromium.org/1673783004/diff/120001/components/mus/ws/serv...
components/mus/ws/server_window_surface.cc:49: cc::SurfaceManager* manager =
manager_->GetSurfaceManager();
manager_ might already be through it's destructor. Seems best to make
~ServerWindowSurfaceManager to explicitly empty type_to_surface_map_ so that
ServerWindowSurfaceManager is still valid. Alternatively make this cache
cc::SurfaceManager. Lastly, naming a variable manager that is not the same as
'manager_' is confusing.

[enne (OOO), 2016-03-01 22:20:33]

https://codereview.chromium.org/1673783004/diff/120001/components/mus/ws/serv...
File components/mus/ws/server_window_surface.cc (right):

https://codereview.chromium.org/1673783004/diff/120001/components/mus/ws/serv...
components/mus/ws/server_window_surface.cc:49: cc::SurfaceManager* manager =
manager_->GetSurfaceManager();
On 2016/03/01 at 03:41:44, sky wrote:
> manager_ might already be through it's destructor. Seems best to make
~ServerWindowSurfaceManager to explicitly empty type_to_surface_map_ so that
ServerWindowSurfaceManager is still valid. Alternatively make this cache
cc::SurfaceManager. Lastly, naming a variable manager that is not the same as
'manager_' is confusing.

Ah, good catch.  Changed the manager dtor to clear the map.  (Also, renamed that
variable.  Oops.)

[sky, 2016-03-02 00:23:53]

LGTM

[enne (OOO), 2016-03-02 19:04:00]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-03-02 19:04:01]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
jbauman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1673783004/#ps160001
(title: "Fix HasAnySurface;register id namespace correctly")

[commit-bot: I haz the power, 2016-03-02 19:04:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/160001

[enne (OOO), 2016-03-02 19:04:56]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-03-02 19:05:03]

Description was changed from

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

BUG=401331
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[enne (OOO), 2016-03-02 19:05:19]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-02 19:05:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/160001

[commit-bot: I haz the power, 2016-03-02 19:45:24]

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

[commit-bot: I haz the power, 2016-03-02 19:45:29]

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

[enne (OOO), 2016-03-02 22:04:40]

sky, sorry but I had a couple of mus unit tests failures I didn't see before.  I
prefer the tests to be more real than to add robustness to missing pieces in
production code, so I made these changes.  Just want a sanity check as this is
not an area I am at all familiar with.

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/test...
File components/mus/ws/test_server_window_delegate.cc (right):

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/test...
components/mus/ws/test_server_window_delegate.cc:14: : root_window_(nullptr),
surfaces_state_(new SurfacesState()) {}
Added in a real surfaces state so there was a real cc::SurfaceManager both here
and in the window_tree_unittest.cc.

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/wind...
File components/mus/ws/window_finder_unittest.cc (right):

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/wind...
components/mus/ws/window_finder_unittest.cc:30: ServerWindow
child2(&window_delegate, WindowId(1, 4));
I think WindowIds need to be unique? This failed before because they now both
try to register the same surface id namespace, which is derived from their
WindowId, and this asserts.

[sky, 2016-03-02 23:45:35]

SLGTM

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/test...
File components/mus/ws/test_server_window_delegate.cc (right):

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/test...
components/mus/ws/test_server_window_delegate.cc:14: : root_window_(nullptr),
surfaces_state_(new SurfacesState()) {}
On 2016/03/02 22:04:40, enne wrote:
> Added in a real surfaces state so there was a real cc::SurfaceManager both
here
> and in the window_tree_unittest.cc.

SG

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/wind...
File components/mus/ws/window_finder_unittest.cc (right):

https://codereview.chromium.org/1673783004/diff/180001/components/mus/ws/wind...
components/mus/ws/window_finder_unittest.cc:30: ServerWindow
child2(&window_delegate, WindowId(1, 4));
On 2016/03/02 22:04:40, enne wrote:
> I think WindowIds need to be unique? This failed before because they now both
> try to register the same surface id namespace, which is derived from their
> WindowId, and this asserts.

Good fix. Window ids should definitely be uniqute.

[enne (OOO), 2016-03-03 01:18:46]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-03-03 01:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
sky@chromium.org, jbauman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1673783004/#ps180001
(title: "Fix mus unit tests")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/180001

[commit-bot: I haz the power, 2016-03-03 02:07:41]

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

[commit-bot: I haz the power, 2016-03-03 02:07:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2016-03-03 18:47:05]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-03 18:47:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/180001

[commit-bot: I haz the power, 2016-03-03 22:40:36]

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

[commit-bot: I haz the power, 2016-03-03 22:40:38]

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

[enne (OOO), 2016-03-03 22:41:27]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-03 22:41:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/180001

[commit-bot: I haz the power, 2016-03-04 01:26:29]

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

[commit-bot: I haz the power, 2016-03-04 01:26:31]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[enne (OOO), 2016-03-04 01:31:03]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-04 01:31:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/180001

[commit-bot: I haz the power, 2016-03-04 01:35:46]

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

[commit-bot: I haz the power, 2016-03-04 01:35:48]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[enne (OOO), 2016-03-04 19:43:18]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-03-04 19:43:18]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
sky@chromium.org, jbauman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1673783004/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-04 19:43:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/200001

[commit-bot: I haz the power, 2016-03-04 21:11:41]

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

[commit-bot: I haz the power, 2016-03-04 21:11:43]

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

[enne (OOO), 2016-03-04 21:23:03]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-04 21:23:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/200001

[commit-bot: I haz the power, 2016-03-04 23:36:26]

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

[commit-bot: I haz the power, 2016-03-04 23:36:28]

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

[boliu, 2016-03-05 00:42:23]

On 2016/03/04 23:36:28, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

Tried this locally. Webview is falling over with this DCHECK failure:

03-05 00:40:47.107  8296  8356 F chromium: [FATAL:surface_manager.cc(141)] Check
failed:

signal 6 (SIGABRT), code -6 in tid 8356 (RenderThread)
pid: 8296, tid: 8356, name: RenderThread  >>> org.chromium.webview_shell <<<
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
[FATAL:surface_manager.cc(141)] Check failed:
valid_surface_id_namespaces_.count(id_namespace) == 1u (0 vs. 1)

Stack Trace:
  RELADDR   FUNCTION                      FILE:LINE
  000792af  ~LogMessage                   base/logging.cc:521
  000157a7  RegisterSurfaceFactoryClient  cc/surfaces/surface_manager.cc:141
  0006ac25  HardwareRenderer             
android_webview/browser/hardware_renderer.cc:56
  000770e5  DrawGL                       
android_webview/browser/shared_renderer_state.cc:275

[enne (OOO), 2016-03-07 18:58:48]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-03-07 18:58:49]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
sky@chromium.org, jbauman@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1673783004/#ps220001
(title: "Register id namespace on Android")

[enne (OOO), 2016-03-07 18:59:38]

Thanks for that.  I had misremembered this passing on a previous run and thought
it was just neverending flake.  That's an easy fix--just needs the namespace to
be registered with surface manager.

[commit-bot: I haz the power, 2016-03-07 18:59:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/220001

[commit-bot: I haz the power, 2016-03-07 20:53:41]

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

[commit-bot: I haz the power, 2016-03-07 20:53:44]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[enne (OOO), 2016-03-08 22:26:46]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-08 22:27:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673783004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673783004/220001

[enne (OOO), 2016-03-08 22:27:17]

android bot still failed, but it looked like it couldn't dismiss a popup? I
verified locally that my latest change fixed that webview dcheck that boliu saw.
 Will give this another run.

[commit-bot: I haz the power, 2016-03-09 00:25:22]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-09 00:26:18]

Description was changed from

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

BUG=401331
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

SurfaceManager now maintains a dag of surface id namespaces.
Optionally, a single BeginFrameSource input can be attached to a single
namespace node.  Every namespace node also has a SurfaceFactoryClient.
This client is informed of a current BeginFrameSource, which is chosen
from any BeginFrameSource attached to it or a parent of that node.
Any children of that namespace also are able to use that source.

SurfaceManager is responsible for picking which source to use, of which
it currently just picks the first one until that source goes is removed
after which it arbitrarily picks another valid one.  In practice, this
means that a window moved to another display in ChromeOS will switch its
BeginFrameSource after the window is dropped onto the new window.

Because the users of this dag all have very different requirements, the
ordering of SurfaceFactoryClient registration, namespace hierarchy
registration, and BeginFrameSource attaching are not particularly
strict.  BeginFrameSources, SurfaceFactoryClients, and hierarchies can
be registered and unregistered in any order with respect to each other.

BUG=401331
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4e3c9d5b507f8a7cfb99ef3cc855184943a1dbd8
Cr-Commit-Position: refs/heads/master@{#379988}
==========

[commit-bot: I haz the power, 2016-03-09 00:26:21]

Patchset 12 (id:??) landed as
https://crrev.com/4e3c9d5b507f8a7cfb99ef3cc855184943a1dbd8
Cr-Commit-Position: refs/heads/master@{#379988}