Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632573002:
Workaround NetworkConnectionManager race condition (Closed)
|
Created:
3 years, 11 months ago by Pete Williamson Modified:
3 years, 11 months ago CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionstyle
Workaround NetworkConnectionManager race condition
Sometimes when GCM NetworkManager fires, Chromium doesn't yet know that
we have a connection, and reports the current connection type as
CONNECTION_NONE, even though android reports that we have a connection.
If we find a time when NetworkConnectionManager says there is no connection,
but android says there is a connection, send a valid connection type down
to the request coordinator, so it can start downloading a file.
We need to merge this to M56.
BUG=680831
Review-Url: https://codereview.chromium.org/2632573002
Cr-Commit-Position: refs/heads/master@{#444121}
Committed: https://chromium.googlesource.com/chromium/src/+/87501749a54ab7879f4233e15a4fdb9b0f6adc46
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add translation function #
Total comments: 6
Messages
Total messages: 17 (5 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org
Sometimes when GCM NetworkManager fires, Chromium doesn't yet know that we have
a connection, and reports the current connection type as CONNECTION_NONE, even
though android reports that we have a connection.
If we find a time when NetworkConnectionManager says there is no connection,
but android says there is a connection, send a valid connection type down to the
request coordinator, so it can start downloading a file.
We need to merge this to M56.
dimich@chromium.org changed reviewers: + dimich@chromium.org
drive-by: https://codereview.chromium.org/2632573002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2632573002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:655: // activeNetwork.getType(). Why not do it now? Sounds like a few lines of code and pretty straightforward, no?
lgtm for 56 patch https://codereview.chromium.org/2632573002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2632573002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:655: // activeNetwork.getType(). On 2017/01/13 02:28:41, Dmitry Titov wrote: > Why not do it now? Sounds like a few lines of code and pretty straightforward, > no? I think the idea was for a simpler merge to 56 (simpler change and test?). I am ok with this for 56 with follow-up or mapping codes (of course). Would be nice to use an existing mapping - not sure if this would work https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a...
On 2017/01/13 02:28:42, Dmitry Titov wrote: > drive-by: > > https://codereview.chromium.org/2632573002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java > (right): > > https://codereview.chromium.org/2632573002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:655: > // activeNetwork.getType(). > Why not do it now? Sounds like a few lines of code and pretty straightforward, > no? OK, added the code to do it now. Is this better? (As DougArnett suggests, I was trying to keep the patch as small as possible for M56. This is a slightly larger, but more robust patch.)
https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:654: connectionType = convertAndroidNetworkTypeToConnectionType(activeNetwork.getType()); With the convert method now why bother with NetworkChangeNotifier? Just use Android ConnectivityManager and convert it?
https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:654: connectionType = convertAndroidNetworkTypeToConnectionType(activeNetwork.getType()); On 2017/01/13 23:13:16, dougarnett wrote: > With the convert method now why bother with NetworkChangeNotifier? > Just use Android ConnectivityManager and convert it? My thinking is that if NCN is available, it has better granularity. It knows the difference between 2G, 3G, and 4G, while android ConnectivityManager does not. So, if we get something from NCN, it is likely better.
Ok, so this iteration at least lets us differentiate between wifi and mobile (and bluetooth). https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:654: connectionType = convertAndroidNetworkTypeToConnectionType(activeNetwork.getType()); On 2017/01/13 23:15:39, Pete Williamson wrote: > On 2017/01/13 23:13:16, dougarnett wrote: > > With the convert method now why bother with NetworkChangeNotifier? > > Just use Android ConnectivityManager and convert it? > > My thinking is that if NCN is available, it has better granularity. It knows > the difference between 2G, 3G, and 4G, while android ConnectivityManager does > not. So, if we get something from NCN, it is likely better. Ah, yuck, looked a bit into the multi-version mess of getting to those classifications! Might be nice if we could kick NCN or I suppose the NetworkChangeNotifierAutoDetect with new current network type (instead of it waiting for its turn to receive the broadcast with the changed network type). https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:667: // for mobile, we don't know if it is 2G, 3G, or 4G, default to worst case of 2G. Possible TODO idea - look at exposing mapping utility method from NCN or NCNAD.
https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:654: connectionType = convertAndroidNetworkTypeToConnectionType(activeNetwork.getType()); On 2017/01/14 00:59:11, dougarnett wrote: > On 2017/01/13 23:15:39, Pete Williamson wrote: > > On 2017/01/13 23:13:16, dougarnett wrote: > > > With the convert method now why bother with NetworkChangeNotifier? > > > Just use Android ConnectivityManager and convert it? > > > > My thinking is that if NCN is available, it has better granularity. It knows > > the difference between 2G, 3G, and 4G, while android ConnectivityManager does > > not. So, if we get something from NCN, it is likely better. > > Ah, yuck, looked a bit into the multi-version mess of getting to those > classifications! > > Might be nice if we could kick NCN or I suppose the > NetworkChangeNotifierAutoDetect with new current network type (instead of it > waiting for its turn to receive the broadcast with the changed network type). One thing I got from the bug I filed on NCN is that we could use our own NCN instance instead of the regular chrome one, and we can ask it to stay awake while chrome is in the background. However, this fix seems simpler for M56. Also, we may be causing lots of unnecessary CPU usage since chrome would watch for all net changes in the background just in case there is a background page request, so that would take a bit of study before we know if it is the right course of action. Furthermore, today we don't really do much with the connection type. Someday we may change our timeout based on connection type. https://codereview.chromium.org/2632573002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:667: // for mobile, we don't know if it is 2G, 3G, or 4G, default to worst case of 2G. On 2017/01/14 00:59:11, dougarnett wrote: > Possible TODO idea - look at exposing mapping utility method from NCN or NCNAD. Noted, but I'd prefer to wait until I checkout the new NCN instance for M57 before adding that TODO. If you want to be sure the idea doesn't get lost, though, I'd be happy to add it.
still lgtm It is interesting to look at fixing NCN as Tarun suggests but we should get this CL ready to land and merge if that doesn't look simple.
The CQ bit was checked by petewil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484682075455130,
"parent_rev": "4eece651a995828d5677e6231fe7af235084dd88", "commit_rev":
"87501749a54ab7879f4233e15a4fdb9b0f6adc46"}
Message was sent while issue was closed.
Description was changed from ========== style Workaround NetworkConnectionManager race condition Sometimes when GCM NetworkManager fires, Chromium doesn't yet know that we have a connection, and reports the current connection type as CONNECTION_NONE, even though android reports that we have a connection. If we find a time when NetworkConnectionManager says there is no connection, but android says there is a connection, send a valid connection type down to the request coordinator, so it can start downloading a file. We need to merge this to M56. BUG=680831 ========== to ========== style Workaround NetworkConnectionManager race condition Sometimes when GCM NetworkManager fires, Chromium doesn't yet know that we have a connection, and reports the current connection type as CONNECTION_NONE, even though android reports that we have a connection. If we find a time when NetworkConnectionManager says there is no connection, but android says there is a connection, send a valid connection type down to the request coordinator, so it can start downloading a file. We need to merge this to M56. BUG=680831 Review-Url: https://codereview.chromium.org/2632573002 Cr-Commit-Position: refs/heads/master@{#444121} Committed: https://chromium.googlesource.com/chromium/src/+/87501749a54ab7879f4233e15a4f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/87501749a54ab7879f4233e15a4f... |
