Internalize ServiceConnector<>

Internalize ServiceConnector<>

BUG=

[DaveMoore, 2014-05-10 15:02:50]

Update

[DaveMoore, 2014-05-12 20:42:57]

Update

[DaveMoore, 2014-05-12 21:29:53]

Merge

[darin (slow to review), 2014-05-12 22:04:52]

This looks great. No major concerns on my part. LGTM w/ nits.

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
File mojo/examples/launcher/launcher.cc (right):

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
mojo/examples/launcher/launcher.cc:191: class LauncherImpl : public
InterfaceImpl<Launcher>,
looks nice! :)

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application* app)
nit: explicit ctor

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
"mojo:mojo_native_viewport_service", &viewport);
You could write app_->ConnectTo("mojo:...", &viewport) here.

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
mojo/examples/launcher/launcher.cc:297:
app.AddService<mojo::examples::LauncherImpl>(&app);
oh, that's an interesting way of using the Context parameter.

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/ap...
File mojo/public/cpp/shell/application.h (right):

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/ap...
mojo/public/cpp/shell/application.h:23: // constructor with that class as its
only argument, otherwise defined an empty
nit: otherwise defined -> otherwise define

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/ap...
mojo/public/cpp/shell/application.h:38: // };
You might want to indicate that the BarContext pointer will remain valid for the
lifetime of the BarImpl.

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
File mojo/public/cpp/shell/lib/service_connector.h (right):

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
mojo/public/cpp/shell/lib/service_connector.h:32:
ServiceImpl::OnConnectionError();
Hmm... this requires that ServiceImpl implement OnConnectionError else there
will be a compile error. Maybe that is OK. I forced subclasses of
InterfaceImpl<..> to implement this method since it was important for memory
management. Here it is not the case since ServiceConnection<..> is managing
memory. Hmm...

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
mojo/public/cpp/shell/lib/service_connector.h:48: struct Constructor {
You might consider a more verbose name like ServiceConnectionConstructor :-/

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
mojo/public/cpp/shell/lib/service_connector.h:109:
internal::Constructor<ServiceImpl, Context>::New(context_);
nit: no need for the internal:: prefix since you are in the internal namespace
already.

https://codereview.chromium.org/275363002/diff/80001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
(right):

https://codereview.chromium.org/275363002/diff/80001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:9:
#include "mojo/public/cpp/bindings/allocation_scope.h"
nit: allocation scope does not appear to be needed by this .cc file

[DaveMoore, 2014-05-12 23:38:02]

Address comments

[DaveMoore, 2014-05-12 23:39:13]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-05-12 23:40:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davemoore@chromium.org/275363002/100001

[DaveMoore, 2014-05-13 00:02:36]

Merge fallout

[DaveMoore, 2014-05-13 00:03:18]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-05-13 00:03:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davemoore@chromium.org/275363002/120001

[commit-bot: I haz the power, 2014-05-13 04:35:31]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-13 04:41:16]

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

[commit-bot: I haz the power, 2014-05-13 04:41:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[DaveMoore, 2014-05-13 17:43:26]

Define two AddService() methods because default template args aren't supported
everywhere

[DaveMoore, 2014-05-13 17:43:52]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-05-13 17:44:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davemoore@chromium.org/275363002/140001

[commit-bot: I haz the power, 2014-05-13 20:43:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-13 20:47:21]

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

[commit-bot: I haz the power, 2014-05-13 20:47:21]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[DaveMoore, 2014-05-14 16:47:32]

Move ViewManagerConnection initialization to SetClient()

[DaveMoore, 2014-05-14 16:50:31]

I had to make some changes to ViewManagerConnection to fix test failures. They
were because this change removed the Initialize() method. No compiler errors
were found because the invocation was template based so it wasn't flagged as
OVERRIDE.

In place of Initialize() I now do that setup in SetClient() which can be
optionally overridden.

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
File mojo/examples/launcher/launcher.cc (right):

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application* app)
On 2014/05/12 22:04:52, darin wrote:
> nit: explicit ctor

Done.

https://codereview.chromium.org/275363002/diff/80001/mojo/examples/launcher/l...
mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
"mojo:mojo_native_viewport_service", &viewport);
On 2014/05/12 22:04:52, darin wrote:
> You could write app_->ConnectTo("mojo:...", &viewport) here.

Done.

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/ap...
File mojo/public/cpp/shell/application.h (right):

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/ap...
mojo/public/cpp/shell/application.h:23: // constructor with that class as its
only argument, otherwise defined an empty
On 2014/05/12 22:04:52, darin wrote:
> nit: otherwise defined -> otherwise define

Done.

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/ap...
mojo/public/cpp/shell/application.h:38: // };
On 2014/05/12 22:04:52, darin wrote:
> You might want to indicate that the BarContext pointer will remain valid for
the
> lifetime of the BarImpl.

