Support the ModemManager1 interfaces for SMS messages

Support the ModemManager1 interfaces for SMS messages

Add dbus support the org.freedesktop.ModemManager1.SMS and
org.freedesktop.ModemManager1.Modem.Messaging interfaces.

BUG=chromium-os:28421
TEST=chromeos_unittests

Change-Id: I4192cb3179a8d377c0b849d3e097a58d3673a379

[Jason Glasgow, 2012-06-05 16:31:48]

Split the CL as suggested by hashimoto

[hashimoto, 2012-06-06 07:19:44]

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
File chromeos/dbus/modem_messaging_client.cc (right):

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
chromeos/dbus/modem_messaging_client.cc:184: proxy = new
ModemMessagingProxy(bus_, service_name, object_path);
Please don't leave a variable uninitialized.
Declare and initialize |proxy| in one sentence.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
File chromeos/dbus/modem_messaging_client_unittest.cc (right):

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
chromeos/dbus/modem_messaging_client_unittest.cc:133:
EXPECT_TRUE(result.Equals(expected_result_));
Does this line compile?

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
chromeos/dbus/modem_messaging_client_unittest.cc:231: writer.OpenArray("ao",
&array_writer);
I think this should be "o", not "ao".
Does this test really run and succeed?

BTW, You can use MessageWriter::AppendArrayOfObjectPaths here if you like.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc
File chromeos/dbus/sms_client.cc (right):

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc#ne...
chromeos/dbus/sms_client.cc:71: void OnSignalConnected(const std::string&
interface,
No need to have this method.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc#ne...
chromeos/dbus/sms_client.cc:99: CHECK(proxy);
No need to CHECK

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc#ne...
chromeos/dbus/sms_client.cc:102: base::Owned(proxy), callback));
First I thought this trick (deleting |proxy| on OnGetAll) was fine.

But, it turns out that the callback passed to dbus::ObjectProxy is not
guaranteed to be deleted on UI thread.
This can be a problem because an object which owns a WeakPtrFactory must be
deleted on the same thread where it is created. (see base/memory/weak_ptr.h for
detail)

So, given conditions below:
1. 'delete proxy' in OnGetAll does not work since it can leak if the callback is
not called for some reason.
2. base::Owned(proxy) does not work since it cannot guarantee the proxy is
deleted on UI thread.
I think here we should have ProxyMap to ensure proxy is deleted (like
ModemMessagingClient), or merge SMSProxy to SMSClientImpl so we don't have
|proxy| and there is no need to delete it at all (it's easy since we are not
connecting to signal)

[Jason Glasgow, 2012-06-06 20:54:20]

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
File chromeos/dbus/modem_messaging_client.cc (right):

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
chromeos/dbus/modem_messaging_client.cc:184: proxy = new
ModemMessagingProxy(bus_, service_name, object_path);
On 2012/06/06 07:19:44, hashimoto wrote:
> Please don't leave a variable uninitialized.
> Declare and initialize |proxy| in one sentence.

Done.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
File chromeos/dbus/modem_messaging_client_unittest.cc (right):

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/modem_messaging_...
chromeos/dbus/modem_messaging_client_unittest.cc:231: writer.OpenArray("ao",
&array_writer);
On 2012/06/06 07:19:44, hashimoto wrote:
> I think this should be "o", not "ao".
> Does this test really run and succeed?
> 
> BTW, You can use MessageWriter::AppendArrayOfObjectPaths here if you like.

Done.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc
File chromeos/dbus/sms_client.cc (right):

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc#ne...
chromeos/dbus/sms_client.cc:71: void OnSignalConnected(const std::string&
interface,
On 2012/06/06 07:19:44, hashimoto wrote:
> No need to have this method.

Done.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc#ne...
chromeos/dbus/sms_client.cc:99: CHECK(proxy);
On 2012/06/06 07:19:44, hashimoto wrote:
> No need to CHECK

Done.

http://codereview.chromium.org/10533006/diff/1/chromeos/dbus/sms_client.cc#ne...
chromeos/dbus/sms_client.cc:102: base::Owned(proxy), callback));
I'm still working on a solution....
On 2012/06/06 07:19:44, hashimoto wrote:
> First I thought this trick (deleting |proxy| on OnGetAll) was fine.
> 
> But, it turns out that the callback passed to dbus::ObjectProxy is not
> guaranteed to be deleted on UI thread.
> This can be a problem because an object which owns a WeakPtrFactory must be
> deleted on the same thread where it is created. (see base/memory/weak_ptr.h
for
> detail)
> 
> So, given conditions below:
> 1. 'delete proxy' in OnGetAll does not work since it can leak if the callback
is
> not called for some reason.
> 2. base::Owned(proxy) does not work since it cannot guarantee the proxy is
> deleted on UI thread.
> I think here we should have ProxyMap to ensure proxy is deleted (like
> ModemMessagingClient), or merge SMSProxy to SMSClientImpl so we don't have
> |proxy| and there is no need to delete it at all (it's easy since we are not
> connecting to signal)
> 

