Added GetNetworkOperatorID util func in net/base/net_util and a non-trivial implementation for Andr…

Added GetNetworkOperatorID util func in net/base/net_util and a non-trivial implementation for Android platform.

This is the first step to add mobile carrier ID into UMA report. Bug provides background information. Please advise if this is not a proper place to hold the change.

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

[bolian, 2014-04-16 17:42:18]

Hello Matt, Misha and Ben,

This CL is ready to review. It adds an API to get network operator ID if
available. This is in preparation to add carrier ID in to UMA report. The linked
bug has more background information.

Note that I am not sure this is the best place to hold such a change, so please
advise a better alternative if you have one in mind.

Thanks,
Bolian

[bengr, 2014-04-16 17:53:14]

On 2014/04/16 17:42:18, bolian wrote:
> Hello Matt, Misha and Ben,
> 
> This CL is ready to review. It adds an API to get network operator ID if
> available. This is in preparation to add carrier ID in to UMA report. The
linked
> bug has more background information.
> 
> Note that I am not sure this is the best place to hold such a change, so
please
> advise a better alternative if you have one in mind.
> 
> Thanks,
> Bolian

I added Yaron, who might have an opinion on where to put this code.

[mef, 2014-04-16 18:47:07]

On 2014/04/16 17:53:14, bengr1 wrote:
> On 2014/04/16 17:42:18, bolian wrote:
> > Hello Matt, Misha and Ben,
> > 
> > This CL is ready to review. It adds an API to get network operator ID if
> > available. This is in preparation to add carrier ID in to UMA report. The
> linked
> > bug has more background information.
> > 
> > Note that I am not sure this is the best place to hold such a change, so
> please
> > advise a better alternative if you have one in mind.
> > 
> > Thanks,
> > Bolian
> 
> I added Yaron, who might have an opinion on where to put this code.

Would it make sense to put Android-specific implementation into
net/android/network_library.*?

[bolian, 2014-04-16 18:54:51]

I am thinking we need a common API so that other platform can add implementation
later, at least I think it makes sense for chromeos as it has network carrier
too.

[mef, 2014-04-16 20:57:57]