Done.

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
File mojo/public/cpp/shell/lib/service_connector.h (right):

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
mojo/public/cpp/shell/lib/service_connector.h:48: struct Constructor {
On 2014/05/12 22:04:52, darin wrote:
> You might consider a more verbose name like ServiceConnectionConstructor :-/

Done.

https://codereview.chromium.org/275363002/diff/80001/mojo/public/cpp/shell/li...
mojo/public/cpp/shell/lib/service_connector.h:109:
internal::Constructor<ServiceImpl, Context>::New(context_);
On 2014/05/12 22:04:52, darin wrote:
> nit: no need for the internal:: prefix since you are in the internal namespace
> already.

Done.

https://codereview.chromium.org/275363002/diff/80001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
(right):

https://codereview.chromium.org/275363002/diff/80001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:9:
#include "mojo/public/cpp/bindings/allocation_scope.h"
On 2014/05/12 22:04:52, darin wrote:
> nit: allocation scope does not appear to be needed by this .cc file

Done.

[DaveMoore, 2014-05-14 16:53:33]

+sky

[darin (slow to review), 2014-05-14 17:04:30]

Hmm... not sure about doing initialization in SetClient. It can't happen in
the ctor because it depends on the client?
On May 14, 2014 9:50 AM, <davemoore@chromium.org> wrote:

> I had to make some changes to ViewManagerConnection to fix test failures.
> They
> were because this change removed the Initialize() method. No compiler
> errors
> were found because the invocation was template based so it wasn't flagged
> as
> OVERRIDE.
>
> In place of Initialize() I now do that setup in SetClient() which can be
> optionally overridden.
>
>
>
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> examples/launcher/launcher.cc
> File mojo/examples/launcher/launcher.cc (right):
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> examples/launcher/launcher.cc#newcode194
> mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application* app)
> On 2014/05/12 22:04:52, darin wrote:
>
>> nit: explicit ctor
>>
>
> Done.
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> examples/launcher/launcher.cc#newcode206
> mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
> "mojo:mojo_native_viewport_service", &viewport);
> On 2014/05/12 22:04:52, darin wrote:
>
>> You could write app_->ConnectTo("mojo:...", &viewport) here.
>>
>
> Done.
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> public/cpp/shell/application.h
> File mojo/public/cpp/shell/application.h (right):
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> public/cpp/shell/application.h#newcode23
> mojo/public/cpp/shell/application.h:23: // constructor with that class
> as its only argument, otherwise defined an empty
> On 2014/05/12 22:04:52, darin wrote:
>
>> nit: otherwise defined -> otherwise define
>>
>
> Done.
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> public/cpp/shell/application.h#newcode38
> mojo/public/cpp/shell/application.h:38: // };
> On 2014/05/12 22:04:52, darin wrote:
>
>> You might want to indicate that the BarContext pointer will remain
>>
> valid for the
>
>> lifetime of the BarImpl.
>>
>
> Done.
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> public/cpp/shell/lib/service_connector.h
> File mojo/public/cpp/shell/lib/service_connector.h (right):
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> public/cpp/shell/lib/service_connector.h#newcode48
> mojo/public/cpp/shell/lib/service_connector.h:48: struct Constructor {
> On 2014/05/12 22:04:52, darin wrote:
>
>> You might consider a more verbose name like
>>
> ServiceConnectionConstructor :-/
>
> Done.
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> public/cpp/shell/lib/service_connector.h#newcode109
> mojo/public/cpp/shell/lib/service_connector.h:109:
> internal::Constructor<ServiceImpl, Context>::New(context_);
> On 2014/05/12 22:04:52, darin wrote:
>
>> nit: no need for the internal:: prefix since you are in the internal
>>
> namespace
>
>> already.
>>
>
> Done.
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> File
> mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> (right):
>
> https://codereview.chromium.org/275363002/diff/80001/mojo/
> services/public/cpp/view_manager/lib/view_manager_synchronizer.cc#newcode9
> mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:9:
> #include "mojo/public/cpp/bindings/allocation_scope.h"
> On 2014/05/12 22:04:52, darin wrote:
>
>> nit: allocation scope does not appear to be needed by this .cc file
>>
>
> Done.
>
> https://codereview.chromium.org/275363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-05-14 17:10:39]

Yes, it uses the client. I'd hate to impose Initialize() on every instance. We
could add it to InterfaceImpl<>, then it could be overridden like we plan to do
with SetClient(). That ties InterfaceImpl<> to Application a bit more than we
wanted.

On 2014/05/14 17:04:30, darin wrote:
> Hmm... not sure about doing initialization in SetClient. It can't happen in
> the ctor because it depends on the client?
> On May 14, 2014 9:50 AM, <mailto:davemoore@chromium.org> wrote:
> 
> > I had to make some changes to ViewManagerConnection to fix test failures.
> > They
> > were because this change removed the Initialize() method. No compiler
> > errors
> > were found because the invocation was template based so it wasn't flagged
> > as
> > OVERRIDE.
> >
> > In place of Initialize() I now do that setup in SetClient() which can be
> > optionally overridden.
> >
> >
> >
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > examples/launcher/launcher.cc
> > File mojo/examples/launcher/launcher.cc (right):
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > examples/launcher/launcher.cc#newcode194
> > mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application* app)
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> nit: explicit ctor
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > examples/launcher/launcher.cc#newcode206
> > mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
> > "mojo:mojo_native_viewport_service", &viewport);
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> You could write app_->ConnectTo("mojo:...", &viewport) here.
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > public/cpp/shell/application.h
> > File mojo/public/cpp/shell/application.h (right):
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > public/cpp/shell/application.h#newcode23
> > mojo/public/cpp/shell/application.h:23: // constructor with that class
> > as its only argument, otherwise defined an empty
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> nit: otherwise defined -> otherwise define
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > public/cpp/shell/application.h#newcode38
> > mojo/public/cpp/shell/application.h:38: // };
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> You might want to indicate that the BarContext pointer will remain
> >>
> > valid for the
> >
> >> lifetime of the BarImpl.
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > public/cpp/shell/lib/service_connector.h
> > File mojo/public/cpp/shell/lib/service_connector.h (right):
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > public/cpp/shell/lib/service_connector.h#newcode48
> > mojo/public/cpp/shell/lib/service_connector.h:48: struct Constructor {
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> You might consider a more verbose name like
> >>
> > ServiceConnectionConstructor :-/
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > public/cpp/shell/lib/service_connector.h#newcode109
> > mojo/public/cpp/shell/lib/service_connector.h:109:
> > internal::Constructor<ServiceImpl, Context>::New(context_);
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> nit: no need for the internal:: prefix since you are in the internal
> >>
> > namespace
> >
> >> already.
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> > File
> > mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> > (right):
> >
> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> > services/public/cpp/view_manager/lib/view_manager_synchronizer.cc#newcode9
> > mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc:9:
> > #include "mojo/public/cpp/bindings/allocation_scope.h"
> > On 2014/05/12 22:04:52, darin wrote:
> >
> >> nit: allocation scope does not appear to be needed by this .cc file
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/275363002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-14 17:33:46]

