Fix NetworkSmsHandler to observe Manager and improve API

Fix NetworkSmsHandler to observe Manager and improve API


BUG=133416
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215783

[stevenjb, 2013-03-09 00:00:53]

This is some prep work for using the new SMS handlers (which have been sitting
around unused for longer than I care to think about) in the Ash status area.

[commit-bot: I haz the power, 2013-03-14 18:30:24]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[stevenjb, 2013-03-14 20:32:02]

Oops, clicked the box on the wrong CL. PTAL when convenient.

[Greg Spencer (Chromium), 2013-03-20 18:30:51]

https://codereview.chromium.org/12669004/diff/1/chromeos/network/network_sms_...
File chromeos/network/network_sms_handler.cc (right):

https://codereview.chromium.org/12669004/diff/1/chromeos/network/network_sms_...
chromeos/network/network_sms_handler.cc:37: const size_t kMaxReceivedMessages =
100;
This seems kind of low to me.  My phone defaults to 500 messages of history (and
I set it to 1000).

Or perhaps we shouldn't even provide a way to get old messages here and let
something else handle this that can provide persistence across sessions as well?

https://codereview.chromium.org/12669004/diff/1/chromeos/network/network_sms_...
chromeos/network/network_sms_handler.cc:417: if (received_messages_.size() >=
kMaxReceivedMessages)
perhaps "while" instead of "if"?  If we allow changing the max dynamically
someday, that may be important.

[stevenjb, 2013-03-25 19:51:10]

https://codereview.chromium.org/12669004/diff/1/chromeos/network/network_sms_...
File chromeos/network/network_sms_handler.cc (right):

https://codereview.chromium.org/12669004/diff/1/chromeos/network/network_sms_...
chromeos/network/network_sms_handler.cc:37: const size_t kMaxReceivedMessages =
100;
On 2013/03/20 18:30:52, Greg Spencer (Chromium) wrote:
> This seems kind of low to me.  My phone defaults to 500 messages of history
(and
> I set it to 1000).
> 
> Or perhaps we shouldn't even provide a way to get old messages here and let
> something else handle this that can provide persistence across sessions as
well?

We don't really have a fully featured SMS system, and we don't persist them once
received. This represents the max number of messages retained in a session
(which would include messages received on connect since the previous connection
to the network). Given our limited UI, more than 100 seemed excessive. That
said, it's just a question of memory, so we could make this 1000 if we're
concerned about people loosing messages.

https://codereview.chromium.org/12669004/diff/1/chromeos/network/network_sms_...
chromeos/network/network_sms_handler.cc:417: if (received_messages_.size() >=
kMaxReceivedMessages)
On 2013/03/20 18:30:52, Greg Spencer (Chromium) wrote:
> perhaps "while" instead of "if"?  If we allow changing the max dynamically
> someday, that may be important.

This is the only place we add entries to received_messages_, so if() is
logically sufficient. 
I'll move this logic to an AddReceivedMessage() function to make that more
clear, with a comment that it is the only place messages should be added.
Looking more closely at the code, I think this should be split into
AddReceivedMessage and NotifyMessageReceived, with a helper function
MessageReceived() that calls both, I'll change that.

[Greg Spencer (Chromium), 2013-03-25 20:00:01]

LGTM

[stevenjb, 2013-07-12 17:50:38]

armansito@ - can you look at this CL and add your feedback? You can take it over
if you want to make some changes and push it while I am gone, or check the
'commit' button if you think it looks OK.

There are currently no consumers of NetworkSmsHandler. We need to re-factor
ash/system/chromeos/network/tray_sms.cc to use NetworkSmsHandler and remove the
existing SMS notification code. (There is no need for any of the SMS code to be
in src/chrome). 

To do that we need to test it of course :)

[armansito, 2013-07-12 21:20:22]

On 2013/07/12 17:50:38, stevenjb (chromium) wrote:
> armansito@ - can you look at this CL and add your feedback? You can take it
over
> if you want to make some changes and push it while I am gone, or check the
> 'commit' button if you think it looks OK.
> 
> There are currently no consumers of NetworkSmsHandler. We need to re-factor
> ash/system/chromeos/network/tray_sms.cc to use NetworkSmsHandler and remove
the
> existing SMS notification code. (There is no need for any of the SMS code to
be
> in src/chrome). 
> 
> To do that we need to test it of course :)

lgtm. I'm going to add to my list to write an end-to-end test for SMS using the
pseudo modem manager so this stuff will be tested.

As a side note: We may eventually have shill expose an SMS interface so that
Chrome doesn't talk to ModemManager directly. This would remove the need for
Chrome to distinguish between cromo (ModemManager) and ModemManager1 APIs.
Though we have no immediate plans for this on the systems side it is on my list
so that we can revise the SMS handler when the time comes.

[commit-bot: I haz the power, 2013-07-12 21:22:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/12669004/15002

[commit-bot: I haz the power, 2013-07-12 21:29:00]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[stevenjb, 2013-07-12 21:48:51]

On 2013/07/12 21:20:22, armansito wrote:
> On 2013/07/12 17:50:38, stevenjb (chromium) wrote:
> > armansito@ - can you look at this CL and add your feedback? You can take it
> over
> > if you want to make some changes and push it while I am gone, or check the
> > 'commit' button if you think it looks OK.
> > 
> > There are currently no consumers of NetworkSmsHandler. We need to re-factor
> > ash/system/chromeos/network/tray_sms.cc to use NetworkSmsHandler and remove
> the
> > existing SMS notification code. (There is no need for any of the SMS code to
> be
> > in src/chrome). 
> > 
> > To do that we need to test it of course :)
> 
> lgtm. I'm going to add to my list to write an end-to-end test for SMS using
the
> pseudo modem manager so this stuff will be tested.
> 
> As a side note: We may eventually have shill expose an SMS interface so that
> Chrome doesn't talk to ModemManager directly. This would remove the need for
> Chrome to distinguish between cromo (ModemManager) and ModemManager1 APIs.
> Though we have no immediate plans for this on the systems side it is on my
list
> so that we can revise the SMS handler when the time comes.

Sounds good, glad to have someone familiar with the lower level handlers looking
at the Chrome side of things. 
Feel free to add a networkingPrivate API for SMS, even if it's just for testing.

I'll go ahead and fix the clang error and commit this by hand.

[commit-bot: I haz the power, 2013-08-06 01:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/12669004/49001

[commit-bot: I haz the power, 2013-08-06 03:57:19]

Change committed as 215783