Mojo Example that demos the difference between providing and requesting an interface

Mojo Example that demos the difference between providing and requesting an interface.

BUG=
R=viettrungluu@chromium.org, yzshen@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/405bc1690e199f5d4bf5d75b998c595c87f914f8

Committed: https://chromium.googlesource.com/external/mojo/+/3f2b646ba07013352093dc05f128b98cb18f4e61

[hansmuller, 2014-11-14 18:12:55]

Patchset #1 (id:1) has been deleted

[hansmuller, 2014-11-14 18:13:08]

Patchset #1 (id:20001) has been deleted

[hansmuller, 2014-11-20 01:32:20]

Patchset #1 (id:40001) has been deleted

[hansmuller, 2014-11-20 01:32:30]

Patchset #1 (id:60001) has been deleted

[hansmuller, 2014-11-20 01:37:43]

hansmuller@chromium.org changed reviewers:
+ yzshen@chromium.org

[hansmuller, 2014-11-20 01:37:44]

I wrote this example to help wrap my head around the difference between
|interface&| and |interface| parameters. The demo is a little unusual because it
connects to a service (IntegerService) and then passes it to another service
(IndirectIntegerService). I think it also illustrates a little of what's
required to share an interface proxy among a bunch of application threads.

[Aaron Boodman, 2014-11-20 07:21:58]

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

[Aaron Boodman, 2014-11-20 07:21:59]

