SMSWatcher: Use the ModemManager1 dbus interfaces

SMSWatcher: Use the ModemManager1 dbus interfaces

Adapt SmsWatcher to monitor both the ModemManager and
ModemManager1 dbus interfaces for SMS messages.

BUG=chromium-os:28421
TEST=test in the UI using a modem with SMSes

Change-Id: Id7a67a9b0d9274e9e3bc79d07d5dc9b928b81479


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141702

[Jason Glasgow, 2012-06-05 16:30:52]

Split the cl as suggested by hashimoto

[stevenjb, 2012-06-05 17:48:05]

http://codereview.chromium.org/10532007/diff/1/chrome/browser/chromeos/cros/s...
File chrome/browser/chromeos/cros/sms_watcher.cc (right):

http://codereview.chromium.org/10532007/diff/1/chrome/browser/chromeos/cros/s...
chrome/browser/chromeos/cros/sms_watcher.cc:72: class GsmWatcher : public
CrosNetworkWatcher {
I think what is confusing me is that these two classes need a common base class
so that either can be stored in watcher_, but it isn't clear to me that they
need to be CrosNetworkWatcher? I think this would be more clear if they had
their own common base class. We might even be able to put some common members in
it.

[Jason Glasgow, 2012-06-05 18:43:44]

http://codereview.chromium.org/10532007/diff/1/chrome/browser/chromeos/cros/s...
File chrome/browser/chromeos/cros/sms_watcher.cc (right):

http://codereview.chromium.org/10532007/diff/1/chrome/browser/chromeos/cros/s...
chrome/browser/chromeos/cros/sms_watcher.cc:72: class GsmWatcher : public
CrosNetworkWatcher {
Yes -- all they really need is a common base class with a virtual destructor.  I
can do that.  I will look to see if there is common code (or members).

 On 2012/06/05 17:48:05, stevenjb (chromium) wrote:
> I think what is confusing me is that these two classes need a common base
class
> so that either can be stored in watcher_, but it isn't clear to me that they
> need to be CrosNetworkWatcher? I think this would be more clear if they had
> their own common base class. We might even be able to put some common members
in
> it.

[Jason Glasgow, 2012-06-06 19:56:57]

PTAL
On 2012/06/05 18:43:44, Jason Glasgow wrote:
>
http://codereview.chromium.org/10532007/diff/1/chrome/browser/chromeos/cros/s...
> File chrome/browser/chromeos/cros/sms_watcher.cc (right):
> 
>
http://codereview.chromium.org/10532007/diff/1/chrome/browser/chromeos/cros/s...
> chrome/browser/chromeos/cros/sms_watcher.cc:72: class GsmWatcher : public
> CrosNetworkWatcher {
> Yes -- all they really need is a common base class with a virtual destructor. 
I
> can do that.  I will look to see if there is common code (or members).
> 
>  On 2012/06/05 17:48:05, stevenjb (chromium) wrote:
> > I think what is confusing me is that these two classes need a common base
> class
> > so that either can be stored in watcher_, but it isn't clear to me that they
> > need to be CrosNetworkWatcher? I think this would be more clear if they had
> > their own common base class. We might even be able to put some common
members
> in
> > it.

[stevenjb, 2012-06-06 23:04:18]

https://chromiumcodereview.appspot.com/10532007/diff/4/chrome/browser/chromeo...
File chrome/browser/chromeos/cros/sms_watcher.cc (right):

https://chromiumcodereview.appspot.com/10532007/diff/4/chrome/browser/chromeo...
chrome/browser/chromeos/cros/sms_watcher.cc:143: LOG(WARNING) << "SMS did not
contain a number";
nit: {}

https://chromiumcodereview.appspot.com/10532007/diff/4/chrome/browser/chromeo...
chrome/browser/chromeos/cros/sms_watcher.cc:167: sms.smsc = smsc.c_str();
nit: {}

https://chromiumcodereview.appspot.com/10532007/diff/4/chrome/browser/chromeo...
chrome/browser/chromeos/cros/sms_watcher.cc:172: validity = -1;
nit: {}

https://chromiumcodereview.appspot.com/10532007/diff/4/chrome/browser/chromeo...
chrome/browser/chromeos/cros/sms_watcher.cc:178: msgclass = -1;
nit: {}

https://chromiumcodereview.appspot.com/10532007/diff/4/chrome/browser/chromeo...
chrome/browser/chromeos/cros/sms_watcher.cc:418: // TODO(jglasgow): or is it
modemmanager::kModemManager1 ???
Has this been figured out?

[stevenjb, 2012-06-06 23:04:32]

lgtm w/ nits

[Jason Glasgow, 2012-06-08 03:28:34]

It works!  Now that the resize has been added the SMSes are delivered and
display properly.

http://codereview.chromium.org/10532007/diff/4/chrome/browser/chromeos/cros/s...
File chrome/browser/chromeos/cros/sms_watcher.cc (right):

http://codereview.chromium.org/10532007/diff/4/chrome/browser/chromeos/cros/s...
chrome/browser/chromeos/cros/sms_watcher.cc:418: // TODO(jglasgow): or is it
modemmanager::kModemManager1 ???
Yes, this has been figured out.  I need to remove the TODO.  Good catch.
On 2012/06/06 23:04:18, stevenjb (chromium) wrote:
> Has this been figured out?

[stevenjb (google-dont-use), 2012-06-08 19:56:48]

lgtm

[commit-bot: I haz the power, 2012-06-12 17:51:38]

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

[commit-bot: I haz the power, 2012-06-12 19:15:08]

Change committed as 141702