New issue
Advanced search Search tips

Issue 688121 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 490870



Sign in to add a comment

NQE prefs may be lost depending on the field trial

Project Member Reported by tbansal@chromium.org, Feb 2 2017

Issue description

In Network Quality Estimator (NQE), currently the prefs are read to the store only if the reading of cache is enabled via field trial. Otherwise, the network quality store is initialized empty. Later, network quality store writes back the store contents to the prefs.

Consider the case when reading of prefs is not enabled. In that case, store would initialize empty, and later it would overwrite the prefs. This leads to loss of data in the network quality prefs.  

Instead, network quality should always read the prefs, but not use them if cache reading is disabled.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8

commit 4ff2bf0b92b736c30c01f584ee41fd10f19f63a8
Author: tbansal <tbansal@chromium.org>
Date: Sat Feb 04 00:35:48 2017

Read NQE prefs to network quality store under all cases

In Network Quality Estimator (NQE), change the logic to read the
prefs even if caching is not enabled via finch trial. If the caching
is  disabled, the network quality store is not queried by NQE.

BUG= 688121 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2672733002
Cr-Commit-Position: refs/heads/master@{#448118}

[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/chrome/browser/net/nqe/ui_network_quality_estimator_service.cc
[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc
[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/net/nqe/network_quality_estimator.h
[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/4ff2bf0b92b736c30c01f584ee41fd10f19f63a8/net/nqe/network_quality_estimator_unittest.cc

Labels: Merge-Request-57
Project Member

Comment 3 by sheriffbot@chromium.org, Feb 6 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 6 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d2efc6efe4277d7b755caa99dc1b188cb6b7044

commit 3d2efc6efe4277d7b755caa99dc1b188cb6b7044
Author: Tarun Bansal <tbansal@google.com>
Date: Mon Feb 06 18:47:45 2017

Read NQE prefs to network quality store under all cases

In Network Quality Estimator (NQE), change the logic to read the
prefs even if caching is not enabled via finch trial. If the caching
is  disabled, the network quality store is not queried by NQE.

BUG= 688121 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2672733002
Cr-Commit-Position: refs/heads/master@{#448118}
(cherry picked from commit 4ff2bf0b92b736c30c01f584ee41fd10f19f63a8)

Review-Url: https://codereview.chromium.org/2674393002 .
Cr-Commit-Position: refs/branch-heads/2987@{#336}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/chrome/browser/net/nqe/ui_network_quality_estimator_service.cc
[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc
[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/net/nqe/network_quality_estimator.h
[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/3d2efc6efe4277d7b755caa99dc1b188cb6b7044/net/nqe/network_quality_estimator_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fad042aa5ace0033293dbbddcd7204fafccd4bdc

commit fad042aa5ace0033293dbbddcd7204fafccd4bdc
Author: tbansal <tbansal@chromium.org>
Date: Thu Feb 09 19:27:20 2017

NQE: Always read prefs in cronet

Always read the network quality estimator (NQE) prefs in Cronet, and
store the read prefs in the network quality store. This
ensures that prefs on the disk are not written over by empty prefs by
the store.

BUG= 688121 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2671233003
Cr-Commit-Position: refs/heads/master@{#449373}

[modify] https://crrev.com/fad042aa5ace0033293dbbddcd7204fafccd4bdc/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/fad042aa5ace0033293dbbddcd7204fafccd4bdc/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java

Components: Internals>Network>NetworkQuality
Labels: M-57
Status: Fixed (was: Started)
Labels: -nqe
Blocking: 490870

Sign in to add a comment