Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322183002:
Add Network Quality Estimator (NQE) pref manager (Closed)
DescriptionAdd Network Quality Estimator (NQE) pref manager
The PrefManager class owns an IO Object (CacheObserver) and
a Pref object (PrefDelegate).
The embedder would need to implement PrefDelegate and create
an instance of PrefManager for prefs to work. Right now, only
writes to the prefs is implemented. Reads would come in the
next CL, along with the Chromium implementation.
BUG=490870
Committed: https://crrev.com/ec22cb6b67efd63c4b682b8964d690d401808b77
Cr-Commit-Position: refs/heads/master@{#420786}
Patch Set 1 : PS #
Total comments: 15
Patch Set 2 : rebased, addressed kundaji and ryansturm comments #
Total comments: 6
Patch Set 3 : Addressed kundaji comments #
Total comments: 2
Patch Set 4 : Rebased, Addressed kundaji comments #
Total comments: 28
Patch Set 5 : rebased #Patch Set 6 : PS #Patch Set 7 : rebased #
Messages
Total messages: 46 (27 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add nqe pref manager BUG= ========== to ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG= ==========
Description was changed from ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG= ========== to ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG= ==========
Description was changed from ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG= ========== to ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG=490870 ==========
tbansal@chromium.org changed reviewers: + kundaji@chromium.org
kundaji: ptal. Thanks.
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
kundaji, ryanstrum: PTAL. Thanks.
https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:90: network_weak_ptr_factory_.reset( |network_weak_ptr_factory_| unused? https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:44: class NET_EXPORT NetworkQualitiesPrefsManager { Does this class need to be prefs specific? Could this be a generic NetworkQualitiesCacheObserver on a different thread? This way if something else needs to listen to these events on the UI thread, the you can reuse this functionality. Similar to UINetworkEstimatorService using IONetworkQualityObserver to propagate events to UI thread. Instead of hardcoding threads, as UINetworkEstimatorService does, the task runner could be passed in.
https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:19: class NetworkQualitiesPrefsManager::CacheObserver Is it cleaner to have NetworkQualitiesPrefsManager inherit from NetworkQualitiesCacheObserver directly? It only uses one method, and there isn't really a threading issue since NetworkQualitiesPrefsManager owns the lifetime management as a unique_ptr. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:51: callback_task_runner_->PostTask( Why do you post this task directly to the prefs thread? It might be cleaner to call the callback on the network thread and have NetworkQualitiesPrefsManager do the PostTask. That way you don't have to pass a taskrunner in. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:100: pref_task_runner_, network_quality_estimator)); Instead of passing in network_quality_estimator, can you just add the observer from here? https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:22: namespace nqe { What's the rationale behind nqe being a namespace for some NQE classes and not others? https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:101: std::unique_ptr<base::WeakPtrFactory<NetworkQualitiesPrefsManager>> Do these weak factories need to be std::unique_ptr? I think weak_ptr binds to the thread when the first pointer is dereferenced, not when the factory is created. Please remove the network one from this CL.
kundaji, ryansturm: ptal. thanks. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:19: class NetworkQualitiesPrefsManager::CacheObserver On 2016/09/12 20:26:55, Ryan Sturm wrote: > Is it cleaner to have NetworkQualitiesPrefsManager inherit from > NetworkQualitiesCacheObserver directly? It only uses one method, and there isn't > really a threading issue since NetworkQualitiesPrefsManager owns the lifetime > management as a unique_ptr. Done. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:51: callback_task_runner_->PostTask( On 2016/09/12 20:26:55, Ryan Sturm wrote: > Why do you post this task directly to the prefs thread? It might be cleaner to > call the callback on the network thread and have NetworkQualitiesPrefsManager do > the PostTask. That way you don't have to pass a taskrunner in. Removed CacheObserver. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:90: network_weak_ptr_factory_.reset( On 2016/09/09 20:30:40, kundaji wrote: > |network_weak_ptr_factory_| unused? Done. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:100: pref_task_runner_, network_quality_estimator)); On 2016/09/12 20:26:55, Ryan Sturm wrote: > Instead of passing in network_quality_estimator, can you just add the observer > from here? Done. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:22: namespace nqe { On 2016/09/12 20:26:55, Ryan Sturm wrote: > What's the rationale behind nqe being a namespace for some NQE classes and not > others? Anything that can be called outside from //net should be in net namespace. Other than that, I try to keep things in net::nqe::internal namespace. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:44: class NET_EXPORT NetworkQualitiesPrefsManager { On 2016/09/09 20:30:40, kundaji wrote: > Does this class need to be prefs specific? Could this be a generic > NetworkQualitiesCacheObserver on a different thread? > > This way if something else needs to listen to these events on the UI thread, the > you can reuse this functionality. > > Similar to UINetworkEstimatorService using IONetworkQualityObserver to propagate > events to UI thread. Instead of hardcoding threads, as UINetworkEstimatorService > does, the task runner could be passed in. I have rewritten the class comment to make it clearer. https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:101: std::unique_ptr<base::WeakPtrFactory<NetworkQualitiesPrefsManager>> On 2016/09/12 20:26:55, Ryan Sturm wrote: > Do these weak factories need to be std::unique_ptr? I think weak_ptr binds to > the thread when the first pointer is dereferenced, not when the factory is > created. Please remove the network one from this CL. Done.
lgtm https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:35: // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and How about starting with: // Propagates network quality pref changes from network // thread to the pref delegate on the pref thread.
ptal. thanks. https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:35: // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and On 2016/09/15 21:13:16, kundaji wrote: > How about starting with: > // Propagates network quality pref changes from network > // thread to the pref delegate on the pref thread. Your comment made me think what exactly class comments should include. https://google.github.io/styleguide/cppguide.html#Class_Comments says that it should include "what the class is for" and "how the class should be used". It seems that including the details on how the network quality is moved from network thread to pref thread seems like an internal detail. wdyt?
https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:35: // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and On 2016/09/15 21:32:30, tbansal1 wrote: > On 2016/09/15 21:13:16, kundaji wrote: > > How about starting with: > > // Propagates network quality pref changes from network > > // thread to the pref delegate on the pref thread. > > Your comment made me think what exactly class comments should include. > https://google.github.io/styleguide/cppguide.html#Class_Comments says that it > should include "what the class is for" and "how the class should be used". > > It seems that including the details on how the network quality is moved from > network thread to pref thread seems like an internal detail. wdyt? It seems to me my suggestion sticks to these exact guidelines. Or am I missing something? "what the class is for": For propagating network quality pref changes from network thread to pref_delegate_ thread. Isn't it? If these events were already available on the pref_delegate_ thread then you wouldn't need this class. I agree that there is no need to say how you implement the thread hop, which happens to be using task runner primitives. https://codereview.chromium.org/2322183002/diff/60001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/60001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:20: pref_task_runner_(base::ThreadTaskRunnerHandle::Get()), Import what you use: #include "base/sequenced_task_runner.h" #include "base/threading/thread_task_runner_handle.h"
lgtm https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/20001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:22: namespace nqe { On 2016/09/15 21:00:59, tbansal1 wrote: > On 2016/09/12 20:26:55, Ryan Sturm wrote: > > What's the rationale behind nqe being a namespace for some NQE classes and not > > others? > > Anything that can be called outside from //net should be in net namespace. Other > than that, I try to keep things in net::nqe::internal namespace. Acknowledged. https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:35: // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and On 2016/09/15 22:46:57, kundaji wrote: > On 2016/09/15 21:32:30, tbansal1 wrote: > > On 2016/09/15 21:13:16, kundaji wrote: > > > How about starting with: > > > // Propagates network quality pref changes from network > > > // thread to the pref delegate on the pref thread. > > > > Your comment made me think what exactly class comments should include. > > https://google.github.io/styleguide/cppguide.html#Class_Comments says that it > > should include "what the class is for" and "how the class should be used". > > > > It seems that including the details on how the network quality is moved from > > network thread to pref thread seems like an internal detail. wdyt? > > It seems to me my suggestion sticks to these exact guidelines. Or am I missing > something? > > "what the class is for": For propagating network quality pref changes from > network thread to pref_delegate_ thread. > > Isn't it? If these events were already available on the pref_delegate_ thread > then you wouldn't need this class. > > I agree that there is no need to say how you implement the thread hop, which > happens to be using task runner primitives. Sounds like you guys are arguing about what the consumer needs to know. If the consumer *should* know that NQE is only on the network thread, then adding the comment is useful. I'm of the belief that a consumer of this shouldn't need to know how NQE is implemented; they should just know that they can get evetns about network quality on the pref thread.
https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:35: // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and On 2016/09/16 16:23:41, Ryan Sturm wrote: > On 2016/09/15 22:46:57, kundaji wrote: > > On 2016/09/15 21:32:30, tbansal1 wrote: > > > On 2016/09/15 21:13:16, kundaji wrote: > > > > How about starting with: > > > > // Propagates network quality pref changes from network > > > > // thread to the pref delegate on the pref thread. > > > > > > Your comment made me think what exactly class comments should include. > > > https://google.github.io/styleguide/cppguide.html#Class_Comments says that > it > > > should include "what the class is for" and "how the class should be used". > > > > > > It seems that including the details on how the network quality is moved from > > > network thread to pref thread seems like an internal detail. wdyt? > > > > It seems to me my suggestion sticks to these exact guidelines. Or am I missing > > something? > > > > "what the class is for": For propagating network quality pref changes from > > network thread to pref_delegate_ thread. > > > > Isn't it? If these events were already available on the pref_delegate_ thread > > then you wouldn't need this class. > > > > I agree that there is no need to say how you implement the thread hop, which > > happens to be using task runner primitives. > > Sounds like you guys are arguing about what the consumer needs to know. If the > consumer *should* know that NQE is only on the network thread, then adding the > comment is useful. I'm of the belief that a consumer of this shouldn't need to > know how NQE is implemented; they should just know that they can get evetns > about network quality on the pref thread. My comment suggestion was mainly for any engineer that happens to look at them. It is not obvious to me from the name of this class and the comments here, what the class is for. I don't have a strong opinion, since now I already know what this class does. Please feel free to disregard if you prefer the comments as written:)
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. thanks. https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/40001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:35: // Using the provided PrefDelegate, NetworkQualitiesPrefsManager creates and On 2016/09/16 17:01:28, kundaji wrote: > On 2016/09/16 16:23:41, Ryan Sturm wrote: > > On 2016/09/15 22:46:57, kundaji wrote: > > > On 2016/09/15 21:32:30, tbansal1 wrote: > > > > On 2016/09/15 21:13:16, kundaji wrote: > > > > > How about starting with: > > > > > // Propagates network quality pref changes from network > > > > > // thread to the pref delegate on the pref thread. > > > > > > > > Your comment made me think what exactly class comments should include. > > > > https://google.github.io/styleguide/cppguide.html#Class_Comments says that > > it > > > > should include "what the class is for" and "how the class should be used". > > > > > > > > It seems that including the details on how the network quality is moved > from > > > > network thread to pref thread seems like an internal detail. wdyt? > > > > > > It seems to me my suggestion sticks to these exact guidelines. Or am I > missing > > > something? > > > > > > "what the class is for": For propagating network quality pref changes from > > > network thread to pref_delegate_ thread. > > > > > > Isn't it? If these events were already available on the pref_delegate_ > thread > > > then you wouldn't need this class. > > > > > > I agree that there is no need to say how you implement the thread hop, which > > > happens to be using task runner primitives. > > > > Sounds like you guys are arguing about what the consumer needs to know. If the > > consumer *should* know that NQE is only on the network thread, then adding the > > comment is useful. I'm of the belief that a consumer of this shouldn't need to > > know how NQE is implemented; they should just know that they can get evetns > > about network quality on the pref thread. > > My comment suggestion was mainly for any engineer that happens to look at them. > > It is not obvious to me from the name of this class and the comments here, what > the class is for. > > I don't have a strong opinion, since now I already know what this class does. > Please feel free to disregard if you prefer the comments as written:) I added the suggested comment, although I added it in the middle (not in the beginning as suggested). Thanks.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
rebased. bengr: ptal. thanks. https://codereview.chromium.org/2322183002/diff/60001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/60001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.cc:20: pref_task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2016/09/15 22:46:57, kundaji wrote: > Import what you use: > #include "base/sequenced_task_runner.h" > #include "base/threading/thread_task_runner_handle.h" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h File net/nqe/network_id.h (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h#n... net/nqe/network_id.h:46: return id + kValueSeparator + It seems like overill to define kValueSeparator. I would either do: return id + "," + ... or return StringPrintf("%s,%s", id.c_str(), ... https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:19: NetworkQualitiesPrefsManager::NetworkQualitiesPrefsManager( Is the NetworkQualitiesPrefsManager a Pref thread object? Was wondering about a race on ShutdownOnPrefThread if not. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:32: network_quality_estimator_->RemoveNetworkQualitiesCacheObserver(this); This seems unsafe, e.g., if called before InitializeOnNetworkThread(). network_quality_estimator_ is constructed with nullptr. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:41: nit: I'd remove the blank lines on 41 and 43. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:77: // Notify the pref delegate so that it updates the prefs on the disk. Do all pref delegates update to disk? I'd remove "on the disk" https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:36: // updates network quality prefs. Instances of this class must be constructed on network quality prefs -> network quality information that is stored in prefs https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:37: // the pref thread, and should later be moved to network thread by calling What is the "pref thread?" to -> to the https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:41: // propagates network quality pref changes from network thread to the provided from -> from the https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:44: // ShutdownOnPrefThread must be called from pref thread before destruction. from -> from the https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:55: // Create an instance of the NetworkQualitiesPrefsManager. Ownership of Create -> Creates https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:56: // |pref_delegate| is taken by this class. Must be constructed on the Pref is Pref capitalized? Here and elsewhere. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:62: // Initialize on Network thread. on -> on the https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:72: // ----------- I would change this comment to simply be: // Pref thread methods: Do this to "Network thread" and "Common" as well.
bengr: ptal. thanks. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h File net/nqe/network_id.h (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h#n... net/nqe/network_id.h:46: return id + kValueSeparator + On 2016/09/22 23:35:46, bengr wrote: > It seems like overill to define kValueSeparator. I would either do: > > return id + "," + ... > > or > > return StringPrintf("%s,%s", id.c_str(), ... The value separator is going to be used soon (in the next cl) in a different function when reading back the prefs and deserializing it. So, it is better to keep it in a common place, but I can remove it for now, if you feel strongly about it. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:19: NetworkQualitiesPrefsManager::NetworkQualitiesPrefsManager( On 2016/09/22 23:35:46, bengr wrote: > Is the NetworkQualitiesPrefsManager a Pref thread object? It will be created and destroyed by UI_NQE keyed service. > Was wondering about a > race on ShutdownOnPrefThread if not. It is created on pref thread (takes ownership of PrefDelegate), initialized on network thread (starts listening to NQE notifications), shutdown on pref thread (where it will delete PrefDelegate), and then destroyed on network thread (where it stops listening to notifications from NQE). This lifecycle is complex, but AFAICT there is no way to simplify it. That's because it is owned by keyed service (UI thread), so first step of construction happens on UI thread (then rest of the initialization happens on network). Similarly, shutdown first happens on UI thread, and the rest of it on network thread. ShutdownOnPrefThread is guaranteed to be called after pref_delegate has been set. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:32: network_quality_estimator_->RemoveNetworkQualitiesCacheObserver(this); On 2016/09/22 23:35:46, bengr wrote: > This seems unsafe, e.g., if called before InitializeOnNetworkThread(). > network_quality_estimator_ is constructed with nullptr. Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:41: On 2016/09/22 23:35:46, bengr wrote: > nit: I'd remove the blank lines on 41 and 43. Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:77: // Notify the pref delegate so that it updates the prefs on the disk. On 2016/09/22 23:35:46, bengr wrote: > Do all pref delegates update to disk? I'd remove "on the disk" Yes. They all update to disk. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:36: // updates network quality prefs. Instances of this class must be constructed on On 2016/09/22 23:35:46, bengr wrote: > network quality prefs -> network quality information that is stored in prefs Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:37: // the pref thread, and should later be moved to network thread by calling On 2016/09/22 23:35:46, bengr wrote: > What is the "pref thread?" Thread that is used to access the prefs (https://cs.chromium.org/search/?q=f:http_server_properties+pref.*thread&sq=pa...). I did not say "UI" because on cronet, these are accessed on the network task runner. > > to -> to the Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:41: // propagates network quality pref changes from network thread to the provided On 2016/09/22 23:35:46, bengr wrote: > from -> from the Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:44: // ShutdownOnPrefThread must be called from pref thread before destruction. On 2016/09/22 23:35:46, bengr wrote: > from -> from the Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:55: // Create an instance of the NetworkQualitiesPrefsManager. Ownership of On 2016/09/22 23:35:46, bengr wrote: > Create -> Creates Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:56: // |pref_delegate| is taken by this class. Must be constructed on the Pref On 2016/09/22 23:35:46, bengr wrote: > is Pref capitalized? Here and elsewhere. Not sure, made it lower case. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:62: // Initialize on Network thread. On 2016/09/22 23:35:46, bengr wrote: > on -> on the Done. https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.h:72: // ----------- On 2016/09/22 23:35:47, bengr wrote: > I would change this comment to simply be: > > // Pref thread methods: > > Do this to "Network thread" and "Common" as well. Done.
lgtm https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h File net/nqe/network_id.h (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h#n... net/nqe/network_id.h:46: return id + kValueSeparator + On 2016/09/23 17:10:32, tbansal1 wrote: > On 2016/09/22 23:35:46, bengr wrote: > > It seems like overill to define kValueSeparator. I would either do: > > > > return id + "," + ... > > > > or > > > > return StringPrintf("%s,%s", id.c_str(), ... > > The value separator is going to be used soon (in the next cl) in a different > function when reading back the prefs and deserializing it. So, it is better to > keep it in a common place, but I can remove it for now, if you feel strongly > about it. Nah. If you're gonna use it, keep it.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h File net/nqe/network_id.h (right): https://codereview.chromium.org/2322183002/diff/120001/net/nqe/network_id.h#n... net/nqe/network_id.h:46: return id + kValueSeparator + On 2016/09/23 20:31:06, bengr wrote: > On 2016/09/23 17:10:32, tbansal1 wrote: > > On 2016/09/22 23:35:46, bengr wrote: > > > It seems like overill to define kValueSeparator. I would either do: > > > > > > return id + "," + ... > > > > > > or > > > > > > return StringPrintf("%s,%s", id.c_str(), ... > > > > The value separator is going to be used soon (in the next cl) in a different > > function when reading back the prefs and deserializing it. So, it is better to > > keep it in a common place, but I can remove it for now, if you feel strongly > > about it. > > Nah. If you're gonna use it, keep it. Acknowledged.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kundaji@chromium.org, ryansturm@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2322183002/#ps180001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG=490870 ========== to ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG=490870 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG=490870 ========== to ========== Add Network Quality Estimator (NQE) pref manager The PrefManager class owns an IO Object (CacheObserver) and a Pref object (PrefDelegate). The embedder would need to implement PrefDelegate and create an instance of PrefManager for prefs to work. Right now, only writes to the prefs is implemented. Reads would come in the next CL, along with the Chromium implementation. BUG=490870 Committed: https://crrev.com/ec22cb6b67efd63c4b682b8964d690d401808b77 Cr-Commit-Position: refs/heads/master@{#420786} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ec22cb6b67efd63c4b682b8964d690d401808b77 Cr-Commit-Position: refs/heads/master@{#420786} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
