GetCurrentConnectionType (in NetworkChangeNotifier.java) gets actual connection type if the value i…

GetCurrentConnectionType (in NetworkChangeNotifier.java) gets actual connection type if the value is CONNECTION_UNKNOWN.

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

[bolian, 2014-05-06 01:40:57]

Hello,

This CL makes getCurrentConnectionType try to get actual connection type when it
is unknown.

Thanks,
Bolian

[Yaron, 2014-05-06 03:35:55]

yfriedman->pliard

[bengr, 2014-05-06 17:16:34]

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:80: if
(mCurrentConnectionType == CONNECTION_UNKNOWN) {
nit: CONNECTION_UNKNOW == mCurrentConnectionType

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:219:
if (mObserver == null) {
Why would mObserver sometimes be null?

[bolian, 2014-05-06 17:22:05]

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:80: if
(mCurrentConnectionType == CONNECTION_UNKNOWN) {
On 2014/05/06 17:16:35, bengr1 wrote:
> nit: CONNECTION_UNKNOW == mCurrentConnectionType

Done.

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/264973019/diff/1/net/android/java/src/org/chr...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:219:
if (mObserver == null) {
When no observer is needed, as the way used by this CL in
getCurrentConnectionType().

[bengr, 2014-05-06 17:25:16]

lgtm

[bolian, 2014-05-06 23:03:45]

Hi Philippe,

Did you get a chance to look at this? We need this fix so that the histograms
could log on the correct connection type.

Thanks,
Bolian

[Philippe, 2014-05-07 08:02:31]

Thanks Bolian, and sorry for the delay. See me comment below :)

Also, are you testing with Chrome or ChromeShell/ContentShell? I'm not sure we
are initializing the Java-side of the NetworkChangeNotifier properly on
non-Chrome targets.

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:81:
NetworkChangeNotifierAutoDetect autoDetector =
NetworkChangeNotifierAutoDetect can't be used on WebView since it requires an
extra permission.

Are you sure you are encountering the problem described in the bug?
|mCurrentConnectionType| is set on line 143 which should happen early during
initialization.

It's not clear to me whether in your case |mCurrentConnectionType| is set to
CONNECTION_UNKNOWN because the field has never be set or because we are failing
to identify the return value of mConnectivityManagerDelegate.getNetworkType().

Can you reproduce the issue consistently?

[bolian, 2014-05-07 15:30:10]

Hi Philippe,

Thanks for the reply. I was testing on Chrome, using my private build.

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:81:
NetworkChangeNotifierAutoDetect autoDetector =
I do see the problem consistently in my private Chrome build, in two phones.
Given the permission problem, looks like I need to make sure
setAutoDetectConnectivityState get called for Chrome. I will check why it did
not happen for me.

[Philippe, 2014-05-07 15:33:20]

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:81:
NetworkChangeNotifierAutoDetect autoDetector =
On 2014/05/07 15:30:11, bolian wrote:
> I do see the problem consistently in my private Chrome build, in two phones.
> Given the permission problem, looks like I need to make sure
> setAutoDetectConnectivityState get called for Chrome. I will check why it did
> not happen for me.

Thanks Bolian, also can you check whether
setAutoDetectConnectivityStateInternal() is correctly called at startup?

[bolian, 2014-05-07 21:03:57]

Thanks, I think this will better fix it. ptal

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:81:
NetworkChangeNotifierAutoDetect autoDetector =
I think I know the problem now. NetworkChangeNotifierDelegateAndroid calls and
caches getCurrentConnectionType before it is detected. I think we need to notify
a connection type change when it is detected (i.e. change from unknown to some
known type).

[Philippe, 2014-05-07 22:45:41]

On 2014/05/07 21:03:57, bolian wrote:
> Thanks, I think this will better fix it. ptal
> 
>
https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
> File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):
> 
>
https://codereview.chromium.org/264973019/diff/20001/net/android/java/src/org...
> net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:81:
> NetworkChangeNotifierAutoDetect autoDetector =
> I think I know the problem now. NetworkChangeNotifierDelegateAndroid calls and
> caches getCurrentConnectionType before it is detected. I think we need to
notify
> a connection type change when it is detected (i.e. change from unknown to some
> known type).

Lgtm, thanks!

[bolian, 2014-05-07 22:55:46]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-05-07 22:56:55]

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

[commit-bot: I haz the power, 2014-05-08 04:18:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 04:47:07]

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

[commit-bot: I haz the power, 2014-05-08 04:47:08]

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

[bolian, 2014-05-08 17:10:26]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-05-08 17:11:48]

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

[commit-bot: I haz the power, 2014-05-08 22:37:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-09 00:08:21]

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

[commit-bot: I haz the power, 2014-05-09 00:08:22]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[bolian, 2014-05-09 07:16:20]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-05-09 07:18:06]

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

[commit-bot: I haz the power, 2014-05-09 08:56:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-09 10:22:45]

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

[commit-bot: I haz the power, 2014-05-09 10:22:47]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[bolian, 2014-05-09 16:42:09]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-05-09 16:43:10]

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

[bolian, 2014-05-09 17:43:44]

Looks like the expectation was the updateCurrentConnectionType should always
notify observer even connection type is not change because other aspect of the
network could have been change (e.g. WiFi SSD). So, I revert that part.

[bolian, 2014-05-09 17:43:54]

The CQ bit was checked by bolian@chromium.org

[commit-bot: I haz the power, 2014-05-09 17:45:55]

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

[commit-bot: I haz the power, 2014-05-10 04:49:39]

Change committed as 269529