Declient calculator test mojom.

Declient math_calculator.mojom.

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/296e869579125ecf78ddb2e10262e1f4ebbdcf0f

[Aaron Boodman, 2015-01-21 23:38:44]

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

[Aaron Boodman, 2015-01-21 23:38:44]

So I have this working, but it fails in checkdeps because of the reference to
base from the SDK.

Before I figure that out, can you verify this direction is OK with you
otherwise?

[jamesr, 2015-01-22 00:30:04]

Seems like a pretty reasonable change.  I would bite the bullet and avoid //base
here and maybe leave a TODO to use a lambda when we can.

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
File mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc (right):

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:34: class
MathCalculatorImpl : public InterfaceImpl<math::Calculator> {
if you're rewriting these anyway might as well switch from InterfaceImpl<> to
Binding<> as well

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:41: // TODO(aa):
Shouldn't it actually reset total_?
instead of a TODO can you just do it? do the tests rely on it?

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:64:
base::Bind(&MathCalculatorUI::Output, base::Unretained(this))),
since there's only one type of Output it should be pretty trivial to create a
functor object and avoid all the base/ dependencies

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:351: typedef
mojo::Callback<void(double)> Callback;
I think we already have this typedef

[Aaron Boodman, 2015-01-22 22:23:39]

Patchset #2 (id:20001) has been deleted

[Aaron Boodman, 2015-01-22 22:23:44]

Patchset #2 (id:40001) has been deleted

[Aaron Boodman, 2015-01-22 22:27:30]

ptal

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
File mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc (right):

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:34: class
MathCalculatorImpl : public InterfaceImpl<math::Calculator> {
On 2015/01/22 00:30:04, jamesr wrote:
> if you're rewriting these anyway might as well switch from InterfaceImpl<> to
> Binding<> as well

It seems to work fine as InterfaceImpl. I'd rather contain the changes as much
as possible.

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:41: // TODO(aa):
Shouldn't it actually reset total_?
On 2015/01/22 00:30:04, jamesr wrote:
> instead of a TODO can you just do it? do the tests rely on it?

Done.

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:64:
base::Bind(&MathCalculatorUI::Output, base::Unretained(this))),
On 2015/01/22 00:30:04, jamesr wrote:
> since there's only one type of Output it should be pretty trivial to create a
> functor object and avoid all the base/ dependencies

You keep using that word 'functor'. I don't think it means what you think it
means :). (It would be an class with a call operator, which won't help us here
AFAICT).

Anyway, we can make one-off classes with Run() methods that delegate back to the
parent class. But that seems so icky. Instead, I defined a helper that is at
least a little bit generally useful in callback.h. See what you think...

https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:351: typedef
mojo::Callback<void(double)> Callback;
On 2015/01/22 00:30:04, jamesr wrote:
> I think we already have this typedef

Done.

[jamesr, 2015-01-22 22:40:21]

On 2015/01/22 22:27:30, Aaron Boodman wrote:
>
https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
> mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:64:
> base::Bind(&MathCalculatorUI::Output, base::Unretained(this))),
> On 2015/01/22 00:30:04, jamesr wrote:
> > since there's only one type of Output it should be pretty trivial to create
a
> > functor object and avoid all the base/ dependencies
> 
> You keep using that word 'functor'. I don't think it means what you think it
> means :). (It would be an class with a call operator, which won't help us here
> AFAICT).

Sorry, yes.  I don't know what to call a class with a Run() :)

[jamesr, 2015-01-22 22:45:24]

https://codereview.chromium.org/864123002/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/callback.h (right):

https://codereview.chromium.org/864123002/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/callback.h:65: namespace {
anonymous namespace inside a header is almost never what you want - it means
that if two translation units #include this header they will not have a shared
type that they can refer to

https://codereview.chromium.org/864123002/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/callback.h:68: class RunnableImpl {
don't put this in public. If you want it in your test feel free to put it there,
but I think if we're going to expand the public Callback we should have it
actually accept real functors for Sink.  The c++11 patch I linked earlier does
this and it's possible to do so in c++98 with SFINAE but nobody's got around to
doing that.  This adds another confusing layer without letting you use the
syntax you really want.

[Aaron Boodman, 2015-01-22 22:48:18]

On 2015/01/22 22:40:21, jamesr wrote:
> On 2015/01/22 22:27:30, Aaron Boodman wrote:
> >
>
https://codereview.chromium.org/864123002/diff/1/mojo/public/cpp/bindings/tes...
> > mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc:64:
> > base::Bind(&MathCalculatorUI::Output, base::Unretained(this))),
> > On 2015/01/22 00:30:04, jamesr wrote:
> > > since there's only one type of Output it should be pretty trivial to
create
> a
> > > functor object and avoid all the base/ dependencies
> > 
> > You keep using that word 'functor'. I don't think it means what you think it
> > means :). (It would be an class with a call operator, which won't help us
here
> > AFAICT).
> 
> Sorry, yes.  I don't know what to call a class with a Run() :)

Well the code calls it 'Runnable...'

[Aaron Boodman, 2015-01-22 22:49:08]

ptal

https://codereview.chromium.org/864123002/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/callback.h (right):

https://codereview.chromium.org/864123002/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/callback.h:65: namespace {
On 2015/01/22 22:45:24, jamesr wrote:
> anonymous namespace inside a header is almost never what you want - it means
> that if two translation units #include this header they will not have a shared
> type that they can refer to

You're right, thanks.

https://codereview.chromium.org/864123002/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/callback.h:68: class RunnableImpl {
On 2015/01/22 22:45:24, jamesr wrote:
> don't put this in public. If you want it in your test feel free to put it
there,

done.

[jamesr, 2015-01-22 22:50:11]

Thanks, lgtm

Maybe someday I'll actually learn C++ and put the useful thing in callback.h

[Aaron Boodman, 2015-01-22 22:51:37]

Committed patchset #4 (id:100001) manually as
296e869579125ecf78ddb2e10262e1f4ebbdcf0f (presubmit successful).