Cleans up usage of ViewManagerServiceImpl::roots_

Cleans up usage of ViewManagerServiceImpl::roots_

. roots_ now always contains the root nodes. Previously I would add a
  fake id if a connection had roots_ and they were all removed. I'ved
  nuked that too.
. Fixed bug in ProcessNodeHierarchyChanged. Specifically if a
  connection added a new node that it didn't previously know about the
  set of known nodes needed to be updated. This is only matters for
  the window manager and may only come up in tests.
. GetUnknownNodes needed to check access policy.
. All children are now removed as part of an embed. Previously this
  wasn't done for the wm, but I can't see a good reason to keep
  that.
. Node::Destroy() would never complete if any of the children were
  from another connection.
. Client lib wasn't properly mapping in nodes from another
  connection.
. Updated a couple of tests appropriately.

BUG=397660
TEST=covered by tests
R=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285837

[sky, 2014-07-26 16:21:39]

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/node.cc (right):

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/node.cc:223: child->LocalDestroy();
I think this is right, but now I'm worried that 215 needs to do the same.

[Ben Goodger (Google), 2014-07-26 16:49:10]

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/node.cc (right):

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/node.cc:223: child->LocalDestroy();
On 2014/07/26 16:21:39, sky wrote:
> I think this is right, but now I'm worried that 215 needs to do the same.

Yeah I would be inclined to think so too...

It should be noted in a comment here that this case basically only applies to
the window manager. No other connection has the ability to see child nodes it
doesn't own (right?).

It should be possible to test this by constructing a hierarchy with a non-owned
child & verifying that ViewManager::GetNodeById(that_id) returns NULL since
LocalDestroy should unregister it.

https://codereview.chromium.org/424533002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/ids.h (right):

https://codereview.chromium.org/424533002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/ids.h:15: // Connection id used to indicate no
connection. That is, no
Connection id is used to

[sky, 2014-07-26 20:46:23]

Take another look?

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/node.cc (right):

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/node.cc:223: child->LocalDestroy();
On 2014/07/26 16:49:10, Ben Goodger (Google) wrote:
> On 2014/07/26 16:21:39, sky wrote:
> > I think this is right, but now I'm worried that 215 needs to do the same.
> 
> Yeah I would be inclined to think so too...
> 
> It should be noted in a comment here that this case basically only applies to
> the window manager. No other connection has the ability to see child nodes it
> doesn't own (right?).

Child nodes yes, but the root(s) of the connection are not owned either.

> It should be possible to test this by constructing a hierarchy with a
non-owned
> child & verifying that ViewManager::GetNodeById(that_id) returns NULL since
> LocalDestroy should unregister it.

When I made 215 do a LocalDestroy too that broke
ViewManagerTest.DestroySecurity. In thinking about it a bit more I don't think
223 or 215 should delete. After all, unowned nodes aren't being deleted from the
server. They still exist (well, the window manager can delete things but this
code isn't allowing that). I think the erase is right with no delete. But
~ViewManagerClientImpl needs to explicitly delete else we leak these nodes.

https://codereview.chromium.org/424533002/diff/20001/mojo/services/view_manag...
File mojo/services/view_manager/ids.h (right):

https://codereview.chromium.org/424533002/diff/20001/mojo/services/view_manag...
mojo/services/view_manager/ids.h:15: // Connection id used to indicate no
connection. That is, no
On 2014/07/26 16:49:10, Ben Goodger (Google) wrote:
> Connection id is used to

Done.

[Ben Goodger (Google), 2014-07-26 22:56:15]

lgtm

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/node.cc (right):

https://codereview.chromium.org/424533002/diff/20001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/node.cc:223: child->LocalDestroy();
OK

[sky, 2014-07-27 00:09:51]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-07-27 00:10:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/424533002/40001

[commit-bot: I haz the power, 2014-07-27 11:30:12]

Change committed as 285837