fyi

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/README.md (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/README.md:4: This demo is intended to highlight the
difference between requesting a service and providing one. The demo is based on
two services: IntegerService and IndirectIntegerService.
You should wrap at 80 cols.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/README.md:10: This trival interface just manages a
single internal integer that's initialized to 0. The Increment() method returns
the old value.
It doesn't seem the term 'old' is meaningful with this interface. It always
returns a new integer right? How would a client know whether it was pre or post
increment.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/README.md:17: This service delegates to the one
IntegerService provided by the Set() method. Clients use Get() to request a
connection to an IntegerService targets the delegate. This is roughly an
IntegerService "pointer".
s/targets/targetting/

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:32: class
DelegatingIntegerServiceImpl : public InterfaceImpl<IntegerService> {
I think it is possible to use jamesr's new bindings fanciness to avoid this
class, and just bind the incoming InterfaceRequests in Get() more directly to
integer_service_. But I haven't thought it through, it's just a feeling.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:82: // The fact that only
digit appears in each column provies that things
This sentence doesn't parse. Missing the word 'one'?

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:84: 
No empty line necessary.

[qsr, 2014-11-20 08:49:10]

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

[qsr, 2014-11-20 08:49:10]

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:38: thread_.Start();
You might want to put a MessagePumpMojo on this thread, otherwise async wait are
using thread hops.

[hansmuller, 2014-11-20 17:53:04]

Still looking at how I might leverage the new Bind class.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/README.md (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/README.md:4: This demo is intended to highlight the
difference between requesting a service and providing one. The demo is based on
two services: IntegerService and IndirectIntegerService.
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> You should wrap at 80 cols.

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/README.md:10: This trival interface just manages a
single internal integer that's initialized to 0. The Increment() method returns
the old value.
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> It doesn't seem the term 'old' is meaningful with this interface. It always
> returns a new integer right? How would a client know whether it was pre or
post
> increment.

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/README.md:17: This service delegates to the one
IntegerService provided by the Set() method. Clients use Get() to request a
connection to an IntegerService targets the delegate. This is roughly an
IntegerService "pointer".
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> s/targets/targetting/

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:38: thread_.Start();
On 2014/11/20 08:49:10, qsr wrote:
> You might want to put a MessagePumpMojo on this thread, otherwise async wait
are
> using thread hops.

I've just tried that.

    base::Thread::Options options;
    options.message_loop_type = base::MessageLoop::TYPE_CUSTOM;
    options.message_pump_factory = base::Bind(&common::MessagePumpMojo::Create);
    CHECK(thread_.StartWithOptions(options));

It causes the app to SEGV upon ApplicationImpl::Terminate(). The focus of this
demo is really interface vs interface&, so leaving this optimization out seems
OK to me. It sleeps to increase thread overlap after all :-).

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:82: // The fact that only
digit appears in each column provies that things
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> This sentence doesn't parse. Missing the word 'one'?

Yes, thanks for reading all of this.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:82: // The fact that only
digit appears in each column provies that things
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> This sentence doesn't parse. Missing the word 'one'?

And "provies".  Yikes.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:84: 
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> No empty line necessary.

Done.

[yzshen1, 2014-11-20 18:17:18]

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/BUILD.gn (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/BUILD.gn:30: sources = [ "indirect_service_demo.cc" ]
style nit: sources before deps, please.
https://code.google.com/p/chromium/wiki/GNStyleGuide#Ordering_within_a_target

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:19: virtual
~IndirectIntegerServiceImpl() {}
Please use
~xxxx() override {}  // without virtual, but with override.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:21: virtual void
Set(IntegerServicePtr service) override;
style nit: no need to have 'virtual'.  (This was a recent change..)

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:32: class
DelegatingIntegerServiceImpl : public InterfaceImpl<IntegerService> {
On 2014/11/20 07:21:58, Aaron Boodman wrote:
> I think it is possible to use jamesr's new bindings fanciness to avoid this
> class, and just bind the incoming InterfaceRequests in Get() more directly to
> integer_service_. But I haven't thought it through, it's just a feeling.

+1

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:34:
DelegatingIntegerServiceImpl(IndirectIntegerServiceImpl* delegate)
explicit, please.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:40: FROM_HERE,
base::Bind(&DemoTask::Run, base::Unretained(this)));
wrong indent.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:64: IntegerServicePtr
integer_service_;
This needs to be destroyed on |thread_|.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:74: // Increment() method
task_iteration_count times, collect the results in
task_iteration_count_

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:103: tasks_.push_back(new
DemoTask(integer_service.PassMessagePipe(),
Who owns these DemoTask instances?

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:110: const unsigned
task_count_ = 10;
Maybe consider "static const" and name them kTaskCount and kTaskIterationCount.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_service_demo.mojom (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.mojom:12: Set(IntegerService?
service);
wrong indent.

[hansmuller, 2014-11-20 19:27:59]

Still looking at the lifetime issues.  I think I've addressed the rest of the
feedback.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/BUILD.gn (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/BUILD.gn:30: sources = [ "indirect_service_demo.cc" ]
On 2014/11/20 18:17:17, yzshen1 wrote:
> style nit: sources before deps, please.
> https://code.google.com/p/chromium/wiki/GNStyleGuide#Ordering_within_a_target

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:19: virtual
~IndirectIntegerServiceImpl() {}
On 2014/11/20 18:17:17, yzshen1 wrote:
> Please use
> ~xxxx() override {}  // without virtual, but with override.

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:21: virtual void
Set(IntegerServicePtr service) override;
On 2014/11/20 18:17:17, yzshen1 wrote:
> style nit: no need to have 'virtual'.  (This was a recent change..)

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_integer_service.cc:32: class
DelegatingIntegerServiceImpl : public InterfaceImpl<IntegerService> {
On 2014/11/20 18:17:17, yzshen1 wrote:
> On 2014/11/20 07:21:58, Aaron Boodman wrote:
> > I think it is possible to use jamesr's new bindings fanciness to avoid this
> > class, and just bind the incoming InterfaceRequests in Get() more directly
to
> > integer_service_. But I haven't thought it through, it's just a feeling.
> 
> +1

I applied your suggestion - IndirectIntegerServiceImpl now implements
IntegerService (too).  Each request for an IntegerService Binds to the one
IndirectIntegerServiceImpl. Much simpler!

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:40: FROM_HERE,
base::Bind(&DemoTask::Run, base::Unretained(this)));
On 2014/11/20 18:17:17, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:74: // Increment() method
task_iteration_count times, collect the results in
On 2014/11/20 18:17:17, yzshen1 wrote:
> task_iteration_count_

Done.

https://codereview.chromium.org/733563002/diff/80001/examples/indirect_servic...
examples/indirect_service/indirect_service_demo.cc:110: const unsigned
task_count_ = 10;
On 2014/11/20 18:17:17, yzshen1 wrote:
> Maybe consider "static const" and name them kTaskCount and
kTaskIterationCount.

Done.

[hansmuller, 2014-11-20 21:29:08]

hansmuller@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[hansmuller, 2014-11-20 21:29:08]

Explicitly manage the lifetime of DemoTask and its thread, as well as the
IntegerServicePtr held by each DemoTask

I've also specified MessagePumpMojo for DemoTasks' threads.

I ran into an intermittent problem with resetting the IntegerServicePtr.
RunLoop::RemoveFirstInvalidHandle() would often fail in
assert(MOJO_RESULT_DEADLINE_EXCEEDED == result) because sometimes mojo::Wait()
returns MOJO_RESULT_OK. I'm no expert, but this appears to be a possible result
when the handle being tested is ready; even though the timeout deadline is 0.

[yzshen1, 2014-11-20 22:01:45]

Only a few nits.

Thanks!

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/indirect_integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_integer_service.cc:36:
integer_service_->Increment(callback);
You might want to check whether integer_service_ is set.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:39: 
unnecessary empty line.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:67: integer_service_.reset();
// Must be done on thread_.
It seems better to do the reset before running the callback. Although it is not
an issue in the current scenario, theoretically finished_callback_ could destroy
this object synchronously.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:95: virtual void
Initialize(ApplicationImpl* app) override {
nit: no need to have virtual

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:146:
std::vector<scoped_ptr<DemoTask>> tasks_;
This is a library feature of C++11 (move-only objects in std::vector). I don't
think we are allowed to use it.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/indirect_service_demo.mojom (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.mojom:12: Set(IntegerService?
service);
wrong indent.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/integer_service.cc:18: virtual ~IntegerServiceImpl()
{}
nit: use "override" instead of "virtual" whenever possible.

[hansmuller, 2014-11-20 22:35:38]

I've made the changes.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/indirect_integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_integer_service.cc:36:
integer_service_->Increment(callback);
On 2014/11/20 22:01:44, yzshen1 wrote:
> You might want to check whether integer_service_ is set.

Done.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/indirect_service_demo.cc (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:67: integer_service_.reset();
// Must be done on thread_.
On 2014/11/20 22:01:44, yzshen1 wrote:
> It seems better to do the reset before running the callback. Although it is
not
> an issue in the current scenario, theoretically finished_callback_ could
destroy
> this object synchronously.

Good point.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:95: virtual void
Initialize(ApplicationImpl* app) override {
On 2014/11/20 22:01:44, yzshen1 wrote:
> nit: no need to have virtual

Sorry, I missed that one.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.cc:146:
std::vector<scoped_ptr<DemoTask>> tasks_;
On 2014/11/20 22:01:44, yzshen1 wrote:
> This is a library feature of C++11 (move-only objects in std::vector). I don't
> think we are allowed to use it.

OK.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/indirect_service_demo.mojom (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/indirect_service_demo.mojom:12: Set(IntegerService?
service);
On 2014/11/20 22:01:44, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
File examples/indirect_service/integer_service.cc (right):

https://codereview.chromium.org/733563002/diff/120001/examples/indirect_servi...
examples/indirect_service/integer_service.cc:18: virtual ~IntegerServiceImpl()
{}
On 2014/11/20 22:01:44, yzshen1 wrote:
> nit: use "override" instead of "virtual" whenever possible.

Sorry, I thought I had cleaned all of these up.

[yzshen1, 2014-11-20 23:07:14]

lgtm

[viettrungluu, 2014-11-20 23:09:30]

lgtm

[hansmuller, 2014-11-21 00:14:56]

Committed patchset #6 (id:180001) manually as
405bc1690e199f5d4bf5d75b998c595c87f914f8.

[hansmuller, 2014-11-21 17:55:01]

Committed patchset #7 (id:200001) manually as
3f2b646ba07013352093dc05f128b98cb18f4e61.