Create a RegisterViewAssociate method in ViewManager

Create a RegisterViewAssociate method in ViewManager
Add tests for ViewManager and Views

#703

R=jeffbrown@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/dec9013739bd49292096072912197341fa056795

[mikejurka, 2016-05-05 00:16:15]

mikejurka@google.com changed reviewers:
+ jeffbrown@google.com

[jeffbrown, 2016-05-10 19:15:15]

some comments that might help

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
File services/ui/view_manager/tests/view_manager_test.cc (right):

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:17: static const
base::TimeDelta kDefaultMessageDelay =
static is implied because this is in an anonymous namespace

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:21: std::cout << str <<
std::endl << std::flush;
consider using LOG(INFO) from #include "base/logging.h", it's a bit easier to
work with

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:24: class ViewAssociateMock
: public mojo::ui::ViewAssociate {
rename MockViewAssociate

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:27: int
num_times_connect_called_ = 0;
order of members is methods, then fields, so this should be at the end

also _ suffix is only used for private fields, so you can remove it here

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:84:
mojo::StrongBinding<mojo::ui::ViewListener> view_listener_binding_;
use Binding here, not StrongBinding

I also recommend moving the listener to another class, perhaps MockView (like
BaseView) or MockViewListener.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:85: mojo::ui::ViewOwnerPtr
client_view_owner_;
I don't think this is used.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:103:
mojo::ui::ViewManagerPtr view_manager;
Consider moving this block into Setup() and making view_manager a (protected)
field.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:114:
GetProxy(&client_view_owner), view_listener.Pass(),
mojo::GetProxy (not sure why this compiles since mojo isn't in scope here)

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:121:
mojo::StrongBinding<mojo::ui::ViewAssociate> view_associate_binding(
This almost but not quite does what you want.  Unfortunately StrongBinding
doesn't take ownership of the implementation object, it only destroys it on
error.  It's basically only useful in one pattern.

Fortunately we can do something simpler here.  If you allocate MockViewAssociate
on the stack then follow it with an Binding, then the objects get destroyed in
the reverse order which is what you want, so nothing leaks.

MockViewAssociate associate_impl;
mojo::ui::ViewAssociatePtr associate;
mojo::Binding<mojo::ui::ViewAssociate> associate_binding(associate,
mojo::GetProxy(associate));

[jeffbrown, 2016-05-10 19:54:21]

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
File services/ui/view_manager/tests/view_manager_test.cc (right):

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:121:
mojo::StrongBinding<mojo::ui::ViewAssociate> view_associate_binding(
On 2016/05/10 19:15:15, jeffbrown wrote:
> This almost but not quite does what you want.  Unfortunately StrongBinding
> doesn't take ownership of the implementation object, it only destroys it on
> error.  It's basically only useful in one pattern.
> 
> Fortunately we can do something simpler here.  If you allocate
MockViewAssociate
> on the stack then follow it with an Binding, then the objects get destroyed in
> the reverse order which is what you want, so nothing leaks.
> 
> MockViewAssociate associate_impl;
> mojo::ui::ViewAssociatePtr associate;
> mojo::Binding<mojo::ui::ViewAssociate> associate_binding(associate,
> mojo::GetProxy(associate));

Whoops, I meant:

mojo::Binding<mojo::ui::ViewAssociate> associate_binding(&associate_impl,
mojo::GetProxy(associate));

[mikejurka, 2016-05-10 23:17:24]

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
File services/ui/view_manager/tests/view_manager_test.cc (right):

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:17: static const
base::TimeDelta kDefaultMessageDelay =
On 2016/05/10 19:15:14, jeffbrown wrote:
> static is implied because this is in an anonymous namespace

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:21: std::cout << str <<
std::endl << std::flush;
On 2016/05/10 19:15:15, jeffbrown wrote:
> consider using LOG(INFO) from #include "base/logging.h", it's a bit easier to
> work with

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:24: class ViewAssociateMock
: public mojo::ui::ViewAssociate {
On 2016/05/10 19:15:14, jeffbrown wrote:
> rename MockViewAssociate

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:27: int
num_times_connect_called_ = 0;
On 2016/05/10 19:15:15, jeffbrown wrote:
> order of members is methods, then fields, so this should be at the end
> 
> also _ suffix is only used for private fields, so you can remove it here

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:84:
mojo::StrongBinding<mojo::ui::ViewListener> view_listener_binding_;
On 2016/05/10 19:15:15, jeffbrown wrote:
> use Binding here, not StrongBinding
> 
> I also recommend moving the listener to another class, perhaps MockView (like
> BaseView) or MockViewListener.

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:85: mojo::ui::ViewOwnerPtr
client_view_owner_;
On 2016/05/10 19:15:15, jeffbrown wrote:
> I don't think this is used.

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:114:
GetProxy(&client_view_owner), view_listener.Pass(),
On 2016/05/10 19:15:14, jeffbrown wrote:
> mojo::GetProxy (not sure why this compiles since mojo isn't in scope here)

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:121:
mojo::StrongBinding<mojo::ui::ViewAssociate> view_associate_binding(
On 2016/05/10 19:15:15, jeffbrown wrote:
> This almost but not quite does what you want.  Unfortunately StrongBinding
> doesn't take ownership of the implementation object, it only destroys it on
> error.  It's basically only useful in one pattern.
> 
> Fortunately we can do something simpler here.  If you allocate
MockViewAssociate
> on the stack then follow it with an Binding, then the objects get destroyed in
> the reverse order which is what you want, so nothing leaks.
> 
> MockViewAssociate associate_impl;
> mojo::ui::ViewAssociatePtr associate;
> mojo::Binding<mojo::ui::ViewAssociate> associate_binding(associate,
> mojo::GetProxy(associate));

Done.

https://codereview.chromium.org/1949233002/diff/40001/services/ui/view_manage...
services/ui/view_manager/tests/view_manager_test.cc:121:
mojo::StrongBinding<mojo::ui::ViewAssociate> view_associate_binding(
On 2016/05/10 19:54:21, jeffbrown wrote:
> On 2016/05/10 19:15:15, jeffbrown wrote:
> > This almost but not quite does what you want.  Unfortunately StrongBinding
> > doesn't take ownership of the implementation object, it only destroys it on
> > error.  It's basically only useful in one pattern.
> > 
> > Fortunately we can do something simpler here.  If you allocate
> MockViewAssociate
> > on the stack then follow it with an Binding, then the objects get destroyed
in
> > the reverse order which is what you want, so nothing leaks.
> > 
> > MockViewAssociate associate_impl;
> > mojo::ui::ViewAssociatePtr associate;
> > mojo::Binding<mojo::ui::ViewAssociate> associate_binding(associate,
> > mojo::GetProxy(associate));
> 
> Whoops, I meant:
> 
> mojo::Binding<mojo::ui::ViewAssociate> associate_binding(&associate_impl,
> mojo::GetProxy(associate));

Acknowledged.

[mikejurka, 2016-05-11 22:29:12]

So far i just have a change that adds a RegisterViewAssociate method.But learned
a lot about mojo and wrote tests for it too. Questions i had:

File: services/ui/view_manager/view_manager_impl.cc

1. RegisterViewAssociate should only really be called by trusted code (ie
launcher) -- how to ensure that?

2. ConnectAssociates passes an connection error callback when setting up a
ViewAssociate. The callback is from ViewManager and it kills the whole app.

So, for now, I pass that error callback into RegisterViewAssociate as well.
However, in cases where RegisterViewAssociate is called directly, we use a
callback that's from ViewAssociateTable, and it removes the view associate from
the table.

Does this sound ok for now?


Also, the tests are sorta flaky. It seems like they always fail if they're run
right after a build, strangely (the KickMessageLoop stuff times out). But then
every time I run them afterward they pass. Any idea why that'd be?

[jeffbrown, 2016-05-11 23:44:20]

https://codereview.chromium.org/1949233002/diff/240001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1949233002/diff/240001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_manager.mojom:73: // The services
provided by |view_associate| will be made available to views
nit: end each sentence with a period

We'll need to do some more work here for security later but we don't know what
form that will take.  So for now, it's ok to leave a TODO.

eg. TODO(mikejurka): This method should only be made accessible to trusted
services, once we have a security story.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/BUILD.gn (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/BUILD.gn:80: "//mojo/ui/associates:mocks",
I don't think this target exits, or isn't in this patch?

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/BUILD.gn:81: "//services/ui/view_manager",
Do you need this dependency given that you have :common above?

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/tests/view_associate_table_test.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_associate_table_test.cc:22: // we have
special permission to access ViewAssociateTable::associates_ since
Slightly cleaner way to do this is to add an "associate_count()" method to
ViewAssociateTable.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/tests/view_manager_test.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_manager_test.cc:88: EXPECT_EQ(1,
mock_view_associate.connect_invokecount);
Would be good to test that we can actually retrieve services from the associate
(via a View) but this is a good start.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/tests/view_manager_test_base.h (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_manager_test_base.h:11: const
base::TimeDelta kDefaultMessageDelay =
You can move this into the .cc file.  Also, we don't allow static initialization
of complex types, so you'll have to declare a simple integer constant for the
number of milliseconds instead.  (Could be fixed if base::TimeDelta's methods
were constexpr.)

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_manager_test_base.h:14: // Run message loop
until condition is true (timeout after 50*4ms = 200ms)
I suspect this is why the test is flaky.  We're actually starting services and a
cold start could be pretty slow.  I'd suggest just looping indefinitely.

Also, consider passing a lambda instead of using a macro.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_manager_test_base.h:27: void
QuitMessageLoopCallback();
This doesn't need to be public, can be private.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.cc (left):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:101: << ", associate_url=" <<
data->url;
Yeah, it would be really useful to be able to print a label here.  I guess
everywhere you saw url before you could replace with label.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:53:
associates_.emplace_back(new AssociateData(associate.Pass(), inspector));
for debugging, it might be useful if each associate had a label (like the View
and ViewTree labels)

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:56: if
(connection_error_callback.is_null()) {
I think this check is going to go away when we move the code that connects to
associates out of ViewManager and into Launcher.  Do you want to do that in this
patch or as a follow-up?

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:63: // connect the associate to
our view inspector
style nit: Treat comments as complete sentences.

https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:71: // wait for the associate
to connect to our view inspector
Technically we're waiting for it to give us back its AssociateInfo in response
to the call to Connect.  It might not do anything with the ViewInspector (could
even throw it away if it doesn't need it).

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.h (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.h:26: friend class
test::ViewAssociateTableTest;
should be in the private section, but I think you can remove it just by adding a
little method to get the size

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_manager_impl.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_manager_impl.cc:10: 
remove blank line

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_manager_impl.cc:41: // this should only really be
called by trusted code (ie launcher) -- how to
// TODO(mikejurka): This should only be ...

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_registry.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_registry.cc:74: mojo::ui::ViewAssociatePtr
view_associate) {
// TODO(mikejurka): Delete callback once registration fully moved to Launcher.

[mikejurka, 2016-05-16 23:35:19]

https://codereview.chromium.org/1949233002/diff/240001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1949233002/diff/240001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_manager.mojom:73: // The services
provided by |view_associate| will be made available to views
On 2016/05/11 23:44:16, jeffbrown wrote:
> nit: end each sentence with a period
> 
> We'll need to do some more work here for security later but we don't know what
> form that will take.  So for now, it's ok to leave a TODO.
> 
> eg. TODO(mikejurka): This method should only be made accessible to trusted
> services, once we have a security story.

Done.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/BUILD.gn (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/BUILD.gn:80: "//mojo/ui/associates:mocks",
On 2016/05/11 23:44:16, jeffbrown wrote:
> I don't think this target exits, or isn't in this patch?
i'm re-using a pre-existing mock, it was in the tree before my change

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/BUILD.gn:81: "//services/ui/view_manager",
On 2016/05/11 23:44:16, jeffbrown wrote:
> Do you need this dependency given that you have :common above?

Done.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/tests/view_manager_test_base.h (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_manager_test_base.h:14: // Run message loop
until condition is true (timeout after 50*4ms = 200ms)
On 2016/05/11 23:44:16, jeffbrown wrote:
> I suspect this is why the test is flaky.  We're actually starting services and
a
> cold start could be pretty slow.  I'd suggest just looping indefinitely.
> 
> Also, consider passing a lambda instead of using a macro.

You mean a lambda for the "while" condition? I feel like that'd be quite
verbose, and this is just for tests.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/tests/view_manager_test_base.h:27: void
QuitMessageLoopCallback();
On 2016/05/11 23:44:16, jeffbrown wrote:
> This doesn't need to be public, can be private.

Done.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:53:
associates_.emplace_back(new AssociateData(associate.Pass(), inspector));
On 2016/05/11 23:44:19, jeffbrown wrote:
> for debugging, it might be useful if each associate had a label (like the View
> and ViewTree labels)

how would we get/generate the label? should it be passed in with
RegisterViewAssociate? I was passing in a "url" before, that could be a label
instead.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:56: if
(connection_error_callback.is_null()) {
On 2016/05/11 23:44:20, jeffbrown wrote:
> I think this check is going to go away when we move the code that connects to
> associates out of ViewManager and into Launcher.  Do you want to do that in
this
> patch or as a follow-up?

follow-up since the existing code should still work for now

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:63: // connect the associate to
our view inspector
On 2016/05/11 23:44:19, jeffbrown wrote:
> style nit: Treat comments as complete sentences.
> 
>
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...

Done.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.h (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.h:26: friend class
test::ViewAssociateTableTest;
On 2016/05/11 23:44:20, jeffbrown wrote:
> should be in the private section, but I think you can remove it just by adding
a
> little method to get the size

why should it be in the private section?

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_manager_impl.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_manager_impl.cc:10: 
On 2016/05/11 23:44:20, jeffbrown wrote:
> remove blank line

Done.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_manager_impl.cc:41: // this should only really be
called by trusted code (ie launcher) -- how to
On 2016/05/11 23:44:20, jeffbrown wrote:
> // TODO(mikejurka): This should only be ...

Done.

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
File services/ui/view_manager/view_registry.cc (right):

https://codereview.chromium.org/1949233002/diff/240001/services/ui/view_manag...
services/ui/view_manager/view_registry.cc:74: mojo::ui::ViewAssociatePtr
view_associate) {
On 2016/05/11 23:44:20, jeffbrown wrote:
> // TODO(mikejurka): Delete callback once registration fully moved to Launcher.

Done.

[jeffbrown, 2016-05-17 00:44:49]

https://codereview.chromium.org/1949233002/diff/280001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1949233002/diff/280001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_manager.mojom:87:
OnViewAssociateConnectionError(string? label) => ();
Returning the label is useful for diagnostics but whoever registered the
associate won't necessarily be able to use it to identify which one died (and
maybe restart it).

Perhaps you could add a ViewAssociateWatcher to the ViewAssociateRegistration
instead so you'd have one per associate so there's no ambiguity.  Or if we
following something like the pattern of ViewOwner / ViewTreeOwner we could do:

RegisterViewAssociate(mojo.ui.ViewAssociate view_associate,
mojo.ui.ViewAssociateOwner& view_associate_owner, string? label);

If the associate dies, the view manager can close its endpoint of the owner
connection.  If the launcher wants to remove the associate, it can close its
endpoint of the owner connection.  So it's slightly more functional.  (Don't
worry about having lots of extra message pipes, they're supposed to be cheap.)

This also works with the array variant too, though thinking on it we might be
happier keeping the simple case and adding a synchronizing method instead.

interface ViewManager {
  /* existing stuff... */

  RegisterViewAssociate(mojo.ui.ViewAssociate view_associate,
mojo.ui.ViewAssociateOwner& view_associate_owner, string? label);

  // TODO: delete when we add methods for clients to detect when associates come
and go
  FiatLux();
}

https://codereview.chromium.org/1949233002/diff/280001/services/ui/launcher/l...
File services/ui/launcher/launch_instance.cc (right):

https://codereview.chromium.org/1949233002/diff/280001/services/ui/launcher/l...
services/ui/launcher/launch_instance.cc:75: // TODO(mikejurka): should we even
be running this callback?
yes, all callbacks have to be invoked (unless you close the connection first)

https://codereview.chromium.org/1949233002/diff/280001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1949233002/diff/280001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:59:
associate_watchers_.push_back(view_associate_watcher.Pass());
prefer emplace_back where possible

https://codereview.chromium.org/1949233002/diff/280001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.h (right):

https://codereview.chromium.org/1949233002/diff/280001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.h:33: const
AssociateConnectionErrorCallback& connection_error_callback,
I think you can remove this callback now given you already have the associate
watcher thingy.

[jeffbrown, 2016-05-17 00:50:01]

FWIW, an empty interface will do for now.  :)

interface ViewAssociateOwner {};

[mikejurka, 2016-05-18 01:14:28]

https://codereview.chromium.org/1949233002/diff/280001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.h (right):

https://codereview.chromium.org/1949233002/diff/280001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.h:33: const
AssociateConnectionErrorCallback& connection_error_callback,
On 2016/05/17 00:44:49, jeffbrown wrote:
> I think you can remove this callback now given you already have the associate
> watcher thingy.

Done.

[jeffbrown, 2016-05-18 18:07:16]

Looking good!  Nearly there.

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_associates.mojom (right):

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_associates.mojom:63:
[ServiceName="mojo::ui::ViewAssociateOwner"]
Please add a comment explaining what this type represents.

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_manager.mojom:86: // Tells the view
manager we've registered all our view associates
nit: end sentences with a period, here and below

Also please add a TODO indicating we should remove this once dynamic
registration and unregistration is fully supported.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/launcher/l...
File services/ui/launcher/launcher_app.cc (right):

https://codereview.chromium.org/1949233002/diff/360001/services/ui/launcher/l...
services/ui/launcher/launcher_app.cc:36:
command_line->GetSwitchValueASCII("view_associate_urls"));
It might be better to split the string here so that the Launch method takes
std::vector<std::string>.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:63:
waiting_to_register_associates_ = false;
This needs to kick deferred work in the same way that dropping the pending
connection count to zero does.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:107: }
nit: if we fail to find the associate then there's something wrong (we're
manipulating a dangling pointer)

You might try something like this:

DVLOG(2) << "ViewAssociate disconnected, removing from table, associate_label="
<< data->label;
RemoveAssociate(associate_data);

...

void ViewAssociateTable::RemoveAssociate(AssociateData* associate_data) {
   for (...) { if found ,remove, return }
   NOT_REACHED();
}

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:165: if
(!waiting_to_register_associates_ && !pending_connection_count_)
Consider renaming CompleteDeferredWork to CompleteDeferredWorkIfNeeded and
checking this condition there.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
File services/ui/view_manager/view_registry.cc (right):

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_registry.cc:69:
associate_table_.RegisterViewAssociate(view_inspector, view_associate.Pass(),
Please sanitize the label here before using it.  See CreateView for example.

[jeffbrown, 2016-05-18 18:07:53]

On your next patch, you should update the submit and description of this CL. 
Unfortunately "git cl upload" doesn't seem to do that for you.

[mikejurka, 2016-05-18 19:08:25]

Description was changed from

==========
WIP, lots of questions for how to do things here

mozart: invert dependency between view associates and view manager #703

BUG=
==========

to

==========
Create a RegisterViewAssociate method in ViewManager
Add tests for ViewManager and Views

#703
==========

[mikejurka, 2016-05-18 19:09:11]

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_associates.mojom (right):

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_associates.mojom:63:
[ServiceName="mojo::ui::ViewAssociateOwner"]
On 2016/05/18 18:07:16, jeffbrown wrote:
> Please add a comment explaining what this type represents.

Done.

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
File mojo/services/ui/views/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1949233002/diff/360001/mojo/services/ui/views...
mojo/services/ui/views/interfaces/view_manager.mojom:86: // Tells the view
manager we've registered all our view associates
On 2016/05/18 18:07:16, jeffbrown wrote:
> nit: end sentences with a period, here and below
> 
> Also please add a TODO indicating we should remove this once dynamic
> registration and unregistration is fully supported.

Done.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/launcher/l...
File services/ui/launcher/launcher_app.cc (right):

https://codereview.chromium.org/1949233002/diff/360001/services/ui/launcher/l...
services/ui/launcher/launcher_app.cc:36:
command_line->GetSwitchValueASCII("view_associate_urls"));
On 2016/05/18 18:07:16, jeffbrown wrote:
> It might be better to split the string here so that the Launch method takes
> std::vector<std::string>.

Doing that would require a conversion of a std::vector (which SplitString
returns) to a mojo::Array, and then potentially back again.

Instead I did it in the constructor of LaunchInstance, but kept the parameter as
a string.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
File services/ui/view_manager/view_associate_table.cc (right):

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:63:
waiting_to_register_associates_ = false;
On 2016/05/18 18:07:16, jeffbrown wrote:
> This needs to kick deferred work in the same way that dropping the pending
> connection count to zero does.

Done.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:107: }
On 2016/05/18 18:07:16, jeffbrown wrote:
> nit: if we fail to find the associate then there's something wrong (we're
> manipulating a dangling pointer)
> 
> You might try something like this:
> 
> DVLOG(2) << "ViewAssociate disconnected, removing from table,
associate_label="
> << data->label;
> RemoveAssociate(associate_data);
> 
> ...
> 
> void ViewAssociateTable::RemoveAssociate(AssociateData* associate_data) {
>    for (...) { if found ,remove, return }
>    NOT_REACHED();
> }

Done.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_associate_table.cc:165: if
(!waiting_to_register_associates_ && !pending_connection_count_)
On 2016/05/18 18:07:16, jeffbrown wrote:
> Consider renaming CompleteDeferredWork to CompleteDeferredWorkIfNeeded and
> checking this condition there.

Done.

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
File services/ui/view_manager/view_registry.cc (right):

https://codereview.chromium.org/1949233002/diff/360001/services/ui/view_manag...
services/ui/view_manager/view_registry.cc:69:
associate_table_.RegisterViewAssociate(view_inspector, view_associate.Pass(),
On 2016/05/18 18:07:16, jeffbrown wrote:
> Please sanitize the label here before using it.  See CreateView for example.

Done.

[jeffbrown, 2016-05-18 21:29:30]

lgtm

However there seems to be something wrong with the Android build.  You should
try building this on Android before submitting the change.

Thanks!

[mikejurka, 2016-05-18 22:15:55]

Description was changed from

==========
Create a RegisterViewAssociate method in ViewManager
Add tests for ViewManager and Views

#703
==========

to

==========
Create a RegisterViewAssociate method in ViewManager
Add tests for ViewManager and Views

#703

R=jeffbrown@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/dec9013739bd492920960729121...
==========

[mikejurka, 2016-05-18 22:15:57]

Committed patchset #22 (id:420001) manually as
dec9013739bd49292096072912197341fa056795 (presubmit successful).