[cros] Add network tray item for mobile network setup.

[cros] Add network tray item for mobile network setup.
This item is only shown for GSM modems w/o SIM card and when mobile config has setup URL defined (initial locale specific).
Initial locale (ex.: en-US, de, es) defines a country where ChromeOS device was bought.

Depends on http://codereview.chromium.org/10141006/

BUG=chrome-os-partner:8092
TEST=Manual.

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

[sadrul, 2012-04-24 10:29:41]

LGTM

[Nikita (slow), 2012-04-24 10:39:44]

+Ben for ash/* owner review.

[Nikita (slow), 2012-04-24 10:41:50]

http://codereview.chromium.org/10201015/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10201015/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:616: if
(cellular->technology_family() == TECHNOLOGY_FAMILY_GSM &&
Based on http://code.google.com/p/chromium-os/issues/detail?id=15444#c31

[commit-bot: I haz the power, 2012-04-24 10:43:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/10201015/1

[commit-bot: I haz the power, 2012-04-24 10:43:31]

Presubmit check for 10201015-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.4s to calculate.

[Nikita (slow), 2012-04-24 12:31:08]

+Steven for changes in chromeos/cros

[Ben Goodger (Google), 2012-04-24 14:59:48]

lgtm

[stevenjb, 2012-04-25 01:29:49]

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:124: // returhs false: GSM modem
and SIM card is present or CDMA modem.
nit: returhs

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:603: if (cellular) {
if (!cellular) return; would simplify this a bit.

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:618: result = true;
The return logic seems flawed now. We can return true now even if we fail to get
the topup url. Shouldn't we return false if !carrier above?

[Nikita (slow), 2012-04-25 14:01:38]

Steven, please take a look.
I've also added handling of the enabling mobile data from tray/settings when SIM
card is absent.

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:124: // returhs false: GSM modem
and SIM card is present or CDMA modem.
On 2012/04/25 01:29:49, stevenjb (chromium) wrote:
> nit: returhs

Done.

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:603: if (cellular) {
On 2012/04/25 01:29:49, stevenjb (chromium) wrote:
> if (!cellular) return; would simplify this a bit.

Done.

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:618: result = true;
On 2012/04/25 01:29:49, stevenjb (chromium) wrote:
> The return logic seems flawed now. We can return true now even if we fail to
get
> the topup url. Shouldn't we return false if !carrier above?

LocaleConfig might be available even if current carrier is not mapped. In fact
that would be exactly the case as we only return setup_url if SIM card is absent
i.e. carrier is not matched.

So GetCellularCarrierInfo returns true if one of these carrier_id/topup_url or
setup_url was initialized.

Code in tray_network.cc handles that in a way so that if carrier information is
defined, it creates top up UI entry. If setup URL is defined, it creates related
UI entry.

Updated comment in header.

Do you think that this should be further split into 2 methods?
bool GetCellularCarrierInfo(std::string* carrier_id, std::string* topup_url); 
bool GetCellularSetupInfo(std::string* setup_url);

[stevenjb, 2012-04-25 16:47:55]

lgtm

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/10201015/diff/3002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:618: result = true;
On 2012/04/25 14:01:38, Nikita Kostylev wrote:
> On 2012/04/25 01:29:49, stevenjb (chromium) wrote:
> > The return logic seems flawed now. We can return true now even if we fail to
> get
> > the topup url. Shouldn't we return false if !carrier above?
> 
> LocaleConfig might be available even if current carrier is not mapped. In fact
> that would be exactly the case as we only return setup_url if SIM card is
absent
> i.e. carrier is not matched.
> 
> So GetCellularCarrierInfo returns true if one of these carrier_id/topup_url or
> setup_url was initialized.
> 
> Code in tray_network.cc handles that in a way so that if carrier information
is
> defined, it creates top up UI entry. If setup URL is defined, it creates
related
> UI entry.
> 
> Updated comment in header.
> 
> Do you think that this should be further split into 2 methods?
> bool GetCellularCarrierInfo(std::string* carrier_id, std::string* topup_url); 
> bool GetCellularSetupInfo(std::string* setup_url);

This seems fine with the updated header. I think it would be better as two
functions, but eventually this should all get moved to ash/ and chromeos/, so I
wouldn't worry about splitting it up as long as the header comment is clear.

[commit-bot: I haz the power, 2012-04-25 16:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/10201015/16002

[commit-bot: I haz the power, 2012-04-25 18:26:12]

Change committed as 133941