Done.

[Jason Glasgow, 2012-06-06 20:56:13]

I've now run the unittests, but there still seems to be memory corruption when I
use the new modem_messaging_client, and I know that I still have a memory leak
in sms_client.  I left the leak there just to see if the object destruction was
causing the memory problem, but alas, no.  I will iterate a little further on
this.

[Jason Glasgow, 2012-06-08 03:30:41]

Ran units tests.  And fixed memory problem in sms_watcher.cc (missing resize
before copy).  This is ready, just waiting on an lgtm and somebody with commit
privileges.

[Jason Glasgow, 2012-06-08 03:49:25]

I take that back.... Still need to deal with memory leak.

[Jason Glasgow, 2012-06-08 19:49:13]

Please take another look.  I've solved the memory leak problem as you suggested.

[Jason Glasgow, 2012-06-08 19:49:13]

Please take another look.  I've solved the memory leak problem as you suggested.

[hashimoto, 2012-06-11 07:45:46]

Looks almost good.
Please run `git try -b linux_chromeos,linux_chromeos_clang` (or gcl try if you
are using SVN instead of Git) to ensure this patch compiles with both gcc and
Clang.

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client.cc (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client.cc:172: const dbus::ObjectPath&
object_path) OVERRIDE {
Remove this method.

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client.h (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client.h:66: const dbus::ObjectPath& object_path)
= 0;
Adding a method only used for testing for a public interface seems not a good
idea.

It seems that you are not using this method in gsm_sms_client_unittest.cc nor
modem_messaging_client_unittest.cc, can you delete this method?
If you want to do something which cannot be done with stub, you can use mocks. 
(gmock provides many ways to program your mock
http://code.google.com/p/googlemock/wiki/CheatSheet)

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client_unittest.cc (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client_unittest.cc:133: EXPECT_TRUE(result ==
*expected_result_);
nit: EXPECT_EQ

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/sms_client.cc
File chromeos/dbus/sms_client.cc (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/sms_client.c...
chromeos/dbus/sms_client.cc:95: base::Unretained(sms),
nit: Use base::Owned?

[Jason Glasgow, 2012-06-11 16:44:47]

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client.cc (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client.cc:172: const dbus::ObjectPath&
object_path) OVERRIDE {
On 2012/06/11 07:45:46, hashimoto wrote:
> Remove this method.

Done.

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client.h (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client.h:66: const dbus::ObjectPath& object_path)
= 0;
I have been following the unit test design done in GsmSMSClient.  I will \
figure out another way to do this.  FWIW, this is used in the units tests of
http://codereview.chromium.org/10539007/ .

I agree, that adding a public interface for testing is not desirable.

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client_unittest.cc (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client_unittest.cc:133: EXPECT_TRUE(result ==
*expected_result_);
On 2012/06/11 07:45:46, hashimoto wrote:
> nit: EXPECT_EQ

Done.

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/sms_client.cc
File chromeos/dbus/sms_client.cc (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/sms_client.c...
chromeos/dbus/sms_client.cc:95: base::Unretained(sms),
On 2012/06/11 07:45:46, hashimoto wrote:
> nit: Use base::Owned?

Done.

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/sms_client.c...
chromeos/dbus/sms_client.cc:103: delete sms;
Removed to be consistent with Owned.

[stevenjb (google-dont-use), 2012-06-11 17:15:06]

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
File chromeos/dbus/modem_messaging_client.h (right):

http://codereview.chromium.org/10533006/diff/11001/chromeos/dbus/modem_messag...
chromeos/dbus/modem_messaging_client.h:66: const dbus::ObjectPath& object_path)
= 0;
On 2012/06/11 16:44:47, Jason Glasgow wrote:
> I have been following the unit test design done in GsmSMSClient.  I will \
> figure out another way to do this.  FWIW, this is used in the units tests of
> http://codereview.chromium.org/10539007/ .
> 
> I agree, that adding a public interface for testing is not desirable.

This was added to the NetworkSmsHandler API so that the stub can generate fake
messages from a user action, without requiring the UI code to know whether or
not the implementation is stub, flimflam, or shill. It happens to be a no-op in
the flimflam/shill implementation, but is still a valid part of the public API.

It is the equivalent to "RequestStatusUpdate" in the power manager client. It
indicates that the UI for SMS is active and if we are doing any lazy updates or
updates on a timer, force one now. That should probably be added to comment,
even though I don't think there is anything to implement for flimflam/shill
(i.e. I think that SMS messages are pushed as they arrive, not pulled, right?)

[Jason Glasgow, 2012-06-11 17:46:19]

flimflam/shill really means ModemManager.  Answering stevenjb's question... New
messages arriving are pushed, i.e. ModemManager sends a signal.  But at startup
(or login), the messages are pulled using the List() function.

I am happy to go with either solution -- a public function called
RequestUpdate() or the "hack" of a solution that I've coded which relies on a
special service argument to List().  Both ways allow unit testing, and both have
their negatives.  One slightly pollutes the public interface, whereas the other
ads hacks to the stub implementation.

[stevenjb (google-dont-use), 2012-06-11 18:14:26]

http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messagi...
File chromeos/dbus/modem_messaging_client.cc (right):

http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messagi...
chromeos/dbus/modem_messaging_client.cc:247: callback.Run(no_paths);
Here is how we would like the NetworkSmsHandler to behave with a stub
implementation:

1. A NetworkSmsHandler is instantiated in ash::TraySms::SmsObserver
(ash/system/network/tray_sms.cc). This should not generate any SMS messages.
2. A user clicks on the tray icon. Currently this calls
NetworkSmsHandler::RequestUpdate to indicate that the user is actively looking
at SMS messages (i.e. they should be up to date). Currently in the stub, this is
what generates a sequence of test messages.

I don't especially care how this is implemented, but NetworkSmsHandler should
behave as described with either stub device handler.

[Jason Glasgow, 2012-06-11 19:22:15]

See below.

On 2012/06/11 18:14:26, stevenjb1 wrote:
>
http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messagi...
> File chromeos/dbus/modem_messaging_client.cc (right):
> 
>
http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messagi...
> chromeos/dbus/modem_messaging_client.cc:247: callback.Run(no_paths);
> Here is how we would like the NetworkSmsHandler to behave with a stub
> implementation:
> 
> 1. A NetworkSmsHandler is instantiated in ash::TraySms::SmsObserver
> (ash/system/network/tray_sms.cc). This should not generate any SMS messages.

This will generate two SMSes from the ModemMessagingClientStubImpl constructor
-- /SMS/0 and /SMS/1.  I thought this was important in order to test the List()
functionality of network_sms_handler.cc, which otherwise will not get tested.

I can remove these two initial SMSes, but the network_sms_hander_unittests will
test less functionality.  It is difficult given the number of "Stub" objects to
figure out a way to test without having ModemMessagingClientStubImpl initialized
with a message or two.

> 2. A user clicks on the tray icon. Currently this calls
> NetworkSmsHandler::RequestUpdate to indicate that the user is actively looking
> at SMS messages (i.e. they should be up to date). Currently in the stub, this
is
> what generates a sequence of test messages.
> 
> I don't especially care how this is implemented, but NetworkSmsHandler should
> behave as described with either stub device handler.

I think we've got this covered.

[stevenjb (google-dont-use), 2012-06-11 19:27:08]

On Mon, Jun 11, 2012 at 12:22 PM, <jglasgow@chromium.org> wrote:

> See below.
>
>
> On 2012/06/11 18:14:26, stevenjb1 wrote:
>
> http://codereview.chromium.**org/10533006/diff/5005/**
>
chromeos/dbus/modem_messaging_**client.cc<http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messaging_client.cc>
>
>> File chromeos/dbus/modem_messaging_**client.cc (right):
>>
>
>
> http://codereview.chromium.**org/10533006/diff/5005/**
>
chromeos/dbus/modem_messaging_**client.cc#newcode247<http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messaging_client.cc#newcode247>
>
>> chromeos/dbus/modem_messaging_**client.cc:247: callback.Run(no_paths);
>> Here is how we would like the NetworkSmsHandler to behave with a stub
>> implementation:
>>
>
>  1. A NetworkSmsHandler is instantiated in ash::TraySms::SmsObserver
>> (ash/system/network/tray_sms.**cc). This should not generate any SMS
>> messages.
>>
>
> This will generate two SMSes from the ModemMessagingClientStubImpl
> constructor
> -- /SMS/0 and /SMS/1.  I thought this was important in order to test the
> List()
> functionality of network_sms_handler.cc, which otherwise will not get
> tested.
>
> I can remove these two initial SMSes, but the network_sms_hander_unittests
> will
> test less functionality.  It is difficult given the number of "Stub"
> objects to
> figure out a way to test without having ModemMessagingClientStubImpl
> initialized
> with a message or two.
>
> That's why I created RequestUpdate(). We definitely want the test
coverage, but is very distracting if every time a developer runs with the
stub implementation a SMS message shows up. We call RequestUpdate() when we
want the stub to generate test messages, i.e. always in tests that use the
stub, and on user action in the UI.


>
>  2. A user clicks on the tray icon. Currently this calls
>> NetworkSmsHandler::**RequestUpdate to indicate that the user is actively
>> looking
>> at SMS messages (i.e. they should be up to date). Currently in the stub,
>> this
>>
> is
>
>> what generates a sequence of test messages.
>>
>
>  I don't especially care how this is implemented, but NetworkSmsHandler
>> should
>> behave as described with either stub device handler.
>>
>
> I think we've got this covered.
>
>
http://codereview.chromium.**org/10533006/<http://codereview.chromium.org/105...
>

[Jason Glasgow, 2012-06-11 21:09:48]

http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messagi...
File chromeos/dbus/modem_messaging_client.cc (right):

http://codereview.chromium.org/10533006/diff/5005/chromeos/dbus/modem_messagi...
chromeos/dbus/modem_messaging_client.cc:247: callback.Run(no_paths);
On 2012/06/11 18:14:26, stevenjb1 wrote:
> Here is how we would like the NetworkSmsHandler to behave with a stub
> implementation:
> 
> 1. A NetworkSmsHandler is instantiated in ash::TraySms::SmsObserver
> (ash/system/network/tray_sms.cc). This should not generate any SMS messages.
> 2. A user clicks on the tray icon. Currently this calls
> NetworkSmsHandler::RequestUpdate to indicate that the user is actively looking
> at SMS messages (i.e. they should be up to date). Currently in the stub, this
is
> what generates a sequence of test messages.
> 
> I don't especially care how this is implemented, but NetworkSmsHandler should
> behave as described with either stub device handler.

Done.

[stevenjb, 2012-06-11 22:06:40]

lgtm

[hashimoto, 2012-06-12 02:24:04]

lgtm with a nit.
Thank for keeping the production code clean.

http://codereview.chromium.org/10533006/diff/12023/chromeos/dbus/mock_modem_m...
File chromeos/dbus/mock_modem_messaging_client.h (right):

http://codereview.chromium.org/10533006/diff/12023/chromeos/dbus/mock_modem_m...
chromeos/dbus/mock_modem_messaging_client.h:36: MOCK_METHOD2(RequestUpdate,
void(const std::string& service_name,
nit: Remove this.

[commit-bot: I haz the power, 2012-06-12 03:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jglasgow@chromium.org/10533006/12023

[commit-bot: I haz the power, 2012-06-12 04:06:23]

Try job failure for 10533006-12023 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Ben Chan, 2012-06-12 08:19:40]

On 2012/06/12 04:06:23, I haz the power (commit-bot) wrote:
> Try job failure for 10533006-12023 (retry) on linux_chromeos for step
"compile"
> (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Seems like CHROMEOS_EXPORT is missing on SmsClient, which fails the linking of
unit tests

[Ben Chan, 2012-06-12 09:13:38]

On 2012/06/12 08:19:40, Ben Chan wrote:
> On 2012/06/12 04:06:23, I haz the power (commit-bot) wrote:
> > Try job failure for 10533006-12023 (retry) on linux_chromeos for step
> "compile"
> > (clobber build).
> > It's a second try, previously, step "compile" failed.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> 
> Seems like CHROMEOS_EXPORT is missing on SmsClient, which fails the linking of
> unit tests

+jglasglow: your CL is committed