Initialize could be called from SetClient or even BindTo{Pipe,Proxy}.
Overriding SetClient means you have to call the base class SetClient. The
downside is that Initialize would then be yet another virtual function and
steal from the "method namespace" of the developer. Hmm...
On May 14, 2014 10:10 AM, <davemoore@chromium.org> wrote:

> Yes, it uses the client. I'd hate to impose Initialize() on every
> instance. We
> could add it to InterfaceImpl<>, then it could be overridden like we plan
> to do
> with SetClient(). That ties InterfaceImpl<> to Application a bit more than
> we
> wanted.
>
> On 2014/05/14 17:04:30, darin wrote:
>
>> Hmm... not sure about doing initialization in SetClient. It can't happen
>> in
>> the ctor because it depends on the client?
>> On May 14, 2014 9:50 AM, <mailto:davemoore@chromium.org> wrote:
>>
>
>  > I had to make some changes to ViewManagerConnection to fix test
>> failures.
>> > They
>> > were because this change removed the Initialize() method. No compiler
>> > errors
>> > were found because the invocation was template based so it wasn't
>> flagged
>> > as
>> > OVERRIDE.
>> >
>> > In place of Initialize() I now do that setup in SetClient() which can be
>> > optionally overridden.
>> >
>> >
>> >
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > examples/launcher/launcher.cc
>> > File mojo/examples/launcher/launcher.cc (right):
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > examples/launcher/launcher.cc#newcode194
>> > mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application* app)
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> nit: explicit ctor
>> >>
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > examples/launcher/launcher.cc#newcode206
>> > mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
>> > "mojo:mojo_native_viewport_service", &viewport);
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> You could write app_->ConnectTo("mojo:...", &viewport) here.
>> >>
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > public/cpp/shell/application.h
>> > File mojo/public/cpp/shell/application.h (right):
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > public/cpp/shell/application.h#newcode23
>> > mojo/public/cpp/shell/application.h:23: // constructor with that class
>> > as its only argument, otherwise defined an empty
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> nit: otherwise defined -> otherwise define
>> >>
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > public/cpp/shell/application.h#newcode38
>> > mojo/public/cpp/shell/application.h:38: // };
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> You might want to indicate that the BarContext pointer will remain
>> >>
>> > valid for the
>> >
>> >> lifetime of the BarImpl.
>> >>
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > public/cpp/shell/lib/service_connector.h
>> > File mojo/public/cpp/shell/lib/service_connector.h (right):
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > public/cpp/shell/lib/service_connector.h#newcode48
>> > mojo/public/cpp/shell/lib/service_connector.h:48: struct Constructor {
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> You might consider a more verbose name like
>> >>
>> > ServiceConnectionConstructor :-/
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > public/cpp/shell/lib/service_connector.h#newcode109
>> > mojo/public/cpp/shell/lib/service_connector.h:109:
>> > internal::Constructor<ServiceImpl, Context>::New(context_);
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> nit: no need for the internal:: prefix since you are in the internal
>> >>
>> > namespace
>> >
>> >> already.
>> >>
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
>> > File
>> > mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
>> > (right):
>> >
>> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> > services/public/cpp/view_manager/lib/view_manager_
>> synchronizer.cc#newcode9
>> > mojo/services/public/cpp/view_manager/lib/view_manager_
>> synchronizer.cc:9:
>> > #include "mojo/public/cpp/bindings/allocation_scope.h"
>> > On 2014/05/12 22:04:52, darin wrote:
>> >
>> >> nit: allocation scope does not appear to be needed by this .cc file
>> >>
>> >
>> > Done.
>> >
>> > https://codereview.chromium.org/275363002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/275363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-05-14 18:08:35]

I'm not worried about where to call it from (several options). It's more about
stealing a really common name. I think at a minimum we would need to make it
less of a collision.

On 2014/05/14 17:33:46, darin wrote:
> Initialize could be called from SetClient or even BindTo{Pipe,Proxy}.
> Overriding SetClient means you have to call the base class SetClient. The
> downside is that Initialize would then be yet another virtual function and
> steal from the "method namespace" of the developer. Hmm...
> On May 14, 2014 10:10 AM, <mailto:davemoore@chromium.org> wrote:
> 
> > Yes, it uses the client. I'd hate to impose Initialize() on every
> > instance. We
> > could add it to InterfaceImpl<>, then it could be overridden like we plan
> > to do
> > with SetClient(). That ties InterfaceImpl<> to Application a bit more than
> > we
> > wanted.
> >
> > On 2014/05/14 17:04:30, darin wrote:
> >
> >> Hmm... not sure about doing initialization in SetClient. It can't happen
> >> in
> >> the ctor because it depends on the client?
> >> On May 14, 2014 9:50 AM, <mailto:davemoore@chromium.org> wrote:
> >>
> >
> >  > I had to make some changes to ViewManagerConnection to fix test
> >> failures.
> >> > They
> >> > were because this change removed the Initialize() method. No compiler
> >> > errors
> >> > were found because the invocation was template based so it wasn't
> >> flagged
> >> > as
> >> > OVERRIDE.
> >> >
> >> > In place of Initialize() I now do that setup in SetClient() which can be
> >> > optionally overridden.
> >> >
> >> >
> >> >
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > examples/launcher/launcher.cc
> >> > File mojo/examples/launcher/launcher.cc (right):
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > examples/launcher/launcher.cc#newcode194
> >> > mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application* app)
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> nit: explicit ctor
> >> >>
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > examples/launcher/launcher.cc#newcode206
> >> > mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
> >> > "mojo:mojo_native_viewport_service", &viewport);
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> You could write app_->ConnectTo("mojo:...", &viewport) here.
> >> >>
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > public/cpp/shell/application.h
> >> > File mojo/public/cpp/shell/application.h (right):
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > public/cpp/shell/application.h#newcode23
> >> > mojo/public/cpp/shell/application.h:23: // constructor with that class
> >> > as its only argument, otherwise defined an empty
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> nit: otherwise defined -> otherwise define
> >> >>
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > public/cpp/shell/application.h#newcode38
> >> > mojo/public/cpp/shell/application.h:38: // };
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> You might want to indicate that the BarContext pointer will remain
> >> >>
> >> > valid for the
> >> >
> >> >> lifetime of the BarImpl.
> >> >>
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > public/cpp/shell/lib/service_connector.h
> >> > File mojo/public/cpp/shell/lib/service_connector.h (right):
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > public/cpp/shell/lib/service_connector.h#newcode48
> >> > mojo/public/cpp/shell/lib/service_connector.h:48: struct Constructor {
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> You might consider a more verbose name like
> >> >>
> >> > ServiceConnectionConstructor :-/
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > public/cpp/shell/lib/service_connector.h#newcode109
> >> > mojo/public/cpp/shell/lib/service_connector.h:109:
> >> > internal::Constructor<ServiceImpl, Context>::New(context_);
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> nit: no need for the internal:: prefix since you are in the internal
> >> >>
> >> > namespace
> >> >
> >> >> already.
> >> >>
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> >> > File
> >> > mojo/services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> >> > (right):
> >> >
> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> > services/public/cpp/view_manager/lib/view_manager_
> >> synchronizer.cc#newcode9
> >> > mojo/services/public/cpp/view_manager/lib/view_manager_
> >> synchronizer.cc:9:
> >> > #include "mojo/public/cpp/bindings/allocation_scope.h"
> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >
> >> >> nit: allocation scope does not appear to be needed by this .cc file
> >> >>
> >> >
> >> > Done.
> >> >
> >> > https://codereview.chromium.org/275363002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://codereview.chromium.org/275363002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-14 18:17:30]

Yeah, I don't like stealing such a common method name either.

I was thinking that another option would be to put a method on Context to
report the creation of a ServiceImpl, which would be passed a pointer to
the object.

Other thoughts:

1- In the case that an InterfaceImpl is instantiated outside
ServiceConnector, the developer has the ability to call an "initialize"
method  on the impl since they are responsible for creating the impl.

2- Perhaps it would be worth re-evaluating the code that needs to use
client immediately after construction. Maybe this indicates that
Shell::Connect should really take a callback. Or alternatively, the
ViewManager interface could define an Initialize method with a callback!

-Darin
On May 14, 2014 11:08 AM, <davemoore@chromium.org> wrote:

> I'm not worried about where to call it from (several options). It's more
> about
> stealing a really common name. I think at a minimum we would need to make
> it
> less of a collision.
>
> On 2014/05/14 17:33:46, darin wrote:
>
>> Initialize could be called from SetClient or even BindTo{Pipe,Proxy}.
>> Overriding SetClient means you have to call the base class SetClient. The
>> downside is that Initialize would then be yet another virtual function and
>> steal from the "method namespace" of the developer. Hmm...
>> On May 14, 2014 10:10 AM, <mailto:davemoore@chromium.org> wrote:
>>
>
>  > Yes, it uses the client. I'd hate to impose Initialize() on every
>> > instance. We
>> > could add it to InterfaceImpl<>, then it could be overridden like we
>> plan
>> > to do
>> > with SetClient(). That ties InterfaceImpl<> to Application a bit more
>> than
>> > we
>> > wanted.
>> >
>> > On 2014/05/14 17:04:30, darin wrote:
>> >
>> >> Hmm... not sure about doing initialization in SetClient. It can't
>> happen
>> >> in
>> >> the ctor because it depends on the client?
>> >> On May 14, 2014 9:50 AM, <mailto:davemoore@chromium.org> wrote:
>> >>
>> >
>> >  > I had to make some changes to ViewManagerConnection to fix test
>> >> failures.
>> >> > They
>> >> > were because this change removed the Initialize() method. No compiler
>> >> > errors
>> >> > were found because the invocation was template based so it wasn't
>> >> flagged
>> >> > as
>> >> > OVERRIDE.
>> >> >
>> >> > In place of Initialize() I now do that setup in SetClient() which
>> can be
>> >> > optionally overridden.
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > examples/launcher/launcher.cc
>> >> > File mojo/examples/launcher/launcher.cc (right):
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > examples/launcher/launcher.cc#newcode194
>> >> > mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application*
>> app)
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> nit: explicit ctor
>> >> >>
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > examples/launcher/launcher.cc#newcode206
>> >> > mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
>> >> > "mojo:mojo_native_viewport_service", &viewport);
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> You could write app_->ConnectTo("mojo:...", &viewport) here.
>> >> >>
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > public/cpp/shell/application.h
>> >> > File mojo/public/cpp/shell/application.h (right):
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > public/cpp/shell/application.h#newcode23
>> >> > mojo/public/cpp/shell/application.h:23: // constructor with that
>> class
>> >> > as its only argument, otherwise defined an empty
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> nit: otherwise defined -> otherwise define
>> >> >>
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > public/cpp/shell/application.h#newcode38
>> >> > mojo/public/cpp/shell/application.h:38: // };
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> You might want to indicate that the BarContext pointer will remain
>> >> >>
>> >> > valid for the
>> >> >
>> >> >> lifetime of the BarImpl.
>> >> >>
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > public/cpp/shell/lib/service_connector.h
>> >> > File mojo/public/cpp/shell/lib/service_connector.h (right):
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > public/cpp/shell/lib/service_connector.h#newcode48
>> >> > mojo/public/cpp/shell/lib/service_connector.h:48: struct
>> Constructor {
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> You might consider a more verbose name like
>> >> >>
>> >> > ServiceConnectionConstructor :-/
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > public/cpp/shell/lib/service_connector.h#newcode109
>> >> > mojo/public/cpp/shell/lib/service_connector.h:109:
>> >> > internal::Constructor<ServiceImpl, Context>::New(context_);
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> nit: no need for the internal:: prefix since you are in the internal
>> >> >>
>> >> > namespace
>> >> >
>> >> >> already.
>> >> >>
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
>> >> > File
>> >> > mojo/services/public/cpp/view_manager/lib/view_manager_
>> synchronizer.cc
>> >> > (right):
>> >> >
>> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
>> >> > services/public/cpp/view_manager/lib/view_manager_
>> >> synchronizer.cc#newcode9
>> >> > mojo/services/public/cpp/view_manager/lib/view_manager_
>> >> synchronizer.cc:9:
>> >> > #include "mojo/public/cpp/bindings/allocation_scope.h"
>> >> > On 2014/05/12 22:04:52, darin wrote:
>> >> >
>> >> >> nit: allocation scope does not appear to be needed by this .cc file
>> >> >>
>> >> >
>> >> > Done.
>> >> >
>> >> > https://codereview.chromium.org/275363002/
>> >> >
>> >>
>> >
>> >  To unsubscribe from this group and stop receiving emails from it, send
>> an
>> >>
>> > email
>> >
>> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >>
>> >
>> >
>> >
>> > https://codereview.chromium.org/275363002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/275363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-05-14 18:21:43]

There were only two instances of Initialize(). One needed the client and one
didn't. Neither needed to know when the connection was done. So the one that
needed the client overrode that and the other moved the initialization into the
constructor (which now has the context). So what I have now satisfies both cases
reasonably. 

On 2014/05/14 18:17:30, darin wrote:
> Yeah, I don't like stealing such a common method name either.
> 
> I was thinking that another option would be to put a method on Context to
> report the creation of a ServiceImpl, which would be passed a pointer to
> the object.
> 
> Other thoughts:
> 
> 1- In the case that an InterfaceImpl is instantiated outside
> ServiceConnector, the developer has the ability to call an "initialize"
> method  on the impl since they are responsible for creating the impl.
> 
> 2- Perhaps it would be worth re-evaluating the code that needs to use
> client immediately after construction. Maybe this indicates that
> Shell::Connect should really take a callback. Or alternatively, the
> ViewManager interface could define an Initialize method with a callback!
> 
> -Darin
> On May 14, 2014 11:08 AM, <mailto:davemoore@chromium.org> wrote:
> 
> > I'm not worried about where to call it from (several options). It's more
> > about
> > stealing a really common name. I think at a minimum we would need to make
> > it
> > less of a collision.
> >
> > On 2014/05/14 17:33:46, darin wrote:
> >
> >> Initialize could be called from SetClient or even BindTo{Pipe,Proxy}.
> >> Overriding SetClient means you have to call the base class SetClient. The
> >> downside is that Initialize would then be yet another virtual function and
> >> steal from the "method namespace" of the developer. Hmm...
> >> On May 14, 2014 10:10 AM, <mailto:davemoore@chromium.org> wrote:
> >>
> >
> >  > Yes, it uses the client. I'd hate to impose Initialize() on every
> >> > instance. We
> >> > could add it to InterfaceImpl<>, then it could be overridden like we
> >> plan
> >> > to do
> >> > with SetClient(). That ties InterfaceImpl<> to Application a bit more
> >> than
> >> > we
> >> > wanted.
> >> >
> >> > On 2014/05/14 17:04:30, darin wrote:
> >> >
> >> >> Hmm... not sure about doing initialization in SetClient. It can't
> >> happen
> >> >> in
> >> >> the ctor because it depends on the client?
> >> >> On May 14, 2014 9:50 AM, <mailto:davemoore@chromium.org> wrote:
> >> >>
> >> >
> >> >  > I had to make some changes to ViewManagerConnection to fix test
> >> >> failures.
> >> >> > They
> >> >> > were because this change removed the Initialize() method. No compiler
> >> >> > errors
> >> >> > were found because the invocation was template based so it wasn't
> >> >> flagged
> >> >> > as
> >> >> > OVERRIDE.
> >> >> >
> >> >> > In place of Initialize() I now do that setup in SetClient() which
> >> can be
> >> >> > optionally overridden.
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > examples/launcher/launcher.cc
> >> >> > File mojo/examples/launcher/launcher.cc (right):
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > examples/launcher/launcher.cc#newcode194
> >> >> > mojo/examples/launcher/launcher.cc:194: LauncherImpl(Application*
> >> app)
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> nit: explicit ctor
> >> >> >>
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > examples/launcher/launcher.cc#newcode206
> >> >> > mojo/examples/launcher/launcher.cc:206: ConnectTo(app_->shell(),
> >> >> > "mojo:mojo_native_viewport_service", &viewport);
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> You could write app_->ConnectTo("mojo:...", &viewport) here.
> >> >> >>
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > public/cpp/shell/application.h
> >> >> > File mojo/public/cpp/shell/application.h (right):
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > public/cpp/shell/application.h#newcode23
> >> >> > mojo/public/cpp/shell/application.h:23: // constructor with that
> >> class
> >> >> > as its only argument, otherwise defined an empty
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> nit: otherwise defined -> otherwise define
> >> >> >>
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > public/cpp/shell/application.h#newcode38
> >> >> > mojo/public/cpp/shell/application.h:38: // };
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> You might want to indicate that the BarContext pointer will remain
> >> >> >>
> >> >> > valid for the
> >> >> >
> >> >> >> lifetime of the BarImpl.
> >> >> >>
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > public/cpp/shell/lib/service_connector.h
> >> >> > File mojo/public/cpp/shell/lib/service_connector.h (right):
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > public/cpp/shell/lib/service_connector.h#newcode48
> >> >> > mojo/public/cpp/shell/lib/service_connector.h:48: struct
> >> Constructor {
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> You might consider a more verbose name like
> >> >> >>
> >> >> > ServiceConnectionConstructor :-/
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > public/cpp/shell/lib/service_connector.h#newcode109
> >> >> > mojo/public/cpp/shell/lib/service_connector.h:109:
> >> >> > internal::Constructor<ServiceImpl, Context>::New(context_);
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> nit: no need for the internal:: prefix since you are in the internal
> >> >> >>
> >> >> > namespace
> >> >> >
> >> >> >> already.
> >> >> >>
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > services/public/cpp/view_manager/lib/view_manager_synchronizer.cc
> >> >> > File
> >> >> > mojo/services/public/cpp/view_manager/lib/view_manager_
> >> synchronizer.cc
> >> >> > (right):
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/diff/80001/mojo/
> >> >> > services/public/cpp/view_manager/lib/view_manager_
> >> >> synchronizer.cc#newcode9
> >> >> > mojo/services/public/cpp/view_manager/lib/view_manager_
> >> >> synchronizer.cc:9:
> >> >> > #include "mojo/public/cpp/bindings/allocation_scope.h"
> >> >> > On 2014/05/12 22:04:52, darin wrote:
> >> >> >
> >> >> >> nit: allocation scope does not appear to be needed by this .cc file
> >> >> >>
> >> >> >
> >> >> > Done.
> >> >> >
> >> >> > https://codereview.chromium.org/275363002/
> >> >> >
> >> >>
> >> >
> >> >  To unsubscribe from this group and stop receiving emails from it, send
> >> an
> >> >>
> >> > email
> >> >
> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >> >>
> >> >
> >> >
> >> >
> >> > https://codereview.chromium.org/275363002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> >
> > https://codereview.chromium.org/275363002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-14 20:00:20]

LGTM

https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
File mojo/services/view_manager/view_manager_connection.cc (right):

https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
mojo/services/view_manager/view_manager_connection.cc:90:
InterfaceImpl<IViewManager>::SetClient(client);
It is this case that makes me wonder if the ViewManager interface shouldn't just
have an Initialize method that clients are expected to call.

interface ViewManager {
  Initialize() => (uint16 connection_id, uint32 next_server_change_id);
  ...
};

[sky, 2014-05-14 20:12:29]

https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
File mojo/services/view_manager/view_manager_connection.cc (right):

https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
mojo/services/view_manager/view_manager_connection.cc:90:
InterfaceImpl<IViewManager>::SetClient(client);
On 2014/05/14 20:00:21, darin wrote:
> It is this case that makes me wonder if the ViewManager interface shouldn't
just
> have an Initialize method that clients are expected to call.
> 
> interface ViewManager {
>   Initialize() => (uint16 connection_id, uint32 next_server_change_id);
>   ...
> };

The clients don't know this information, only the server.

[darin (slow to review), 2014-05-14 20:30:35]

That's why I put the method on ViewManager. (I should have typed
IViewManager.)

My suggestion is to replace IViewManagerClient::OnConnectionEstablished
with IViewManager::Initialize.

-Darin


On Wed, May 14, 2014 at 1:12 PM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/275363002/diff/160001/
> mojo/services/view_manager/view_manager_connection.cc
> File mojo/services/view_manager/view_manager_connection.cc (right):
>
> https://codereview.chromium.org/275363002/diff/160001/
> mojo/services/view_manager/view_manager_connection.cc#newcode90
> mojo/services/view_manager/view_manager_connection.cc:90:
> InterfaceImpl<IViewManager>::SetClient(client);
> On 2014/05/14 20:00:21, darin wrote:
>
>> It is this case that makes me wonder if the ViewManager interface
>>
> shouldn't just
>
>> have an Initialize method that clients are expected to call.
>>
>
>  interface ViewManager {
>>    Initialize() => (uint16 connection_id, uint32
>>
> next_server_change_id);
>
>>    ...
>> };
>>
>
> The clients don't know this information, only the server.
>
> https://codereview.chromium.org/275363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-05-14 20:39:10]

Sorry, I haven't followed the whole thread. Why can't the server send
an initial message like ViewManager is doing?

On Wed, May 14, 2014 at 1:30 PM, Darin Fisher <darin@chromium.org> wrote:
> That's why I put the method on ViewManager. (I should have typed
> IViewManager.)
>
> My suggestion is to replace IViewManagerClient::OnConnectionEstablished with
> IViewManager::Initialize.
>
> -Darin
>
>
> On Wed, May 14, 2014 at 1:12 PM, <sky@chromium.org> wrote:
>>
>>
>>
>>
https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
>> File mojo/services/view_manager/view_manager_connection.cc (right):
>>
>>
>>
https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
>> mojo/services/view_manager/view_manager_connection.cc:90:
>> InterfaceImpl<IViewManager>::SetClient(client);
>> On 2014/05/14 20:00:21, darin wrote:
>>>
>>> It is this case that makes me wonder if the ViewManager interface
>>
>> shouldn't just
>>>
>>> have an Initialize method that clients are expected to call.
>>
>>
>>> interface ViewManager {
>>>    Initialize() => (uint16 connection_id, uint32
>>
>> next_server_change_id);
>>>
>>>    ...
>>> };
>>
>>
>> The clients don't know this information, only the server.
>>
>> https://codereview.chromium.org/275363002/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-14 21:44:37]

