Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347783003:
[OfflinePages, NetworkQualityEstimator] Use NetworkQualityEstimator to decide on triggering Backgro… (Closed)
|
Created:
4 years, 3 months ago by dougarnett Modified:
4 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OfflinePages, NetworkQualityEstimator] Use NetworkQualityEstimator to decide on triggering Background Loading immediately.
Defines a NetworkQualityEstimator interface, plumbs it into Offline Pages RequestCoordinator, and uses it in logic to determine immediate StartProcessing.
BUG=647724
Committed: https://crrev.com/2e385c12e9e4f2b5969bb18ef63ebd66df8c0283
Cr-Commit-Position: refs/heads/master@{#419572}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addresses some feedback #
Total comments: 24
Patch Set 3 : Addressed feedback #
Messages
Total messages: 29 (16 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + petewil@chromium.org, ryansturm@chromium.org, tbansal@chromium.org
Tarun and Ryan, this defines a NetworkQualityProvider interface so that I can pass an interface over the UINetworkQualityEstimatorService into a component (RequestCoordinator). Please advise on naming, declaration location, etc. This CL still needs some unit test additions but want to make some review progress on the approach first.
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...)
Seems like adding the interface and plumbing it down is the right thing to do. I'm slightly worried about the KeyedServicesFactories dependencies on each other, but I don't know enough about KeyedServices to say if there is a problem. https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:68: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile)); This returns a raw pointer and doesn't expect ownership to be taken here. I haven't looked through the lifetime of the RequestCoordinator, does it outlive profile? Normally passing this as a raw pointer should be fine, but certain lifetimes could outlive the KeyedService. https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:392: if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) Normally, we use Slow 2g instead of just 2g, I'm not sure of the exact requirements for this though. Check if quality >= offline: quality >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE && quality <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G; https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off... https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... net/nqe/network_quality_estimator.h:124: class NET_EXPORT NetworkQualityProvider { NetworkQualityEstimator has the same signature for GetEffectiveconnectionType, so we might want to use this interface for that as well (we could make a core set of methods that consumers outside of net/ might want to use). Tbansal can think about that. If we do want to use the interface for NQE, then it might have to move to the top of the file instead.
https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:68: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile)); On 2016/09/16 18:54:46, Ryan Sturm wrote: > This returns a raw pointer and doesn't expect ownership to be taken here. I > haven't looked through the lifetime of the RequestCoordinator, does it outlive > profile? Normally passing this as a raw pointer should be fine, but certain > lifetimes could outlive the KeyedService. There is a KeyedService method, Shutdown that could invalidate the raw pointer that needs to be passed in before destruction. https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...
https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:66: std::unique_ptr<net::NetworkQualityEstimator::NetworkQualityProvider> The caller can't take ownership of the raw ptr since there is only one instance of UINetworkQualityEstimatorService per profile (and there may be other consumers). generally, the convention in Chromium is that a method returns a unique_ptr if it expects caller to take ownership of it (a raw pointer, otherwise). https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:385: if (connection == net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE) GetEffectiveConnection type will return E_C_T_OFFLINE if the device is actually offline. So, you do not need to check net::NetworkChangeNotifier separately. https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:392: if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) On 2016/09/16 18:54:46, Ryan Sturm wrote: > Normally, we use Slow 2g instead of just 2g, I'm not sure of the exact > requirements for this though. Check if quality >= offline: > > quality >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE > && > quality <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G; > > https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off... Currently, the method returns if ECT enum is UNKNOWN which I think is WAI. https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... net/nqe/network_quality_estimator.h:124: class NET_EXPORT NetworkQualityProvider { On 2016/09/16 18:54:46, Ryan Sturm wrote: > If we do want to use the interface for NQE, then it might have > to move to the top of the file instead. Lets do that later, not now. If we do it, then ideally it should be in //net/base. Then //net/nqe is just one of the implementors.
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:66: std::unique_ptr<net::NetworkQualityEstimator::NetworkQualityProvider> On 2016/09/16 19:13:50, tbansal1 wrote: > The caller can't take ownership of the raw ptr since there is only one instance > of UINetworkQualityEstimatorService per profile (and there may be other > consumers). > > generally, the convention in Chromium is that a method returns a unique_ptr if > it expects caller to take ownership of it (a raw pointer, otherwise). thanks https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:68: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile)); On 2016/09/16 19:08:26, Ryan Sturm wrote: > On 2016/09/16 18:54:46, Ryan Sturm wrote: > > This returns a raw pointer and doesn't expect ownership to be taken here. I > > haven't looked through the lifetime of the RequestCoordinator, does it outlive > > profile? Normally passing this as a raw pointer should be fine, but certain > > lifetimes could outlive the KeyedService. > > There is a KeyedService method, Shutdown that could invalidate the raw pointer > that needs to be passed in before destruction. > > https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv... > I'm not clear if you mean just invalidate in RequestCoordinator::Shutdown (updated for that) or try to invalidate pointer in RequestCoordinator from UINetworkQualityEstimatorService::Shutdown. https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:385: if (connection == net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE) On 2016/09/16 19:13:50, tbansal1 wrote: > GetEffectiveConnection type will return E_C_T_OFFLINE if the device is actually > offline. So, you do not need to check net::NetworkChangeNotifier separately. Done. https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:392: if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) On 2016/09/16 18:54:46, Ryan Sturm wrote: > Normally, we use Slow 2g instead of just 2g, I'm not sure of the exact > requirements for this though. Check if quality >= offline: > > quality >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE > && > quality <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G; > > https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off... So this linked code is for deciding to navigate to offline page or not if the network IsNetworkProhibitivelySlow, right? UNKNOWN does not indicate that it is slow. This new case is trying to decide whether to kick off a background load or not if the network is known to be recently good. So I think for this case UNKNOWN should not be included as known good condition. https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... net/nqe/network_quality_estimator.h:124: class NET_EXPORT NetworkQualityProvider { On 2016/09/16 19:13:50, tbansal1 wrote: > On 2016/09/16 18:54:46, Ryan Sturm wrote: > > If we do want to use the interface for NQE, then it might have > > to move to the top of the file instead. > > Lets do that later, not now. If we do it, then ideally it should be in > //net/base. Then //net/nqe is just one of the implementors. +1 I could add a TODO if you like.
lgtm chrome/browser/net/nqe/ and net/nqe % nits https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:69: static_cast<net::NetworkQualityEstimator::NetworkQualityProvider*>( Curious: Is there a reason to do explicit static_cast? https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:11: #include "net/nqe/network_quality_estimator.h" nit: Remove this include since it is already in the .h file. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:386: if (quality >= net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) { nit: I kind of liked it the old way if(quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) return; which should lead to fewer indentations, but it is your call. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:131: class NetworkQualityEstimatorStub nit: may be s/NetworkQualityEstimatorStub/TestNetworkQualityEstimator/ https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:320: nqe_(new NetworkQualityEstimatorStub()), Is this not leaking memory? May be you can store it as a unique_ptr in NetworkQualityEstimatorStub so that it gets deleted automatically.
https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:68: net::NetworkQualityEstimator::NetworkQualityProvider* nqe = nqe -> network_quality_estimator or network_quality_provider https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:386: if (quality >= net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) { This excludes EFFECTIVE_CONNECTION_TYPE_SLOW_2G. Do we think it should be included or excluded? https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:68: net::NetworkQualityEstimator::NetworkQualityProvider* nqe); nqe -> network_quality_estimator or network_quality_provider (everywhere, please) https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:387: SetEffectiveConnectionTypeForTesting( Maybe set up a default type in the ::SetUp function to OFFLINE, and only override it in the tests that need a different setting. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:1011: net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_OFFLINE); Maybe EFFECTIVE_CONNECTION_TYPE_SLOW_2G (unless we decide that 2G slow should still be started).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % making GetEffectiveConnectionType a const method https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:68: UINetworkQualityEstimatorServiceFactory::GetForProfile(profile)); On 2016/09/16 21:24:42, dougarnett wrote: > On 2016/09/16 19:08:26, Ryan Sturm wrote: > > On 2016/09/16 18:54:46, Ryan Sturm wrote: > > > This returns a raw pointer and doesn't expect ownership to be taken here. I > > > haven't looked through the lifetime of the RequestCoordinator, does it > outlive > > > profile? Normally passing this as a raw pointer should be fine, but certain > > > lifetimes could outlive the KeyedService. > > > > There is a KeyedService method, Shutdown that could invalidate the raw pointer > > that needs to be passed in before destruction. > > > > > https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv... > > > > I'm not clear if you mean just invalidate in RequestCoordinator::Shutdown > (updated for that) or try to invalidate pointer in RequestCoordinator from > UINetworkQualityEstimatorService::Shutdown. Perfect. https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:392: if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) On 2016/09/16 21:24:42, dougarnett wrote: > On 2016/09/16 18:54:46, Ryan Sturm wrote: > > Normally, we use Slow 2g instead of just 2g, I'm not sure of the exact > > requirements for this though. Check if quality >= offline: > > > > quality >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE > > && > > quality <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G; > > > > > https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off... > > So this linked code is for deciding to navigate to offline page or not if the > network IsNetworkProhibitivelySlow, right? UNKNOWN does not indicate that it is > slow. > > This new case is trying to decide whether to kick off a background load or not > if the network is known to be recently good. So I think for this case UNKNOWN > should not be included as known good condition. > Totally misread this chunk of code. Seems like the right comparison, feel free to go back to the < and early return like tbansal@ suggested in another comment. https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2347783003/diff/1/net/nqe/network_quality_est... net/nqe/network_quality_estimator.h:124: class NET_EXPORT NetworkQualityProvider { On 2016/09/16 21:24:43, dougarnett wrote: > On 2016/09/16 19:13:50, tbansal1 wrote: > > On 2016/09/16 18:54:46, Ryan Sturm wrote: > > > If we do want to use the interface for NQE, then it might have > > > to move to the top of the file instead. > > > > Lets do that later, not now. If we do it, then ideally it should be in > > //net/base. Then //net/nqe is just one of the implementors. > > +1 I could add a TODO if you like. Don't worry about adding the TODO. We're not sure yet, and we'll talk about implementing and expanding this interface to get more functional parity on IO/UI. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:130: // TODO(dougarnett): Replace with real NQE instance when/if support interface. For the purposes of this test, you'd probably be better off not using the real NQE since you just want to test the limited interface, so this TODO isn't necessary IMO. https://codereview.chromium.org/2347783003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2347783003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:127: virtual EffectiveConnectionType GetEffectiveConnectionType() = 0; Can this be a const function?
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:68: net::NetworkQualityEstimator::NetworkQualityProvider* nqe = On 2016/09/16 22:26:01, Pete Williamson wrote: > nqe -> network_quality_estimator or network_quality_provider Done. https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:69: static_cast<net::NetworkQualityEstimator::NetworkQualityProvider*>( On 2016/09/16 22:05:55, tbansal1 wrote: > Curious: Is there a reason to do explicit static_cast? Done. https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2347783003/diff/20001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:11: #include "net/nqe/network_quality_estimator.h" On 2016/09/16 22:05:55, tbansal1 wrote: > nit: Remove this include since it is already in the .h file. Done. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:386: if (quality >= net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) { On 2016/09/16 22:26:01, Pete Williamson wrote: > This excludes EFFECTIVE_CONNECTION_TYPE_SLOW_2G. Do we think it should be > included or excluded? Key question. Yes, as a starting point I am thinking we want poor connection excluded (and non-poor 2G included) - otherwise, we might not bother here with the NQE after all. We might want some more user intent understood from the UX to know we should make every effort now (vs. user just queuing up a set of pages for later while the user limps along in the foreground). https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:386: if (quality >= net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) { On 2016/09/16 22:05:55, tbansal1 wrote: > nit: I kind of liked it the old way > if(quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) > return; > which should lead to fewer indentations, but it is your call. Yeah, this was due to taking out the basic network filter and not wanting drop through without it. Now trying it back in for else case of no NQE. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:68: net::NetworkQualityEstimator::NetworkQualityProvider* nqe); On 2016/09/16 22:26:01, Pete Williamson wrote: > nqe -> network_quality_estimator or network_quality_provider (everywhere, > please) Done. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:130: // TODO(dougarnett): Replace with real NQE instance when/if support interface. On 2016/09/19 17:15:40, Ryan Sturm wrote: > For the purposes of this test, you'd probably be better off not using the real > NQE since you just want to test the limited interface, so this TODO isn't > necessary IMO. Done. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:131: class NetworkQualityEstimatorStub On 2016/09/16 22:05:56, tbansal1 wrote: > nit: may be s/NetworkQualityEstimatorStub/TestNetworkQualityEstimator/ Acknowledged. I prefer that too but matching naming style that already exists in this test class. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:320: nqe_(new NetworkQualityEstimatorStub()), On 2016/09/16 22:05:56, tbansal1 wrote: > Is this not leaking memory? May be you can store it as a unique_ptr in > NetworkQualityEstimatorStub so that it gets deleted automatically. Done. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:387: SetEffectiveConnectionTypeForTesting( On 2016/09/16 22:26:01, Pete Williamson wrote: > Maybe set up a default type in the ::SetUp function to OFFLINE, and only > override it in the tests that need a different setting. Done. https://codereview.chromium.org/2347783003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:1011: net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_OFFLINE); On 2016/09/16 22:26:01, Pete Williamson wrote: > Maybe EFFECTIVE_CONNECTION_TYPE_SLOW_2G (unless we decide that 2G slow should > still be started). Added ...WhenPoorlyConnected test variant. https://codereview.chromium.org/2347783003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2347783003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:127: virtual EffectiveConnectionType GetEffectiveConnectionType() = 0; On 2016/09/19 17:15:40, Ryan Sturm wrote: > Can this be a const function? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2347783003/#ps40001 (title: "Addressed feedback")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages, NetworkQualityEstimator] Use NetworkQualityEstimator to decide on triggering Background Loading immediately. Defines a NetworkQualityEstimator interface, plumbs it into Offline Pages RequestCoordinator, and uses it in logic to determine immediate StartProcessing. BUG=647724 ========== to ========== [OfflinePages, NetworkQualityEstimator] Use NetworkQualityEstimator to decide on triggering Background Loading immediately. Defines a NetworkQualityEstimator interface, plumbs it into Offline Pages RequestCoordinator, and uses it in logic to determine immediate StartProcessing. BUG=647724 Committed: https://crrev.com/2e385c12e9e4f2b5969bb18ef63ebd66df8c0283 Cr-Commit-Position: refs/heads/master@{#419572} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2e385c12e9e4f2b5969bb18ef63ebd66df8c0283 Cr-Commit-Position: refs/heads/master@{#419572} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
