Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103323007:
Exposing NQE on the Browser UI thread (Closed)
DescriptionExposing NQE on the Browser UI thread
This involves an IO base observer which passes messages to the UI thread
using a KeyedService. Consumers should be able to consume this similarly
to the way the NQE is already consumed.
BUG=615563
Committed: https://crrev.com/a699b6ea04c4e2d1413babc597fb2e01bc0261ec
Cr-Commit-Position: refs/heads/master@{#406630}
Patch Set 1 #
Total comments: 16
Patch Set 2 : tbansal comments + threadChecker #Patch Set 3 : rebase #
Total comments: 22
Patch Set 4 : tbansal comments #
Total comments: 4
Patch Set 5 : tbansal nits #Patch Set 6 : tbansal comment #Patch Set 7 : removing weakPtr #
Total comments: 2
Patch Set 8 : changing OWNERS to map to net/nqe/OWNERS #
Total comments: 16
Patch Set 9 : rebase #Patch Set 10 : Addressed mmenke comments #Patch Set 11 : rebase #Patch Set 12 : Adding browsertest #Patch Set 13 : typo #
Total comments: 10
Patch Set 14 : nits #
Total comments: 31
Patch Set 15 : tbansal comments #
Total comments: 10
Patch Set 16 : mmenke comments #Messages
Total messages: 58 (31 generated)
tbansal@chromium.org changed reviewers: + tbansal@chromium.org
https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... File chrome/browser/net/nqe/io_network_quality_observer.cc (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.cc:24: DCHECK(io_thread->globals()->network_quality_estimator); Is it guaranteed that nqe will be initialized when this is called? https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... File chrome/browser/net/nqe/io_network_quality_observer.h (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.h:18: class IONetworkQualityObserver Would it be preferable to make this an inner class to UINetworkQualityObserver since it is never going to be used by anybody else. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.h:27: void InitializeOnIOThread(IOThread* io_thread); const IOThread* https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.h:37: base::WeakPtr<UINetworkQualityEstimatorService> service_; You do not really need a weak ptr to service_ because service_ owns |this|. So, it should be non-null for the lifetime of |this|. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:21: io_task_runner_->DeleteSoon(FROM_HERE, observer_); DCHECK the return value. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:37: IONetworkQualityObserver* observer_; may be call it io_observer_ https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:39: base::WeakPtrFactory<UINetworkQualityEstimatorService> weak_factory_; No need gorfor weak_factory_. Generally, it is preferable to keep weak ptrs to minimum. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc:14: UINetworkQualityEstimatorServiceFactory::GetForProfile(Profile* profile) { DCHECK that profile is not off-the-record by calling GetProfileType()
https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... File chrome/browser/net/nqe/io_network_quality_observer.cc (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.cc:24: DCHECK(io_thread->globals()->network_quality_estimator); On 2016/07/01 22:22:02, tbansal1 wrote: > Is it guaranteed that nqe will be initialized when this is called? Acknowledged. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... File chrome/browser/net/nqe/io_network_quality_observer.h (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.h:18: class IONetworkQualityObserver On 2016/07/01 22:22:02, tbansal1 wrote: > Would it be preferable to make this an inner class to UINetworkQualityObserver > since it is never going to be used by anybody else. Done. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.h:27: void InitializeOnIOThread(IOThread* io_thread); On 2016/07/01 22:22:02, tbansal1 wrote: > const IOThread* Done. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/io_n... chrome/browser/net/nqe/io_network_quality_observer.h:37: base::WeakPtr<UINetworkQualityEstimatorService> service_; On 2016/07/01 22:22:02, tbansal1 wrote: > You do not really need a weak ptr to service_ because service_ owns |this|. So, > it should be non-null for the lifetime of |this|. not strict ownership. The timing of posting from here while the delete is posted from the other thread could lead to issues. That is to say that this is not deleted synchronously in service_'s destructor. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:21: io_task_runner_->DeleteSoon(FROM_HERE, observer_); On 2016/07/01 22:22:02, tbansal1 wrote: > DCHECK the return value. Done. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:37: IONetworkQualityObserver* observer_; On 2016/07/01 22:22:03, tbansal1 wrote: > may be call it io_observer_ Done. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:39: base::WeakPtrFactory<UINetworkQualityEstimatorService> weak_factory_; On 2016/07/01 22:22:03, tbansal1 wrote: > No need gorfor weak_factory_. Generally, it is preferable to keep weak ptrs to > minimum. Acknowledged. https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc (right): https://codereview.chromium.org/2103323007/diff/1/chrome/browser/net/nqe/ui_n... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc:14: UINetworkQualityEstimatorServiceFactory::GetForProfile(Profile* profile) { On 2016/07/01 22:22:03, tbansal1 wrote: > DCHECK that profile is not off-the-record by calling GetProfileType() Done.
https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:15: class UINetworkQualityEstimatorService::IONetworkQualityObserver Add class comments. Something along the lines of: this class listens to changes in effective connection type. It is created on UI thread, but moved to IO thread. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:23: thread_checker_.DetachFromThread(); DCHECK(ui_task_runner_); https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:37: io_thread->globals()->network_quality_estimator->GetWeakPtr(); Just use the raw pointer. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:38: network_quality_estimator_->AddEffectiveConnectionTypeObserver(this); Check if network_quality_estimator_ is non-null before you call this. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:48: void OnEffectiveConnectionTypeChanged( Why protected? Also, add comment: // net::NetworkQualityEstimator::EffectiveConnectionTypeObserver implementation: https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:28: void Initialize(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, pass as const ref? https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:33: net::NetworkQualityEstimator::EffectiveConnectionType type() { return type_; } const function Also, I think you should move the body to .cc, and add thread checker. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:37: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; include: base/memory/ref_counted.h https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:38: class IONetworkQualityObserver; This should be the first line in the private. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.h (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.h:18: class UINetworkQualityEstimatorService; A linebreak after namespace.
https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:15: class UINetworkQualityEstimatorService::IONetworkQualityObserver On 2016/07/13 00:07:27, tbansal1 wrote: > Add class comments. Something along the lines of: this class listens to changes > in effective connection type. It is created on UI thread, but moved to IO > thread. Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:23: thread_checker_.DetachFromThread(); On 2016/07/13 00:07:28, tbansal1 wrote: > DCHECK(ui_task_runner_); Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:37: io_thread->globals()->network_quality_estimator->GetWeakPtr(); On 2016/07/13 00:07:28, tbansal1 wrote: > Just use the raw pointer. I'm not sure that the network_quality_estimator_->RemoveEffectiveConnectionTypeObserver above will be safe without a WeakPtr. If the IO process globals are deleted before the UI process KeyedServices, couldn't we get a use after free? https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:38: network_quality_estimator_->AddEffectiveConnectionTypeObserver(this); On 2016/07/13 00:07:28, tbansal1 wrote: > Check if network_quality_estimator_ is non-null before you call this. Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:48: void OnEffectiveConnectionTypeChanged( On 2016/07/13 00:07:28, tbansal1 wrote: > Why protected? > > Also, add comment: > // net::NetworkQualityEstimator::EffectiveConnectionTypeObserver implementation: Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:28: void Initialize(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner, On 2016/07/13 00:07:28, tbansal1 wrote: > pass as const ref? Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:33: net::NetworkQualityEstimator::EffectiveConnectionType type() { return type_; } On 2016/07/13 00:07:28, tbansal1 wrote: > const function > Also, I think you should move the body to .cc, and add thread checker. Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:37: scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_; On 2016/07/13 00:07:28, tbansal1 wrote: > include: base/memory/ref_counted.h Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:38: class IONetworkQualityObserver; On 2016/07/13 00:07:28, tbansal1 wrote: > This should be the first line in the private. Done. https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.h (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.h:18: class UINetworkQualityEstimatorService; On 2016/07/13 00:07:28, tbansal1 wrote: > A linebreak after namespace. Done.
Can you also add an OWNERS file to chrome/browser/net/nqe/, and point it to //net/nqe/OWNERS. Something like: https://cs.chromium.org/search/?q=f:OWNERS+%22file://%22+-%22per-file%22&sq=p... https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:37: io_thread->globals()->network_quality_estimator->GetWeakPtr(); On 2016/07/13 17:28:14, RyanSturm wrote: > On 2016/07/13 00:07:28, tbansal1 wrote: > > Just use the raw pointer. > > I'm not sure that the > network_quality_estimator_->RemoveEffectiveConnectionTypeObserver above will be > safe without a WeakPtr. If the IO process globals are deleted before the UI > process KeyedServices, couldn't we get a use after free? The IOThread::globals are destroyed in IOThread::Cleanup() which is called after the message loop ends. https://cs.chromium.org/chromium/src/base/threading/thread.h?rcl=1468408847&l... https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:27: // This is created on the UI thread, but used only on the UI thread. s/used only on the UI thread/used only on the IO thread/ https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc (right): https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc:17: DCHECK(profile->GetProfileType() != Profile::INCOGNITO_PROFILE); Use DCHECK_NE
https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:27: // This is created on the UI thread, but used only on the UI thread. On 2016/07/13 17:56:11, tbansal1 wrote: > s/used only on the UI thread/used only on the IO thread/ Done. https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc (right): https://codereview.chromium.org/2103323007/diff/60001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.cc:17: DCHECK(profile->GetProfileType() != Profile::INCOGNITO_PROFILE); On 2016/07/13 17:56:11, tbansal1 wrote: > Use DCHECK_NE Done.
https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:37: io_thread->globals()->network_quality_estimator->GetWeakPtr(); On 2016/07/13 17:56:11, tbansal1 wrote: > On 2016/07/13 17:28:14, RyanSturm wrote: > > On 2016/07/13 00:07:28, tbansal1 wrote: > > > Just use the raw pointer. > > > > I'm not sure that the > > network_quality_estimator_->RemoveEffectiveConnectionTypeObserver above will > be > > safe without a WeakPtr. If the IO process globals are deleted before the UI > > process KeyedServices, couldn't we get a use after free? > > The IOThread::globals are destroyed in IOThread::Cleanup() which is called > after the message loop ends. > https://cs.chromium.org/chromium/src/base/threading/thread.h?rcl=1468408847&l... Because of the above, you can remove the WeakPtr, and use the raw pointer from here and from NQE class. Generally, it is considered an anti-pattern to expose WeakPtrs broadly.
On 2016/07/13 18:08:13, tbansal1 wrote: > https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... > File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): > > https://codereview.chromium.org/2103323007/diff/40001/chrome/browser/net/nqe/... > chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:37: > io_thread->globals()->network_quality_estimator->GetWeakPtr(); > On 2016/07/13 17:56:11, tbansal1 wrote: > > On 2016/07/13 17:28:14, RyanSturm wrote: > > > On 2016/07/13 00:07:28, tbansal1 wrote: > > > > Just use the raw pointer. > > > > > > I'm not sure that the > > > network_quality_estimator_->RemoveEffectiveConnectionTypeObserver above will > > be > > > safe without a WeakPtr. If the IO process globals are deleted before the UI > > > process KeyedServices, couldn't we get a use after free? > > > > The IOThread::globals are destroyed in IOThread::Cleanup() which is called > > after the message loop ends. > > > https://cs.chromium.org/chromium/src/base/threading/thread.h?rcl=1468408847&l... > > Because of the above, you can remove the WeakPtr, and use the raw pointer from > here and > from NQE class. > Generally, it is considered an anti-pattern to expose WeakPtrs broadly. Missed that comment. Changed to a raw pointer.
lgtm % nit. Thanks for doing this. https://codereview.chromium.org/2103323007/diff/120001/chrome/browser/net/nqe... File chrome/browser/net/nqe/OWNERS (right): https://codereview.chromium.org/2103323007/diff/120001/chrome/browser/net/nqe... chrome/browser/net/nqe/OWNERS:1: bengr@chromium.org nit: Can you change this to file://net/nqe/OWNERS
https://codereview.chromium.org/2103323007/diff/120001/chrome/browser/net/nqe... File chrome/browser/net/nqe/OWNERS (right): https://codereview.chromium.org/2103323007/diff/120001/chrome/browser/net/nqe... chrome/browser/net/nqe/OWNERS:1: bengr@chromium.org On 2016/07/13 18:27:23, tbansal1 wrote: > nit: Can you change this to > file://net/nqe/OWNERS Done.
ryansturm@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: PTAL at *, thanks
https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:39: if (!io_thread->globals()->network_quality_estimator) Hrm...Is there any plan to use per-profile data instead? Wonder if this really gets anything from being a Profile keyed service (BrowserContextKeyedService? Whatever).. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:58: DCHECK(thread_checker_.CalledOnValidThread()); Seems much clearer to DCHECK against browser thread instead - as-is, it's a bit confusing what's run on what thread. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:30: // Called when the EffectiveConnectionType has updated to |type|. May want to mention NCN::GetConnectionType, any how this is different, so people don't have to dig deeper to figure out what this actually does. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:32: net::NetworkQualityEstimator::EffectiveConnectionType type); methods that are only called by inner classes (IONetworkQualityObserver) should be private. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:34: // Creates |io_observer_|, and posts a task to initialize it. See comment about getting rid of this. More generally, io_observer_ isn't really a part of the public API, so probably best not to mention it in documentation of public methods. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:40: net::NetworkQualityEstimator::EffectiveConnectionType type() const; Since this isn't inlined, GetType, maybe? https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:211: BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); Any reason not to just register the factory in chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc, like most of the others? It ensures the service will be created for each profile, I believe. Also, rather than passing in the threads directly, UINetworkQualityEstimatorService can depend on BrowserThread itself (Use TestBrowserThreadBundle for tests).
One other thing...Speaking of tests...Have you thought about testing this class?
The CQ bit was checked by ryansturm@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 checked by ryansturm@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...
I can't think of a good way to test this. At best, I could create the two threads and call to update the effective type, but it seems like testing getters and setters. If that's something that sounds like it's reasonable, I can do it; I just don't see a huge amount of value. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:39: if (!io_thread->globals()->network_quality_estimator) On 2016/07/14 19:10:12, mmenke wrote: > Hrm...Is there any plan to use per-profile data instead? Wonder if this really > gets anything from being a Profile keyed service (BrowserContextKeyedService? > Whatever).. Because network prefs will be stored per profile, an estimate of the initial EffectiveConnectionType would be available synchronously during start up for some profiles and not others. This matches the decision to use per-profile prefs, which was made to have clear ownership of this information in a profile. If a user wants to clear their cache, it will clear the prefs for that user, for example. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:58: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/07/14 19:10:12, mmenke wrote: > Seems much clearer to DCHECK against browser thread instead - as-is, it's a bit > confusing what's run on what thread. Done. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:30: // Called when the EffectiveConnectionType has updated to |type|. On 2016/07/14 19:10:12, mmenke wrote: > May want to mention NCN::GetConnectionType, any how this is different, so people > don't have to dig deeper to figure out what this actually does. Done. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:32: net::NetworkQualityEstimator::EffectiveConnectionType type); On 2016/07/14 19:10:12, mmenke wrote: > methods that are only called by inner classes (IONetworkQualityObserver) should > be private. Done. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:34: // Creates |io_observer_|, and posts a task to initialize it. On 2016/07/14 19:10:12, mmenke wrote: > See comment about getting rid of this. More generally, io_observer_ isn't > really a part of the public API, so probably best not to mention it in > documentation of public methods. Done. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:40: net::NetworkQualityEstimator::EffectiveConnectionType type() const; On 2016/07/14 19:10:12, mmenke wrote: > Since this isn't inlined, GetType, maybe? Done. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:211: BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); On 2016/07/14 19:10:12, mmenke wrote: > Any reason not to just register the factory in > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc, like most > of the others? It ensures the service will be created for each profile, I > believe. > > Also, rather than passing in the threads directly, > UINetworkQualityEstimatorService can depend on BrowserThread itself (Use > TestBrowserThreadBundle for tests). I can register the factory in chrome_browser_main_extra_parts_profiles, but I also need to create an instance for the profile somewhere to start up the service earlier than the first lazy consumer. I've removed Initialize call altogether since I won't be passing in TaskRunners. Let me know if you think there is a better place to create the first instance of the UINetworkQualityEstimatorService.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/07/14 21:42:12, RyanSturm wrote: > I can't think of a good way to test this. At best, I could create the two > threads and call to update the effective type, but it seems like testing getters > and setters. If that's something that sounds like it's reasonable, I can do it; > I just don't see a huge amount of value. This seems concerning - this is a simple class, but a broken NQE/UINQEService seems like something that could end up being unnoticed for quite a while. Seems like this also means we can't make good browser tests for any UINQES-consuming objects. Two options: * We could mock out the profile's NQE, or the network status reported by it. This gets a bit ugly, since we can't swap out the main NQE at runtime, unfortunately, particularly if we don't want to touch production code. This is a general problem with mocking out network stack components. * Get a real NQE result. We could either make a mock URLRequest that claims to be really fast, or just call into the NQE ourselves. Since the NQE only counts real network requests, we should be able to make the network appear really fast or really slow this way, as long as we aren't using an embedded test server (Which would add "real" network results into the mix). This doesn't seem too bad, does it? So we just set up a consumer of the class, and make sure it sees the expected results. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:39: if (!io_thread->globals()->network_quality_estimator) On 2016/07/14 21:42:12, RyanSturm wrote: > On 2016/07/14 19:10:12, mmenke wrote: > > Hrm...Is there any plan to use per-profile data instead? Wonder if this > really > > gets anything from being a Profile keyed service (BrowserContextKeyedService? > > Whatever).. > > Because network prefs will be stored per profile, an estimate of the initial > EffectiveConnectionType would be available synchronously during start up for > some profiles and not others. This matches the decision to use per-profile > prefs, which was made to have clear ownership of this information in a profile. > If a user wants to clear their cache, it will clear the prefs for that user, for > example. ok, as long as there's a reason, SGTM. https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:211: BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); On 2016/07/14 21:42:12, RyanSturm wrote: > On 2016/07/14 19:10:12, mmenke wrote: > > Any reason not to just register the factory in > > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc, like most > > of the others? It ensures the service will be created for each profile, I > > believe. > > > > Also, rather than passing in the threads directly, > > UINetworkQualityEstimatorService can depend on BrowserThread itself (Use > > TestBrowserThreadBundle for tests). > > I can register the factory in chrome_browser_main_extra_parts_profiles, but I > also need to create an instance for the profile somewhere to start up the > service earlier than the first lazy consumer. I've removed Initialize call > altogether since I won't be passing in TaskRunners. Let me know if you think > there is a better place to create the first instance of the > UINetworkQualityEstimatorService. The first lazy consumer is probably going to be created along with the profile, no? Doesn't seem like you gain a whole lot by creating just a bit before that, on the same thread.
The CQ bit was checked by ryansturm@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...
tbansal, mmenke: PTAL On 2016/07/18 21:34:18, mmenke wrote: > On 2016/07/14 21:42:12, RyanSturm wrote: > > I can't think of a good way to test this. At best, I could create the two > > threads and call to update the effective type, but it seems like testing > getters > > and setters. If that's something that sounds like it's reasonable, I can do > it; > > I just don't see a huge amount of value. > > This seems concerning - this is a simple class, but a broken NQE/UINQEService > seems like something that could end up being unnoticed for quite a while. Seems > like this also means we can't make good browser tests for any UINQES-consuming > objects. > > Two options: > * We could mock out the profile's NQE, or the network status reported by it. > This gets a bit ugly, since we can't swap out the main NQE at runtime, > unfortunately, particularly if we don't want to touch production code. This is > a general problem with mocking out network stack components. > > * Get a real NQE result. We could either make a mock URLRequest that claims to > be really fast, or just call into the NQE ourselves. Since the NQE only counts > real network requests, we should be able to make the network appear really fast > or really slow this way, as long as we aren't using an embedded test server > (Which would add "real" network results into the mix). This doesn't seem too > bad, does it? So we just set up a consumer of the class, and make sure it sees > the expected results. > I went with a browsertest approach that changes the NQE code to allow an override. I introduced a util function that other browsertest could consume to change the UI NQE reported value. > https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... > File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): > > https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/net/nqe... > chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:39: if > (!io_thread->globals()->network_quality_estimator) > On 2016/07/14 21:42:12, RyanSturm wrote: > > On 2016/07/14 19:10:12, mmenke wrote: > > > Hrm...Is there any plan to use per-profile data instead? Wonder if this > > really > > > gets anything from being a Profile keyed service > (BrowserContextKeyedService? > > > Whatever).. > > > > Because network prefs will be stored per profile, an estimate of the initial > > EffectiveConnectionType would be available synchronously during start up for > > some profiles and not others. This matches the decision to use per-profile > > prefs, which was made to have clear ownership of this information in a > profile. > > If a user wants to clear their cache, it will clear the prefs for that user, > for > > example. > > ok, as long as there's a reason, SGTM. > > https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > https://codereview.chromium.org/2103323007/diff/140001/chrome/browser/profile... > chrome/browser/profiles/profile_impl_io_data.cc:211: > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); > On 2016/07/14 21:42:12, RyanSturm wrote: > > On 2016/07/14 19:10:12, mmenke wrote: > > > Any reason not to just register the factory in > > > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc, like > most > > > of the others? It ensures the service will be created for each profile, I > > > believe. > > > > > > Also, rather than passing in the threads directly, > > > UINetworkQualityEstimatorService can depend on BrowserThread itself (Use > > > TestBrowserThreadBundle for tests). > > > > I can register the factory in chrome_browser_main_extra_parts_profiles, but I > > also need to create an instance for the profile somewhere to start up the > > service earlier than the first lazy consumer. I've removed Initialize call > > altogether since I won't be passing in TaskRunners. Let me know if you think > > there is a better place to create the first instance of the > > UINetworkQualityEstimatorService. > > The first lazy consumer is probably going to be created along with the profile, > no? Doesn't seem like you gain a whole lot by creating just a bit before that, > on the same thread. Done.
The CQ bit was checked by ryansturm@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.
Thanks for writing the tests. still lgtm. https://codereview.chromium.org/2103323007/diff/240001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h (right): https://codereview.chromium.org/2103323007/diff/240001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:15: // |type| to all observers, including the UINetworkQualityEstimatorService. s/observers/effective connection type observers/ https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:743: stop_reporting_effective_connection_type_ = Add thread checker. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:749: FOR_EACH_OBSERVER( Add thread checker. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:243: bool stop_reporting_effective_connection_type); I would suggest not including any param in the function, and then setting |stop_sending.._| to true in the function body. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:656: // from calls to ReportEffectiveConnectionTypeForTesting. s/to/from/
The CQ bit was checked by ryansturm@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.
https://codereview.chromium.org/2103323007/diff/240001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h (right): https://codereview.chromium.org/2103323007/diff/240001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:15: // |type| to all observers, including the UINetworkQualityEstimatorService. On 2016/07/19 23:05:22, tbansal1 wrote: > s/observers/effective connection type observers/ Done. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:743: stop_reporting_effective_connection_type_ = On 2016/07/19 23:05:22, tbansal1 wrote: > Add thread checker. Done. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:749: FOR_EACH_OBSERVER( On 2016/07/19 23:05:22, tbansal1 wrote: > Add thread checker. Done. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:243: bool stop_reporting_effective_connection_type); On 2016/07/19 23:05:22, tbansal1 wrote: > I would suggest not including any param in the function, and then setting > |stop_sending.._| to true in the function body. Done. https://codereview.chromium.org/2103323007/diff/240001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:656: // from calls to ReportEffectiveConnectionTypeForTesting. On 2016/07/19 23:05:22, tbansal1 wrote: > s/to/from/ Done.
The CQ bit was checked by ryansturm@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.
Looked a bit more carefully. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:8: #include <memory> include probably not needed. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:11: #include "base/memory/ref_counted.h" include probably not needed. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:27: void Shutdown() override; This can be private method. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:29: // The current EffectiveConnectionType. Rename to GetEffectiveConnectionType to match the API name on the IO thread. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:48: base::ThreadChecker thread_checker_; #include thread checker. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:19: IN_PROC_BROWSER_TEST_F(UINetworkQualityEstimatorServiceBrowserTest, #include content/public/test/browser_test.h for the macro. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:24: Profile* profile = ProfileManager::GetActiveUserProfile(); Forward declare Profile. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.cc:42: content::BrowserThread::PostTaskAndReply( #include base/task_runner.h for PostTaskAndReply. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:14: // Prevents NQE from reporting EffectiveConnectionType to observers, and reports s/NQE/NetworkQualityEstimator/ s/and reports/and forces it to report/ https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:15: // |type| to all EffectiveConnectionTypeObservers, including the s/all/all its/ https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:16: // UINetworkQualityEstimatorService. I do not think you should say anything about UINetworkQualityEstimatorService because there is no guarantee that UINetworkQualityEstimatorService will be registered as an observer (that requires creating KeyedService which this function does not do). https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:17: // This blocks the UI thread until the IO task runs and replies. s/UI thread/calling thread/ https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:197: chrome_browser_net::UINetworkQualityEstimatorServiceFactory::GetInstance(); I forgot, would this be called on non-incognito profiles? If yes, you may want to return null in the factory if the profile is incognito: https://cs.chromium.org/chromium/src/chrome/browser/android/data_usage/data_u... https://codereview.chromium.org/2103323007/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2103323007/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:242: void StopReportingEffectiveConnectionTypeForTesting(); I do not think that this method is necessary. ReportEffectiveConnectionTypeForTesting would force the reporting of ECT, and that would be the last thing that happens on IO thread, followed by the test checking the value of ECT on the UI thread (the task runners are all sequential). This method would probably be required later (e.g., for testings prefs, or offline integration), but if it is not required in this CL, it is better to not include it.
The CQ bit was checked by ryansturm@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...
tbansal: PTAL, thanks https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:8: #include <memory> On 2016/07/20 03:32:35, tbansal1 wrote: > include probably not needed. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:11: #include "base/memory/ref_counted.h" On 2016/07/20 03:32:35, tbansal1 wrote: > include probably not needed. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:27: void Shutdown() override; On 2016/07/20 03:32:35, tbansal1 wrote: > This can be private method. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:29: // The current EffectiveConnectionType. On 2016/07/20 03:32:35, tbansal1 wrote: > Rename to GetEffectiveConnectionType to match the API name on the IO thread. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:48: base::ThreadChecker thread_checker_; On 2016/07/20 03:32:35, tbansal1 wrote: > #include thread checker. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:19: IN_PROC_BROWSER_TEST_F(UINetworkQualityEstimatorServiceBrowserTest, On 2016/07/20 03:32:35, tbansal1 wrote: > #include content/public/test/browser_test.h for the macro. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:24: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2016/07/20 03:32:35, tbansal1 wrote: > Forward declare Profile. Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.cc:42: content::BrowserThread::PostTaskAndReply( On 2016/07/20 03:32:35, tbansal1 wrote: > #include base/task_runner.h for PostTaskAndReply. This is a different PostTaskAndReply in browser_thread.h https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?... https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:14: // Prevents NQE from reporting EffectiveConnectionType to observers, and reports On 2016/07/20 03:32:36, tbansal1 wrote: > s/NQE/NetworkQualityEstimator/ > s/and reports/and forces it to report/ Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:15: // |type| to all EffectiveConnectionTypeObservers, including the On 2016/07/20 03:32:35, tbansal1 wrote: > s/all/all its/ Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:16: // UINetworkQualityEstimatorService. On 2016/07/20 03:32:36, tbansal1 wrote: > I do not think you should say anything about UINetworkQualityEstimatorService > because there is no guarantee that UINetworkQualityEstimatorService will be > registered as an observer (that requires creating KeyedService which this > function does not do). Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.h:17: // This blocks the UI thread until the IO task runs and replies. On 2016/07/20 03:32:36, tbansal1 wrote: > s/UI thread/calling thread/ Done. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:197: chrome_browser_net::UINetworkQualityEstimatorServiceFactory::GetInstance(); On 2016/07/20 03:32:36, tbansal1 wrote: > I forgot, would this be called on non-incognito profiles? If yes, you may want > to return null in the factory if the profile is incognito: > https://cs.chromium.org/chromium/src/chrome/browser/android/data_usage/data_u... This just sets up the factory. There's a DCHECK in the factory for GetForProfile, so that seems sufficient to verify that incognito profiles do not have the actual service. https://codereview.chromium.org/2103323007/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2103323007/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:242: void StopReportingEffectiveConnectionTypeForTesting(); On 2016/07/20 03:32:36, tbansal1 wrote: > I do not think that this method is necessary. > ReportEffectiveConnectionTypeForTesting would force the reporting of ECT, and > that would be the last thing that happens on IO thread, followed by the test > checking the value of ECT on the UI thread (the task runners are all > sequential). > > This method would probably be required later (e.g., for testings prefs, or > offline integration), but if it is not required in this CL, it is better to not > include it. This will definitely be required later; I'll leave implmenting it to whoever ends up needing it first.
LGTM https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:69: UINetworkQualityEstimatorService::UINetworkQualityEstimatorService( Are you thinking this class won't need to support its own observers, or thinking you'll add them later? I'm fine, either way, just curious. https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:74: DCHECK(thread_checker_.CalledOnValidThread()); optional: Rather than a thread checks, suggest just using DCHECK_CURRENTLY_ON(content::BrowserThread::UI); It's a bit more explicit. https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:16: namespace chrome_browser_net { The currently preferred style is not to use the chrome_browser_net namespace (Current usage is inconsistent, but was a thread about this a while back that arrived at a consensus not to use namespaces for subdirectories of chrome) https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:16: class UINetworkQualityEstimatorServiceBrowserTest I'd suggest putting all this in an anonymous namespace, particularly the test fixture (Admittedly, not something we're likely to have name conflicts for, anyways) https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:20: }; could just replace this with "using UINetworkQualityEstimatorServiceBrowserTest = InProcessBrowserTest;" It's pretty much the same thing, but...erm... C++11-ier, and makes clearer this class isn't doing anything different.
lgtm. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_test_util.cc:42: content::BrowserThread::PostTaskAndReply( On 2016/07/20 16:36:46, RyanSturm wrote: > On 2016/07/20 03:32:35, tbansal1 wrote: > > #include base/task_runner.h for PostTaskAndReply. > > This is a different PostTaskAndReply in browser_thread.h > > https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?... Acknowledged. https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/profile... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/2103323007/diff/260001/chrome/browser/profile... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:197: chrome_browser_net::UINetworkQualityEstimatorServiceFactory::GetInstance(); On 2016/07/20 16:36:46, RyanSturm wrote: > On 2016/07/20 03:32:36, tbansal1 wrote: > > I forgot, would this be called on non-incognito profiles? If yes, you may want > > to return null in the factory if the profile is incognito: > > > https://cs.chromium.org/chromium/src/chrome/browser/android/data_usage/data_u... > > This just sets up the factory. There's a DCHECK in the factory for > GetForProfile, so that seems sufficient to verify that incognito profiles do not > have the actual service. Acknowledged. https://codereview.chromium.org/2103323007/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2103323007/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:242: void StopReportingEffectiveConnectionTypeForTesting(); On 2016/07/20 16:36:46, RyanSturm wrote: > On 2016/07/20 03:32:36, tbansal1 wrote: > > I do not think that this method is necessary. > > ReportEffectiveConnectionTypeForTesting would force the reporting of ECT, and > > that would be the last thing that happens on IO thread, followed by the test > > checking the value of ECT on the UI thread (the task runners are all > > sequential). > > > > This method would probably be required later (e.g., for testings prefs, or > > offline integration), but if it is not required in this CL, it is better to > not > > include it. > > This will definitely be required later; I'll leave implmenting it to whoever > ends up needing it first. Acknowledged.
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:69: UINetworkQualityEstimatorService::UINetworkQualityEstimatorService( On 2016/07/20 17:11:33, mmenke wrote: > Are you thinking this class won't need to support its own observers, or thinking > you'll add them later? I'm fine, either way, just curious. The current plan is to add them later. https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:74: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/07/20 17:11:33, mmenke wrote: > optional: Rather than a thread checks, suggest just using > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > > It's a bit more explicit. sgtm. https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:16: namespace chrome_browser_net { On 2016/07/20 17:11:33, mmenke wrote: > The currently preferred style is not to use the chrome_browser_net namespace > (Current usage is inconsistent, but was a thread about this a while back that > arrived at a consensus not to use namespaces for subdirectories of chrome) Done. https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:16: class UINetworkQualityEstimatorServiceBrowserTest On 2016/07/20 17:11:33, mmenke wrote: > I'd suggest putting all this in an anonymous namespace, particularly the test > fixture (Admittedly, not something we're likely to have name conflicts for, > anyways) Done. https://codereview.chromium.org/2103323007/diff/280001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:20: }; On 2016/07/20 17:11:33, mmenke wrote: > could just replace this with "using UINetworkQualityEstimatorServiceBrowserTest > = InProcessBrowserTest;" It's pretty much the same thing, but...erm... > C++11-ier, and makes clearer this class isn't doing anything different. 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.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2103323007/#ps300001 (title: "mmenke comments")
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.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Exposing NQE on the Browser UI thread This involves an IO base observer which passes messages to the UI thread using a KeyedService. Consumers should be able to consume this similarly to the way the NQE is already consumed. BUG=615563 ========== to ========== Exposing NQE on the Browser UI thread This involves an IO base observer which passes messages to the UI thread using a KeyedService. Consumers should be able to consume this similarly to the way the NQE is already consumed. BUG=615563 Committed: https://crrev.com/a699b6ea04c4e2d1413babc597fb2e01bc0261ec Cr-Commit-Position: refs/heads/master@{#406630} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a699b6ea04c4e2d1413babc597fb2e01bc0261ec Cr-Commit-Position: refs/heads/master@{#406630} |