It can, but then we need to provide a way for the ViewManager to know when
it can do that. It needs client() to be set before it can do that. That
happens after the constructor runs. As you can see in this CL, Dave
overrides the SetClient method to know when to call OnConnectionEstablished.

I'm concerned about two things:

1) Your SetClient implementation now needs to call
InterfaceImpl<IViewManager>::SetClient to actually get the client() set.
2) SetClient doesn't sound like the name of a method where you should do
initialization work.

-Darin


On Wed, May 14, 2014 at 1:39 PM, Scott Violet <sky@chromium.org> wrote:

> Sorry, I haven't followed the whole thread. Why can't the server send
> an initial message like ViewManager is doing?
>
> On Wed, May 14, 2014 at 1:30 PM, Darin Fisher <darin@chromium.org> wrote:
> > That's why I put the method on ViewManager. (I should have typed
> > IViewManager.)
> >
> > My suggestion is to replace IViewManagerClient::OnConnectionEstablished
> with
> > IViewManager::Initialize.
> >
> > -Darin
> >
> >
> > On Wed, May 14, 2014 at 1:12 PM, <sky@chromium.org> wrote:
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
> >> File mojo/services/view_manager/view_manager_connection.cc (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/275363002/diff/160001/mojo/services/view_mana...
> >> mojo/services/view_manager/view_manager_connection.cc:90:
> >> InterfaceImpl<IViewManager>::SetClient(client);
> >> On 2014/05/14 20:00:21, darin wrote:
> >>>
> >>> It is this case that makes me wonder if the ViewManager interface
> >>
> >> shouldn't just
> >>>
> >>> have an Initialize method that clients are expected to call.
> >>
> >>
> >>> interface ViewManager {
> >>>    Initialize() => (uint16 connection_id, uint32
> >>
> >> next_server_change_id);
> >>>
> >>>    ...
> >>> };
> >>
> >>
> >> The clients don't know this information, only the server.
> >>
> >> https://codereview.chromium.org/275363002/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-05-15 01:53:59]

Add OnConnectionEstablished()

[DaveMoore, 2014-05-15 01:59:02]

Add OnConnectionEstablished()

[darin (slow to review), 2014-05-15 18:06:20]

https://codereview.chromium.org/275363002/diff/200001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_impl_internal.h (right):

https://codereview.chromium.org/275363002/diff/200001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:15: class
InterfaceImplBase : public ErrorHandler {
I don't think this interface is needed. InterfaceImplState should just inherit
from ErrorHandler.

https://codereview.chromium.org/275363002/diff/200001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:23: class
WithInterfaceImplBase : public Base {
This class should just be renamed InterfaceImplBase.

https://codereview.chromium.org/275363002/diff/200001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_impl_internal.h:70:
OnConnectionEstablished();
no need for the virtual function call here. this can just be the code in
OnConnectionEstablished.

https://codereview.chromium.org/275363002/diff/200001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc (right):

https://codereview.chromium.org/275363002/diff/200001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:131: BindToProxy(impl,
&calc);
note: BindToProxy returns the first argument, so you can write:

  MathCalculatorImpl* impl = BindToProxy(new MathCalculatorImpl(), &calc);

https://codereview.chromium.org/275363002/diff/200001/mojo/services/dbus_echo...
File mojo/services/dbus_echo/dbus_echo_service.cc (right):

https://codereview.chromium.org/275363002/diff/200001/mojo/services/dbus_echo...
mojo/services/dbus_echo/dbus_echo_service.cc:33: 
nit: spurious new line

[darin (slow to review), 2014-05-15 22:26:03]

Uploaded here w/ these changes applied:
https://codereview.chromium.org/284113009

[darin (slow to review), 2014-05-16 18:19:39]

On 2014/05/15 22:26:03, darin wrote:
> Uploaded here w/ these changes applied:
> https://codereview.chromium.org/284113009

Committed here:
https://src.chromium.org/viewvc/chrome?revision=270945&view=revision