https://codereview.chromium.org/238793007/diff/1/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/238793007/diff/1/net/base/net_util.cc#newcode...
net/base/net_util.cc:1847: return base::android::ConvertJavaStringToUTF8(
I meant move this Android-specific implementation into
net/android/network_library.*

https://codereview.chromium.org/238793007/diff/1/net/base/net_util.cc#newcode...
net/base/net_util.cc:1857: bool NetworkInfo_GetNetworkOperatorID(JNIEnv* env) {
This could go to android-specific module.

[bolian, 2014-04-16 23:40:52]

https://codereview.chromium.org/238793007/diff/1/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/238793007/diff/1/net/base/net_util.cc#newcode...
net/base/net_util.cc:1847: return base::android::ConvertJavaStringToUTF8(
Done. Yes, I think that is better.

https://codereview.chromium.org/238793007/diff/1/net/base/net_util.cc#newcode...
net/base/net_util.cc:1857: bool NetworkInfo_GetNetworkOperatorID(JNIEnv* env) {
On 2014/04/16 20:57:57, mef wrote:
> This could go to android-specific module.

Done.

[bolian, 2014-04-21 17:08:07]

ping.

[mef, 2014-04-21 17:13:42]

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:249:
(TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE);
telephonyManager may be null:

http://stackoverflow.com/questions/13050292/chances-of-telephony-service-retu...

[Yaron, 2014-04-21 17:19:36]

If mmenke/mef are fine with this functionality added to net_util then this lgtm.

If you want to localize it further, there's DeviceTelephonyInfo.java and it's
native counterpart. You could add the functionality there instead.

[mef, 2014-04-21 17:27:28]

On 2014/04/21 17:19:36, Yaron wrote:
> If mmenke/mef are fine with this functionality added to net_util then this
lgtm.
> 
> If you want to localize it further, there's DeviceTelephonyInfo.java and it's
> native counterpart. You could add the functionality there instead.

Good point. Where is this information needed for UMA reports?

[bolian, 2014-04-21 17:47:46]

I plan to call this in chrome/browser/metrics/metrics_network_observer.cc
whenever the network is changed.
I think I can call content/ (where the native part of DeviceTelephonyInfo.java
is) from there.  So, I will make the move.  Thanks.

[bengr, 2014-04-21 20:50:57]

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:244: * Returns
the numeric name (MCC+MNC) of current registered operator.
"of the"

https://codereview.chromium.org/238793007/diff/40001/net/android/network_libr...
File net/android/network_library.h (right):

https://codereview.chromium.org/238793007/diff/40001/net/android/network_libr...
net/android/network_library.h:74: // Returns the numeric name (MCC+MNC) of
current registered operator.
"of the"

https://codereview.chromium.org/238793007/diff/40001/net/android/network_libr...
net/android/network_library.h:74: // Returns the numeric name (MCC+MNC) of
current registered operator.
Also, please explain what MCC+MNC means in the comment.

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util.h#newc...
net/base/net_util.h:526: NET_EXPORT std::string GetNetworkOperatorID();
Why is this also declared in network_library.h?

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util_win.cc...
net/base/net_util_win.cc:289: return "";
Does it makes sense to have a separate return value for not found? E.g.:

bool GetNetworkOperatorID(std::string* id);

[bolian, 2014-04-21 22:37:17]

Moved the change to under content/common/android as suggested by Yaron. Please
take a look again. Thanks.

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:244: * Returns
the numeric name (MCC+MNC) of current registered operator.
On 2014/04/21 20:50:58, bengr1 wrote:
> "of the"

Done.

https://codereview.chromium.org/238793007/diff/40001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:249:
(TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE);
On 2014/04/21 17:13:42, mef wrote:
> telephonyManager may be null:
> 
>
http://stackoverflow.com/questions/13050292/chances-of-telephony-service-retu...

Done.

https://codereview.chromium.org/238793007/diff/40001/net/android/network_libr...
File net/android/network_library.h (right):

https://codereview.chromium.org/238793007/diff/40001/net/android/network_libr...
net/android/network_library.h:74: // Returns the numeric name (MCC+MNC) of
current registered operator.
On 2014/04/21 20:50:58, bengr1 wrote:
> "of the"

Done.

https://codereview.chromium.org/238793007/diff/40001/net/android/network_libr...
net/android/network_library.h:74: // Returns the numeric name (MCC+MNC) of
current registered operator.
On 2014/04/21 20:50:58, bengr1 wrote:
> Also, please explain what MCC+MNC means in the comment.

Done.

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util.h#newc...
net/base/net_util.h:526: NET_EXPORT std::string GetNetworkOperatorID();
Removed. All moved.

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util_win.cc
File net/base/net_util_win.cc (right):

https://codereview.chromium.org/238793007/diff/40001/net/base/net_util_win.cc...
net/base/net_util_win.cc:289: return "";
Android API returns the empty string if no network operator configured, I think
we should do the same.

[mef, 2014-04-21 22:45:18]

lgtm, but I'm not an owner out there...

[bolian, 2014-04-21 23:09:38]

Added Brett, who is an owner.
Hello, Brett, could you take a look at the change?
Thanks!

[Yaron, 2014-04-22 00:26:27]

lgtm

[bengr, 2014-04-22 01:08:18]

lgtm

[mmenke, 2014-04-22 19:40:42]

On 2014/04/22 01:08:18, bengr1 wrote:
> lgtm

I defer to Misha on this one, no need to wait for my review.

[mef, 2014-04-22 19:48:02]

On 2014/04/22 19:40:42, mmenke wrote:
> On 2014/04/22 01:08:18, bengr1 wrote:
> > lgtm
> 
> I defer to Misha on this one, no need to wait for my review.

Thanks, it is not even in net/ anymore...

[bolian, 2014-04-22 21:58:24]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-04-22 21:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bolian@chromium.org/238793007/80001

[commit-bot: I haz the power, 2014-04-22 22:31:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 22:31:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[bolian, 2014-04-23 01:51:29]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-04-23 01:52:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bolian@chromium.org/238793007/80001

[commit-bot: I haz the power, 2014-04-23 03:30:27]

Change committed as 265513