Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1316863006:
Populate EEP estimate in NQE (Closed)
|
Created:
5 years, 3 months ago by tbansal1 Modified:
5 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Randy Smith (Not in Mondays), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPopulate EEP estimate in NQE
Call EEP on every network change, and populate the estimates
into the NQE observation buffer.
BUG=472685
Committed: https://crrev.com/9ee5d296239e3bc21b5c70259242ce6c4cefc96b
Cr-Commit-Position: refs/heads/master@{#349032}
Patch Set 1 : Minor fix #
Total comments: 14
Patch Set 2 : Addressed bengr comments #
Total comments: 8
Patch Set 3 : Rebased, with changes to DRP code #Patch Set 4 : Addressed bengr comments #
Total comments: 3
Patch Set 5 : Addressed asvitkine comments #
Total comments: 13
Patch Set 6 : Tests and mmenke comments #
Total comments: 20
Patch Set 7 : Rebased, modified to call EEP only on network changes #Patch Set 8 : Changed method names #
Total comments: 22
Patch Set 9 : Fixed tests, addressed comments #
Total comments: 2
Patch Set 10 : Removed network delegate #Patch Set 11 : Using spawned test server #Patch Set 12 : comment out local http server on ios to prevent compilation errors #Patch Set 13 : More commenting out on ioS #Patch Set 14 : Use EXPECT_TRUE instead of DCHECK #Messages
Total messages: 59 (16 generated)
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: PTAL. I will submit this after https://codereview.chromium.org/1273173002/ has landed.
https://codereview.chromium.org/1316863006/diff/20001/net/base/external_estim... File net/base/external_estimate_provider.h (right): https://codereview.chromium.org/1316863006/diff/20001/net/base/external_estim... net/base/external_estimate_provider.h:62: // Places a request to the external estimate provider to update the network // Requests an updated network quality estimate from the external estimate provider. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:147: eep_queried_(base::TimeTicks()), Don't use eep. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:809: if (!external_estimates_provider_->GetTimeSinceLastUpdate( external_estimate_provider_? https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:815: eep_queried_ = base::TimeTicks::Now(); external_estimate_request_time_. Also, is this used? https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:835: if (!eep_delayed_task_posted_ && base::MessageLoop::current()) { eep_delayed_task_posted_ -> external_estimate_pending_? https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:847: void NetworkQualityEstimator::RunExternalEstimateProviderDelayedTask() { I don't understand the flow. Here's what I understand: You start with QEEP and if there was not a previous update, you request one. Otherwise you get the previous RTT and throughput values and add them to your observations. You then post to run QEEP again. What I don't understand is what gets new external estimates? Nothing? Please document the flow. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.h:452: bool eep_delayed_task_posted_; How about external_estimate_pending_?
bengr: PTAL. Thanks. https://codereview.chromium.org/1316863006/diff/20001/net/base/external_estim... File net/base/external_estimate_provider.h (right): https://codereview.chromium.org/1316863006/diff/20001/net/base/external_estim... net/base/external_estimate_provider.h:62: // Places a request to the external estimate provider to update the network On 2015/09/02 20:27:56, bengr wrote: > // Requests an updated network quality estimate from the external estimate > provider. Done. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:147: eep_queried_(base::TimeTicks()), On 2015/09/02 20:27:56, bengr wrote: > Don't use eep. Done. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:809: if (!external_estimates_provider_->GetTimeSinceLastUpdate( On 2015/09/02 20:27:56, bengr wrote: > external_estimate_provider_? Done. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:815: eep_queried_ = base::TimeTicks::Now(); On 2015/09/02 20:27:56, bengr wrote: > external_estimate_request_time_. Also, is this used? It is used now. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:835: if (!eep_delayed_task_posted_ && base::MessageLoop::current()) { On 2015/09/02 20:27:56, bengr wrote: > eep_delayed_task_posted_ -> external_estimate_pending_? Done. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:847: void NetworkQualityEstimator::RunExternalEstimateProviderDelayedTask() { On 2015/09/02 20:27:56, bengr wrote: > I don't understand the flow. Here's what I understand: You start with QEEP and > if there was not a previous update, you request one. Otherwise you get the > previous RTT and throughput values and add them to your observations. You then > post to run QEEP again. > > What I don't understand is what gets new external estimates? Nothing? > > Please document the flow. Updated to make it synchronous call. No posting of tasks. It should be clearer now. https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1316863006/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.h:452: bool eep_delayed_task_posted_; On 2015/09/02 20:27:56, bengr wrote: > How about external_estimate_pending_? Done.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:40001) has been deleted
lgtm with nits https://codereview.chromium.org/1316863006/diff/100001/chrome/browser/android... File chrome/browser/android/net/external_estimate_provider_android.h (right): https://codereview.chromium.org/1316863006/diff/100001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.h:56: // net::ExternalEstimateProvider implementation: I'm ok with you adding this comment only to the first override. Your call. https://codereview.chromium.org/1316863006/diff/100001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.h:66: void RequestUpdate() const override; I would move this up with the other overrides. https://codereview.chromium.org/1316863006/diff/100001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1316863006/diff/100001/net/base/network_quali... net/base/network_quality_estimator.h:451: // Values of external estimate provider status. Move the next line up and fill out to 80 characters. https://codereview.chromium.org/1316863006/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1316863006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28099: + Records the interaction of Network Quality Estimator with external estimates Can you be clearer?
tbansal@chromium.org changed reviewers: + asvitkine@chromium.org, mmenke@chromium.org
mmenke: PTAL at net/base/external_estimate_provider.h asvitkine: PTAL at histograms.xml. Thanks. https://codereview.chromium.org/1316863006/diff/100001/chrome/browser/android... File chrome/browser/android/net/external_estimate_provider_android.h (right): https://codereview.chromium.org/1316863006/diff/100001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.h:56: // net::ExternalEstimateProvider implementation: On 2015/09/08 18:14:23, bengr wrote: > I'm ok with you adding this comment only to the first override. Your call. Thanks, this made it much cleaner. Done. https://codereview.chromium.org/1316863006/diff/100001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.h:66: void RequestUpdate() const override; On 2015/09/08 18:14:23, bengr wrote: > I would move this up with the other overrides. Done. https://codereview.chromium.org/1316863006/diff/100001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1316863006/diff/100001/net/base/network_quali... net/base/network_quality_estimator.h:451: // Values of external estimate provider status. On 2015/09/08 18:14:23, bengr wrote: > Move the next line up and fill out to 80 characters. Done. https://codereview.chromium.org/1316863006/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1316863006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28099: + Records the interaction of Network Quality Estimator with external estimates On 2015/09/08 18:14:24, bengr wrote: > Can you be clearer? Done.
https://codereview.chromium.org/1316863006/diff/130001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/130001/net/base/network_quali... net/base/network_quality_estimator.cc:197: UMA_HISTOGRAM_ENUMERATION("NQE.ExternalEstimateProviderStatus", Please make a helper function that logs this histogram. Each macro expands to a blob of machine code, so duplicating it for the same histogram results in unnecessary bloat. The helper function can take the specific enum as a parameter. https://codereview.chromium.org/1316863006/diff/130001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1316863006/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28279: + Estimator with external estimates provider. Nit: Mention when it's logged.
Alexei, Matt: PTAL. Thanks. https://codereview.chromium.org/1316863006/diff/130001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/130001/net/base/network_quali... net/base/network_quality_estimator.cc:197: UMA_HISTOGRAM_ENUMERATION("NQE.ExternalEstimateProviderStatus", On 2015/09/08 21:33:10, Alexei Svitkine wrote: > Please make a helper function that logs this histogram. > > Each macro expands to a blob of machine code, so duplicating it for the same > histogram results in unnecessary bloat. > > The helper function can take the specific enum as a parameter. Made the helper function. Had to move the enum definition up in the .h file.
lgtm
On 2015/09/09 15:03:30, Alexei Svitkine wrote: > lgtm Sorry for slowness - I'll get back to you today.
Should have tests for this, with a mock external estimator (And probably a mock clock as well) https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:182: external_estimate_request_time_(base::TimeTicks()) { Don't need to explicitly call the default constructor for base::TimeTicks (Or pretty much anything with a 0-argument constructor) https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:867: kExternalEstimateProviderQueryIntervalMsec)) { Why do both MaybeQueryExternalEstimateProvider and QueryExternalEstimateProvider both check against kExternalEstimateProviderQueryIntervalMsec? Yes, they use slightly different base times, but the redundancy seems weird and unless external_estimate_provider_->GetTimeSinceLastUpdate has a lot of overhead, doesn't seem to get us anything. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:880: rtt.InMilliseconds(), base::TimeTicks::Now() - time_since_last_update)); So we just toss in the EEP's estimates in a bucket with everything else?
https://codereview.chromium.org/1316863006/diff/10007/chrome/browser/android/... File chrome/browser/android/net/external_estimate_provider_android.h (right): https://codereview.chromium.org/1316863006/diff/10007/chrome/browser/android/... chrome/browser/android/net/external_estimate_provider_android.h:48: void RequestUpdate() const override; I don't see implementations of these in this CL. Are you missing a file in this CL?
https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:569: MaybeQueryExternalEstimateProvider(); Wait..So the frequency one calls this method can affect how many values we have from ExternalEstimateProvider in our obvservation list? That seems like horrible behavior. Or am I missing something? https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:836: void NetworkQualityEstimator::OnUpdatedEstimateAvailable() { Why is this needed? Seems like we pull it whenever we need it, anyways.
Sorry for the confusion. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:569: MaybeQueryExternalEstimateProvider(); On 2015/09/11 15:07:22, mmenke wrote: > Wait..So the frequency one calls this method can affect how many values we have > from ExternalEstimateProvider in our obvservation list? That seems like > horrible behavior. Or am I missing something? Ah, right...It's the queries vs maybe queries. This still seems weird to me - treating the stuff from the EEP as normal samples, and then just weighting them over time like normal samples. I think it would make more sense to get an estimate from observations, and an estimate from the EEP, and combine them in some way instead. Or alternatively, just prefer the EEP's estimate when we have one.
mmenke: PTAL. Thanks. https://codereview.chromium.org/1316863006/diff/10007/chrome/browser/android/... File chrome/browser/android/net/external_estimate_provider_android.h (right): https://codereview.chromium.org/1316863006/diff/10007/chrome/browser/android/... chrome/browser/android/net/external_estimate_provider_android.h:48: void RequestUpdate() const override; On 2015/09/11 14:28:08, mmenke wrote: > I don't see implementations of these in this CL. Are you missing a file in this > CL? These have just been rearranged to keep all overriding functions together. The implementation is in .cc file. Now, rearranged the contents in the .cc to match the order in .h. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:182: external_estimate_request_time_(base::TimeTicks()) { On 2015/09/11 14:26:29, mmenke wrote: > Don't need to explicitly call the default constructor for base::TimeTicks (Or > pretty much anything with a 0-argument constructor) Done. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:569: MaybeQueryExternalEstimateProvider(); On 2015/09/11 15:17:52, mmenke wrote: > On 2015/09/11 15:07:22, mmenke wrote: > > Wait..So the frequency one calls this method can affect how many values we > have > > from ExternalEstimateProvider in our obvservation list? That seems like > > horrible behavior. Or am I missing something? > > Ah, right...It's the queries vs maybe queries. > > This still seems weird to me - treating the stuff from the EEP as normal > samples, and then just weighting them over time like normal samples. I think it > would make more sense to get an estimate from observations, and an estimate from > the EEP, and combine them in some way instead. Or alternatively, just prefer > the EEP's estimate when we have one. Please see the reply below. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:836: void NetworkQualityEstimator::OnUpdatedEstimateAvailable() { On 2015/09/11 15:07:22, mmenke wrote: > Why is this needed? Seems like we pull it whenever we need it, anyways. EEP that we are currently using on Android has async API. So, this is how it goes with that EEP: (1) native code calls requestUpdated() when it wants to request a fresh estimate. This goes through JNI, calls the async GMS API, and forces it to fetch fresh estimate. (2) At some point in future (hopefully, within milliseconds), GMS API returns the result. (3) Then, the EEP Java plumbing on the Android notifies native that a new estimate is available by calling OnUpdatedEstimateAvailable(). (4) Now, native knows that it can call the GetRTT() API, and get the fresh result. I can point you to the downstream CL, if needed. I have document this life cycle partially here (Lines 870 where I call requestUpdated()). However, a lot of this is implementation specific. So, it is documented more properly in the downstream CL. should care about this. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:867: kExternalEstimateProviderQueryIntervalMsec)) { On 2015/09/11 14:26:29, mmenke wrote: > Why do both MaybeQueryExternalEstimateProvider and QueryExternalEstimateProvider > both check against kExternalEstimateProviderQueryIntervalMsec? Yes, they use > slightly different base times, but the redundancy seems weird and unless > external_estimate_provider_->GetTimeSinceLastUpdate has a lot of overhead, > doesn't seem to get us anything. One is checking when was the last time native requested EEP for updated. This reduces the number of JNI calls to once every |kExternalEstimateProviderQueryIntervalMsec| milliseconds. Second checks if the estimate provided by EEP is fresh. If not, it forces EEP to fetch new estimate. Now, using two different variable names to make it clearer. https://codereview.chromium.org/1316863006/diff/10007/net/base/network_qualit... net/base/network_quality_estimator.cc:880: rtt.InMilliseconds(), base::TimeTicks::Now() - time_since_last_update)); On 2015/09/11 14:26:29, mmenke wrote: > So we just toss in the EEP's estimates in a bucket with everything else? Long-term goal is to look at other useful events: (i) Significant change in the WiFi or cellular signal strength, (ii) Device moves from one cell tower to another. In these cases, it is preferable to lower down the weight of Chromium estimates by a factor, and fetch fresh EEP estimate and give it higher weight (because EEP estimates may be better). Combining NQE and EEP estimates in some way is also worth exploring, but I suspect that the best way to combine would depend on network and/or user behavior (e.g., how many pages they load). So, exploring this is low-priority. For now, we do not have ways to detect these events. So, calling EEP on (i) network change events (ii) Regular intervals. I do not feel strongly about (ii). I don't have data to back it up, and can remove it for now. This would roughly correspond to: use EEP only if NQE is unavailable. Later, when we have knowledge of signal strength/cell tower events, then may be we can call EEP more frequently.
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/1316863006/diff/180001/net/base/external_esti... File net/base/external_estimate_provider.h (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/external_esti... net/base/external_estimate_provider.h:64: virtual void RequestUpdate() const = 0; Why not just Update()? Does Request() add anything to its meaning? Maybe it's free to just ignore the request or something? https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:850: kExternalEstimateProviderQueryIntervalMsec)) { None of the tests check this logic. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:885: rtt.InMilliseconds(), base::TimeTicks::Now() - time_since_last_update)); I still think just randomly adding these sources together like this is a mistake. Suppose Android's system estimate does the same thing you're doing...Suddenly you're weighting previous RTT measurements significantly more (They affect the current estimate, and the previous system estimate). It also has weird effects in that the more consumers you have sampling the data, the greater weight system measurements have, which seems a bit weird. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:893: base::TimeTicks::Now() - time_since_last_update)); I don't think any of the tests check that these observations are actually added, or are combined correctly with estimates from other sources. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:673: nit: Add something along the lines of: // ExternalEstimateProvider implementation: https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:690: NOTIMPLEMENTED(); This should probably be ADD_FAILURE https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:701: void SetTimeSinceLastUpdate(const base::TimeDelta& time_since_last_update) { nit: set_time_since_last_update(base::TimeDelta time_since_last_update) {} (Rename it as a simple setter, don't pass TimeDeltas by reference, since they're so small). https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:701: void SetTimeSinceLastUpdate(const base::TimeDelta& time_since_last_update) { nit: This should not go in the middle of the override methods. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:722: mutable size_t rtt_calls_count_; nit: get_rtt_count_, to match request_update_count_?
Patchset #7 (id:200001) has been deleted
PTAL. thanks. https://codereview.chromium.org/1316863006/diff/180001/net/base/external_esti... File net/base/external_estimate_provider.h (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/external_esti... net/base/external_estimate_provider.h:64: virtual void RequestUpdate() const = 0; On 2015/09/14 20:07:06, mmenke wrote: > Why not just Update()? Does Request() add anything to its meaning? Maybe it's > free to just ignore the request or something? Done. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:850: kExternalEstimateProviderQueryIntervalMsec)) { On 2015/09/14 20:07:06, mmenke wrote: > None of the tests check this logic. Obsolete. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:885: rtt.InMilliseconds(), base::TimeTicks::Now() - time_since_last_update)); On 2015/09/14 20:07:06, mmenke wrote: > I still think just randomly adding these sources together like this is a > mistake. > > Suppose Android's system estimate does the same thing you're doing...Suddenly > you're weighting previous RTT measurements significantly more (They affect the > current estimate, and the previous system estimate). > > It also has weird effects in that the more consumers you have sampling the data, > the greater weight system measurements have, which seems a bit weird. Modified the CL to add estimate from external provider only on network changes. However, that estimate is still mixed with the other observations. So, there is no periodic sampling of EEP anymore. There is another CL that tags the Observation with the source from where the observation came (https://codereview.chromium.org/1273173002/diff/300001/net/base/network_quali...). With that, it would be possible to distinguish the observations, if there is a need to do that. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:893: base::TimeTicks::Now() - time_since_last_update)); On 2015/09/14 20:07:06, mmenke wrote: > I don't think any of the tests check that these observations are actually added, > or are combined correctly with estimates from other sources. Added test to make sure that the observation size increases. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:673: On 2015/09/14 20:07:06, mmenke wrote: > nit: Add something along the lines of: > > // ExternalEstimateProvider implementation: Done. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:690: NOTIMPLEMENTED(); On 2015/09/14 20:07:06, mmenke wrote: > This should probably be ADD_FAILURE Done. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:701: void SetTimeSinceLastUpdate(const base::TimeDelta& time_since_last_update) { On 2015/09/14 20:07:06, mmenke wrote: > nit: This should not go in the middle of the override methods. Done. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:701: void SetTimeSinceLastUpdate(const base::TimeDelta& time_since_last_update) { On 2015/09/14 20:07:06, mmenke wrote: > nit: set_time_since_last_update(base::TimeDelta time_since_last_update) {} > > (Rename it as a simple setter, don't pass TimeDeltas by reference, since they're > so small). Done. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:722: mutable size_t rtt_calls_count_; On 2015/09/14 20:07:06, mmenke wrote: > nit: get_rtt_count_, to match request_update_count_? Did you mean rtt_count_?
Quick response, will do another (Final?) pass later today. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:722: mutable size_t rtt_calls_count_; On 2015/09/14 23:46:57, tbansal1 wrote: > On 2015/09/14 20:07:06, mmenke wrote: > > nit: get_rtt_count_, to match request_update_count_? > > Did you mean rtt_count_? No, I meant get_rtt_count_. Name of the method it's counting calls to (GetRTT), and then count. I'm fine with including the "calls" in the name, though if you do that, should include it in both the variables to count method calls.
Changed the method names. Thanks. https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/180001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:722: mutable size_t rtt_calls_count_; On 2015/09/15 15:19:53, mmenke wrote: > On 2015/09/14 23:46:57, tbansal1 wrote: > > On 2015/09/14 20:07:06, mmenke wrote: > > > nit: get_rtt_count_, to match request_update_count_? > > > > Did you mean rtt_count_? > > No, I meant get_rtt_count_. Name of the method it's counting calls to (GetRTT), > and then count. I'm fine with including the "calls" in the name, though if you > do that, should include it in both the variables to count method calls. That makes sense. Changed the metod names to have a suffix of count.
https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... File chrome/browser/android/net/external_estimate_provider_android.cc (right): https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.cc:97: void ExternalEstimateProviderAndroid::RequestUpdate() const { Update() https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... File chrome/browser/android/net/external_estimate_provider_android.h (right): https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.h:48: void RequestUpdate() const override; Update() https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:688: return true; Maybe also have another test with an ExternalEstimateProvider where these both return false, and make sure we don't get any observations from it? https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:717: size_t get_time_since_last_update_count() const { -get. The get in get_rtt_count is because it's the number of times *Get*RTT was called, not because we're getting the rtt_count (We're not, in fact, getting a count of RTTs). https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:720: size_t get_rtt_count() const { return rtt_count_; } nit: Rename rtt_count_, too. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:721: size_t get_downstream_throughput_kbps_count() const { -get https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:822: base::TimeDelta external_estimate_provider_rtt; Think it's much cleaner to make this a constant (1 millisecond) and check that rtt is exactly what we expect. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:831: EXPECT_EQ(external_estimate_provider_downstream_throughput, kbps); As above, should be comparing these with constants. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:837: base::FilePath(FILE_PATH_LITERAL("net/data/url_request_unittest"))); include FilePath header. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:846: request->SetLoadFlags(request->load_flags() | LOAD_MAIN_FRAME); I don't think we care about the load flags? Can just remove this line. Also want to get rid of the load flag, eventually. Don't want more dependencies on it. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:852: estimator.NotifyRequestCompleted(*request); Why are we doing this manually? Can't we just set the estimator on the URLRequestContext? Initialize it with true instead of false, set its estimator, and then call init on it, and you shouldn't need these two lines.
Patchset #9 (id:260001) has been deleted
Made changes to the NQE tests to make them more consistent and realistic. PTAL. https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... File chrome/browser/android/net/external_estimate_provider_android.cc (right): https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.cc:97: void ExternalEstimateProviderAndroid::RequestUpdate() const { On 2015/09/15 20:09:09, mmenke wrote: > Update() Done. https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... File chrome/browser/android/net/external_estimate_provider_android.h (right): https://codereview.chromium.org/1316863006/diff/240001/chrome/browser/android... chrome/browser/android/net/external_estimate_provider_android.h:48: void RequestUpdate() const override; On 2015/09/15 20:09:09, mmenke wrote: > Update() Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:688: return true; On 2015/09/15 20:09:09, mmenke wrote: > Maybe also have another test with an ExternalEstimateProvider where these both > return false, and make sure we don't get any observations from it? Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:717: size_t get_time_since_last_update_count() const { On 2015/09/15 20:09:09, mmenke wrote: > -get. The get in get_rtt_count is because it's the number of times *Get*RTT was > called, not because we're getting the rtt_count (We're not, in fact, getting a > count of RTTs). I am confused. This returns the number of times *Get*TimeSinceLastUpdate() was called. So, this should have get too. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:720: size_t get_rtt_count() const { return rtt_count_; } On 2015/09/15 20:09:09, mmenke wrote: > nit: Rename rtt_count_, too. Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:721: size_t get_downstream_throughput_kbps_count() const { On 2015/09/15 20:09:09, mmenke wrote: > -get same here. this is counting number of times GetDownstreamThroughputKbps() was called. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:822: base::TimeDelta external_estimate_provider_rtt; On 2015/09/15 20:09:09, mmenke wrote: > Think it's much cleaner to make this a constant (1 millisecond) and check that > rtt is exactly what we expect. Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:831: EXPECT_EQ(external_estimate_provider_downstream_throughput, kbps); On 2015/09/15 20:09:09, mmenke wrote: > As above, should be comparing these with constants. Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:837: base::FilePath(FILE_PATH_LITERAL("net/data/url_request_unittest"))); On 2015/09/15 20:09:09, mmenke wrote: > include FilePath header. Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:846: request->SetLoadFlags(request->load_flags() | LOAD_MAIN_FRAME); On 2015/09/15 20:09:09, mmenke wrote: > I don't think we care about the load flags? Can just remove this line. Also > want to get rid of the load flag, eventually. Don't want more dependencies on > it. Done. https://codereview.chromium.org/1316863006/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:852: estimator.NotifyRequestCompleted(*request); On 2015/09/15 20:09:09, mmenke wrote: > Why are we doing this manually? Can't we just set the estimator on the > URLRequestContext? Initialize it with true instead of false, set its estimator, > and then call init on it, and you shouldn't need these two lines. Done. That's what I was doing in URLRequestTestHTTP.NetworkQualityEstimator test.
LGTM! https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:930: TestNetworkDelegate network_delegate; // Must outlive URLRequest. I don't think we need this?
https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:930: TestNetworkDelegate network_delegate; // Must outlive URLRequest. On 2015/09/15 22:36:53, mmenke wrote: > I don't think we need this? Removed it. Seems like it is required for LocalHttpServer, not for EmbeddedTestServer. Thanks for catching that.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1316863006/#ps300001 (title: "Removed network delegate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316863006/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316863006/300001
On 2015/09/15 22:44:47, tbansal1 wrote: > https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... > File net/base/network_quality_estimator_unittest.cc (right): > > https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... > net/base/network_quality_estimator_unittest.cc:930: TestNetworkDelegate > network_delegate; // Must outlive URLRequest. > On 2015/09/15 22:36:53, mmenke wrote: > > I don't think we need this? > > Removed it. Seems like it is required for LocalHttpServer, not for > EmbeddedTestServer. Thanks for catching that. Weird...It shouldn't be needed for either.
On 2015/09/15 22:46:45, mmenke wrote: > On 2015/09/15 22:44:47, tbansal1 wrote: > > > https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... > > File net/base/network_quality_estimator_unittest.cc (right): > > > > > https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... > > net/base/network_quality_estimator_unittest.cc:930: TestNetworkDelegate > > network_delegate; // Must outlive URLRequest. > > On 2015/09/15 22:36:53, mmenke wrote: > > > I don't think we need this? > > > > Removed it. Seems like it is required for LocalHttpServer, not for > > EmbeddedTestServer. Thanks for catching that. > > Weird...It shouldn't be needed for either. I saw it used at lot of places in url_request_unittest.cc (e.g., https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... and https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...) Not sure if it is really needed.
On 2015/09/15 23:03:19, tbansal1 wrote: > On 2015/09/15 22:46:45, mmenke wrote: > > On 2015/09/15 22:44:47, tbansal1 wrote: > > > > > > https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... > > > File net/base/network_quality_estimator_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1316863006/diff/280001/net/base/network_quali... > > > net/base/network_quality_estimator_unittest.cc:930: TestNetworkDelegate > > > network_delegate; // Must outlive URLRequest. > > > On 2015/09/15 22:36:53, mmenke wrote: > > > > I don't think we need this? > > > > > > Removed it. Seems like it is required for LocalHttpServer, not for > > > EmbeddedTestServer. Thanks for catching that. > > > > Weird...It shouldn't be needed for either. > > I saw it used at lot of places in url_request_unittest.cc (e.g., > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > and > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...) > Not sure if it is really needed. Some tests in that file need it (Namely, those that query it explicitly), but the others don't. Just a case of copying more code than was needed, probably.
Message was sent while issue was closed.
Committed patchset #10 (id:300001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9ee5d296239e3bc21b5c70259242ce6c4cefc96b Cr-Commit-Position: refs/heads/master@{#349032}
Message was sent while issue was closed.
This is failing on multiple bots: http://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/7704 http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/6783 http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/31023 http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%... but I can't just hit the revert button. Manually reverting...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Revert in https://codereview.chromium.org/1348523003/
Message was sent while issue was closed.
Some more failed bots: http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/buil... How did this pass CQ?
Message was sent while issue was closed.
On 2015/09/16 01:56:30, Lei Zhang wrote: > Some more failed bots: > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... > http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/buil... > > How did this pass CQ? Embedded test server is not used in any other tests in src/net/. That makes me wonder if it unsuitable, and I should go back to using LocalHTTPServer. Documentation (https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...) says that embedded test server makes callbacks on UI thread. May be that's the problem, although I can't figure out why. Anyways, I am going to try local HTTP server (like rest of the net stack tests). Only problem is that we will not have test coverage on iOS because local HTTP server is unsupported on iOS.
Message was sent while issue was closed.
On 2015/09/16 16:00:27, tbansal1 wrote: > On 2015/09/16 01:56:30, Lei Zhang wrote: > > Some more failed bots: > > > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... > > > http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/buil... > > > > How did this pass CQ? > > Embedded test server is not used in any other tests in src/net/. That makes me > wonder > if it unsuitable, and I should go back to using LocalHTTPServer. Documentation > (https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...) > says that embedded test server makes callbacks on UI thread. May be that's the > problem, > although I can't figure out why. > > Anyways, I am going to try local HTTP server (like rest of the net stack tests). > Only problem > is that we will not have test coverage on iOS because local HTTP server is > unsupported > on iOS. Embedded test server is the replacement for the spawned test server - it's basically newer than most of the test fixtures is in net, and we'll eventually want to use it everywhere (Both for iOS, and because we're moving on to new Android test infrastructure which can't use the spawned server). I'd like to at least try and figure out what's going on, but may be simplest just to use the spawned server for now.
On 2015/09/16 16:21:59, mmenke wrote: > On 2015/09/16 16:00:27, tbansal1 wrote: > > On 2015/09/16 01:56:30, Lei Zhang wrote: > > > Some more failed bots: > > > > > > http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28... > > > > > > http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/buil... > > > > > > How did this pass CQ? > > > > Embedded test server is not used in any other tests in src/net/. That makes me > > wonder > > if it unsuitable, and I should go back to using LocalHTTPServer. Documentation > > > (https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...) > > says that embedded test server makes callbacks on UI thread. May be that's the > > problem, > > although I can't figure out why. > > > > Anyways, I am going to try local HTTP server (like rest of the net stack > tests). > > Only problem > > is that we will not have test coverage on iOS because local HTTP server is > > unsupported > > on iOS. > > Embedded test server is the replacement for the spawned test server - it's > basically newer than most of the test fixtures is in net, and we'll eventually > want to use it everywhere (Both for iOS, and because we're moving on to new > Android test infrastructure which can't use the spawned server). > > I'd like to at least try and figure out what's going on, but may be simplest > just to use the spawned server for now. Yeah, I did debug a bit. But, all I know so far is that on some trybots, it always fails. On others, it always passes. On my desktop, it always passes making it harder to repro.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1316863006/#ps360001 (title: "More commenting out on ioS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316863006/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316863006/360001
Message was sent while issue was closed.
Committed patchset #13 (id:360001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/b112feef767d087d6c40deb88c983d61c085d457 Cr-Commit-Position: refs/heads/master@{#349232}
Message was sent while issue was closed.
On 2015/09/16 21:27:03, commit-bot: I haz the power wrote: > Patchset 13 (id:??) landed as > https://crrev.com/b112feef767d087d6c40deb88c983d61c085d457 > Cr-Commit-Position: refs/heads/master@{#349232} Reverting: https://codereview.chromium.org/1348393003/
Message was sent while issue was closed.
On 2015/09/16 22:06:24, Justin Donnelly wrote: > On 2015/09/16 21:27:03, commit-bot: I haz the power wrote: > > Patchset 13 (id:??) landed as > > https://crrev.com/b112feef767d087d6c40deb88c983d61c085d457 > > Cr-Commit-Position: refs/heads/master@{#349232} > > Reverting: https://codereview.chromium.org/1348393003/ net_unittests failing on multiple bots (Cast Linux Linux Tests Mac10.9 Tests)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1316863006/#ps380001 (title: "Use EXPECT_TRUE instead of DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316863006/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316863006/380001
Message was sent while issue was closed.
Committed patchset #14 (id:380001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/2c6b0d3834a5d0e6e14848d387b9951acf4cb267 Cr-Commit-Position: refs/heads/master@{#349350}
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9ee5d296239e3bc21b5c70259242ce6c4cefc96b Cr-Commit-Position: refs/heads/master@{#349032} |
