Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2146563002:
Expose effective connection type to Cronet (Closed)
DescriptionExpose effective connection type to Cronet
CronetURLContextAdapter registers as an Effective Connection
Type (ECT) Observer. When it receives notifications on
network thread about changes in ECT, it passes them to
Java land where ECT is stored in a guarded variable.
ECT is computed using transport RTT and downlink bandwidth.
This CL also adds default transport RTT thresholds for
different effective connection types so that NQE can
compute ECT when thresholds have not been specified using
field trial params (as is the case for Cronet).
BUG=625305
Committed: https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e
Cr-Commit-Position: refs/heads/master@{#409624}
Patch Set 1 : Patch #
Total comments: 4
Patch Set 2 : Updated comments #
Total comments: 2
Patch Set 3 : Rebased, addressed Paul comments #
Total comments: 8
Patch Set 4 : Addressed xunjieli comments #
Total comments: 4
Patch Set 5 : Rebased #Patch Set 6 : Addressed comments (added periods) #Messages
Total messages: 47 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Expose effective connection type to Cronet BUG= ========== to ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. BUG=625305 ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: PTAL at * before I pass this on to cronet OWNERS. Thanks!
Description was changed from ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. BUG=625305 ========== to ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. ECT is computed using transport RTT and downlink bandwidth. This CL also adds default transport RTT thresholds for different effective connection types so that NQE can compute ECT when thresholds have not been specified using field trial params (as is the case for Cronet). BUG=625305 ==========
lgtm % a nit https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:330: base::TimeDelta::FromMilliseconds(1870), nit: Can you move the 1870 and 1300 to a static const int (or some other variable outside of this block of code)? It bothers me that the comments seem so far from the actual value.
tbansal@chromium.org changed reviewers: + xunjieli@chromium.org
xunjieli: ptal. Thanks. https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:330: base::TimeDelta::FromMilliseconds(1870), On 2016/07/12 19:57:55, RyanSturm wrote: > nit: Can you move the 1870 and 1300 to a static const int (or some other > variable outside of this block of code)? I would prefer to keep them here instead of using static const. Suppose, I was setting HTTP RTT and downlink throughput threshold too, then it would be 3 static const variables per connection type (so may be 3*7 = 21 static consts). > It bothers me that the comments seem so > far from the actual value. What comments are you referring too?
https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:330: base::TimeDelta::FromMilliseconds(1870), On 2016/07/12 20:32:25, tbansal1 wrote: > On 2016/07/12 19:57:55, RyanSturm wrote: > > nit: Can you move the 1870 and 1300 to a static const int (or some other > > variable outside of this block of code)? > I would prefer to keep them here instead of using static const. Suppose, I was > setting HTTP RTT and downlink throughput threshold too, then it would be 3 > static const variables per connection type (so may be 3*7 = 21 static consts). > > > It bothers me that the comments seem so > > far from the actual value. > What comments are you referring too? > I think it would be more clear like: // 1.87 seconds is the 33rd percentile of 2G transport RTT // observations. static const int slow_2g_rtt_in_milliseconds = 1870; default_connection_thresholds_[...] = ... base::TimeDelta::FromMilliseconds(slow_2g_rtt_in_milliseconds), ...; I'm not sure if the style guide says anything about this. wdyt?
https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2146563002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:330: base::TimeDelta::FromMilliseconds(1870), On 2016/07/12 20:44:47, RyanSturm wrote: > On 2016/07/12 20:32:25, tbansal1 wrote: > > On 2016/07/12 19:57:55, RyanSturm wrote: > > > nit: Can you move the 1870 and 1300 to a static const int (or some other > > > variable outside of this block of code)? > > I would prefer to keep them here instead of using static const. Suppose, I was > > setting HTTP RTT and downlink throughput threshold too, then it would be 3 > > static const variables per connection type (so may be 3*7 = 21 static consts). > > > > > It bothers me that the comments seem so > > > far from the actual value. > > What comments are you referring too? > > > > I think it would be more clear like: > > // 1.87 seconds is the 33rd percentile of 2G transport RTT > // observations. > static const int slow_2g_rtt_in_milliseconds = 1870; > default_connection_thresholds_[...] = ... > base::TimeDelta::FromMilliseconds(slow_2g_rtt_in_milliseconds), ...; > > I'm not sure if the style guide says anything about this. > > wdyt? I updated the comments.
Do we have a design doc for this? If not, can we make one and send it out for a design review?
On 2016/07/13 13:54:17, xunjieli wrote: > Do we have a design doc for this? If not, can we make one and send it out for a > design review? There is no design doc yet. I will try to write something today or tomorrow. You can look the CL after I send out the design doc.
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
On 2016/07/13 17:59:41, tbansal1 wrote: > On 2016/07/13 13:54:17, xunjieli wrote: > > Do we have a design doc for this? If not, can we make one and send it out for > a > > design review? > > There is no design doc yet. I will try to write something today or tomorrow. > You can look the CL after I send out the design doc. Sounds good. Thanks. pauljensen@ will be back next week. It will be good to get his opinion too.
https://codereview.chromium.org/2146563002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2146563002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:905: * //net/nqe/network_quality_estimator.h. I think there are a few issues here: 1. It's not clear to me what "effective connection type" is. It sounds like we're trying to replace Android's ConnectivityManager which seems like a very hard thing to do. 2. I don't think readers of this API know what "network quality estimator" is. 3. We don't want to be referencing C++ code from Java code if at all possible. 4. I don't think users will know what "//net/nqe/network_quality_estimator.h" is.
Patchset #3 (id:80001) has been deleted
tbansal@chromium.org changed reviewers: - pauljensen@chromium.org
xunjieli: PTAL. Thanks. I moved Paul to cc' since he is on vacation.
https://codereview.chromium.org/2146563002/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2146563002/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:905: * //net/nqe/network_quality_estimator.h. On 2016/07/18 13:42:17, pauljensen wrote: > I think there are a few issues here: > 1. It's not clear to me what "effective connection type" is. It sounds like > we're trying to replace Android's ConnectivityManager which seems like a very > hard thing to do. > 2. I don't think readers of this API know what "network quality estimator" is. > 3. We don't want to be referencing C++ code from Java code if at all possible. > 4. I don't think users will know what "//net/nqe/network_quality_estimator.h" > is. Done.
Can you send out the design doc to cronet@? I didn't see that design doc until now. Paul has done a lot of API work, I think it is important to get his opinion too since this will likely affect our plan to integrate with GMSCore and we want to be really careful with new additions. How urgent is this? Are you aiming for M55? https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java (right): https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:20: public enum EffectiveConnectionType { Prefer int over enum. See: https://groups.google.com/a/google.com/forum/?utm_medium=email&utm_source=foo... https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:39: case 0: Please use generated ints and not the hardcoded values. https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:107: nit: new line not needed. https://codereview.chromium.org/2146563002/diff/100001/net/nqe/effective_conn... File net/nqe/effective_connection_type.h (right): https://codereview.chromium.org/2146563002/diff/100001/net/nqe/effective_conn... net/nqe/effective_connection_type.h:22: // GENERATED_JAVA_CLASS_NAME_OVERRIDE: NetworkEffectiveConnectionType So this will generate a NetworkEffectiveConnectionType.java file which includes the enums as static ints. I think it will be confusing to have both NetworkEffectiveConnectionType.java and EffectiveConnectionType.java. Our consumers will see both.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
xunjieli: PTAL. Since this is a hidden API, the risk is low. We can always remove it, or break it. I would like to land this soon as there are some embedders which are interested in using this (I think you and Paul are cc'ed on that thread). https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java (right): https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:20: public enum EffectiveConnectionType { On 2016/08/01 21:25:09, xunjieli wrote: > Prefer int over enum. > See: > https://groups.google.com/a/google.com/forum/?utm_medium=email&utm_source=foo... Thanks, I was not aware of IntDefs. Now this looks very similar to how LoadState is used in Cronet. https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:39: case 0: On 2016/08/01 21:25:09, xunjieli wrote: > Please use generated ints and not the hardcoded values. Using IntDef now (just like LoadState). https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java (right): https://codereview.chromium.org/2146563002/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java:107: On 2016/08/01 21:25:09, xunjieli wrote: > nit: new line not needed. Done. https://codereview.chromium.org/2146563002/diff/100001/net/nqe/effective_conn... File net/nqe/effective_connection_type.h (right): https://codereview.chromium.org/2146563002/diff/100001/net/nqe/effective_conn... net/nqe/effective_connection_type.h:22: // GENERATED_JAVA_CLASS_NAME_OVERRIDE: NetworkEffectiveConnectionType On 2016/08/01 21:25:09, xunjieli wrote: > So this will generate a NetworkEffectiveConnectionType.java file which includes > the enums as static ints. > > I think it will be confusing to have both NetworkEffectiveConnectionType.java > and EffectiveConnectionType.java. Our consumers will see both. This is now very similar to how C++ enum LoadState is converted to Java IntDef Status. An alternate approach is to expose these only for testing (Like I was doing in the previous patch). Then, add a test which ensures that the C++ enum and the Java IntDef remain synchronized. Then, the production code would not specify the ChromiumEffectiveConnectionType.java as a dependency, and consumers would only see one of the two classes.
I still think we should avoid duplicating the generated Java files. I will look into that. LGTM. https://codereview.chromium.org/2146563002/diff/120002/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java (right): https://codereview.chromium.org/2146563002/diff/120002/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:56: * faster 2G connection nit: Add a period at the end. https://codereview.chromium.org/2146563002/diff/120002/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:62: * 3G connection nit: Add a period at the end.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
https://codereview.chromium.org/2146563002/diff/120002/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java (right): https://codereview.chromium.org/2146563002/diff/120002/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:56: * faster 2G connection On 2016/08/03 16:54:13, xunjieli wrote: > nit: Add a period at the end. Done. https://codereview.chromium.org/2146563002/diff/120002/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java:62: * 3G connection On 2016/08/03 16:54:13, xunjieli wrote: > nit: Add a period at the end. 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 tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2146563002/#ps200001 (title: "Addressed comments (added periods)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. ECT is computed using transport RTT and downlink bandwidth. This CL also adds default transport RTT thresholds for different effective connection types so that NQE can compute ECT when thresholds have not been specified using field trial params (as is the case for Cronet). BUG=625305 ========== to ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. ECT is computed using transport RTT and downlink bandwidth. This CL also adds default transport RTT thresholds for different effective connection types so that NQE can compute ECT when thresholds have not been specified using field trial params (as is the case for Cronet). BUG=625305 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. ECT is computed using transport RTT and downlink bandwidth. This CL also adds default transport RTT thresholds for different effective connection types so that NQE can compute ECT when thresholds have not been specified using field trial params (as is the case for Cronet). BUG=625305 ========== to ========== Expose effective connection type to Cronet CronetURLContextAdapter registers as an Effective Connection Type (ECT) Observer. When it receives notifications on network thread about changes in ECT, it passes them to Java land where ECT is stored in a guarded variable. ECT is computed using transport RTT and downlink bandwidth. This CL also adds default transport RTT thresholds for different effective connection types so that NQE can compute ECT when thresholds have not been specified using field trial params (as is the case for Cronet). BUG=625305 Committed: https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e Cr-Commit-Position: refs/heads/master@{#409624} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e Cr-Commit-Position: refs/heads/master@{#409624}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
I'm seeing stacks in this file on some bots: https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... [4960:4152:0803/192101:FATAL:network_quality_estimator.cc(805)] Check failed: !load_timing_info.send_start.is_null() && !load_timing_info.receive_headers_end.is_null(). Backtrace: base::debug::StackTrace::StackTrace [0x100789EE+62] logging::LogMessage::~LogMessage [0x100EBDD7+71] net::NetworkQualityEstimator::RecordCorrelationMetric [0x0C20AD49+985] net::NetworkQualityEstimator::NotifyRequestCompleted [0x0C20A855+789] net::URLRequestHttpJob::DoneWithRequest [0x0C8A23D1+129] net::URLRequestHttpJob::DestroyTransaction [0x0C8A3CFE+334] net::URLRequestHttpJob::Kill [0x0C8A39E1+65] net::URLRequest::OrphanJob [0x0C84F428+40] net::URLRequest::~URLRequest [0x0C862CD0+176] net::URLRequest::`scalar deleting destructor' [0x0C85C577+39] std::default_delete<net::URLRequest>::operator() [0x0BD173A4+52] std::unique_ptr<net::URLRequest,std::default_delete<net::URLRequest> >::reset [0x0BD12819+89] net::URLFetcherCore::ReleaseRequest [0x0C83914D+109] also https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28dbg%29%20test... Could that be due to this CL?
Message was sent while issue was closed.
thakis@, thanks for reporting. This should be fixed now ( https://bugs.chromium.org/p/chromium/issues/detail?id=634240). On Wed, Aug 3, 2016 at 8:38 PM <thakis@chromium.org> wrote: > I'm seeing stacks in this file on some bots: > > > https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... > > [4960:4152:0803/192101:FATAL:network_quality_estimator.cc(805)] Check > failed: > !load_timing_info.send_start.is_null() && > !load_timing_info.receive_headers_end.is_null(). > Backtrace: > base::debug::StackTrace::StackTrace [0x100789EE+62] > logging::LogMessage::~LogMessage [0x100EBDD7+71] > net::NetworkQualityEstimator::RecordCorrelationMetric [0x0C20AD49+985] > net::NetworkQualityEstimator::NotifyRequestCompleted [0x0C20A855+789] > net::URLRequestHttpJob::DoneWithRequest [0x0C8A23D1+129] > net::URLRequestHttpJob::DestroyTransaction [0x0C8A3CFE+334] > net::URLRequestHttpJob::Kill [0x0C8A39E1+65] > net::URLRequest::OrphanJob [0x0C84F428+40] > net::URLRequest::~URLRequest [0x0C862CD0+176] > net::URLRequest::`scalar deleting destructor' [0x0C85C577+39] > std::default_delete<net::URLRequest>::operator() [0x0BD173A4+52] > std::unique_ptr<net::URLRequest,std::default_delete<net::URLRequest> > >::reset > [0x0BD12819+89] > net::URLFetcherCore::ReleaseRequest [0x0C83914D+109] > > > also > > https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28dbg%29%20test... > > > Could that be due to this CL? > > https://codereview.chromium.org/2146563002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Thanks for the change, but the test is still failing, just with a different error: https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... [3060:6028:0804/100543:ERROR:web_ui_browser_test.cc(466)] JS call assumed failed, because JS console error(s) found. gen/chrome/test/data/webui/settings/cr_settings_browsertest-gen.cc(941): error: Value of: RunJavascriptTestF( true, "CrSettingsMainPageTest", "All") Actual: false Expected: true Maybe the actual problem is a race, and the DCHECK was just pointing that out?
Message was sent while issue was closed.
On 2016/08/04 19:21:56, Nico wrote: > Thanks for the change, but the test is still failing, just with a different > error: > https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... > > [3060:6028:0804/100543:ERROR:web_ui_browser_test.cc(466)] JS call assumed > failed, because JS console error(s) found. > gen/chrome/test/data/webui/settings/cr_settings_browsertest-gen.cc(941): error: > Value of: RunJavascriptTestF( true, "CrSettingsMainPageTest", "All") > Actual: false > Expected: true > > Maybe the actual problem is a race, and the DCHECK was just pointing that out? Is there a more detailed log available? I am not sure that this failure is related to this CL or the other //net/nqe/ related CLs.
Message was sent while issue was closed.
On https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes..., click on "CrSettingsMainPageTest.All" (leading here: https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... ) The test has been failing consistently since this CL landed ( https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... – starting at build 2913), so it looks pretty related. Maybe we could speculatively revert this and see if it helps? On Thu, Aug 4, 2016 at 3:32 PM, <tbansal@chromium.org> wrote: > On 2016/08/04 19:21:56, Nico wrote: > > Thanks for the change, but the test is still failing, just with a > different > > error: > > > https://build.chromium.org/p/chromium.fyi/builders/ > ClangToTWin%28dbg%29%20tester/builds/2919 > > > > [3060:6028:0804/100543:ERROR:web_ui_browser_test.cc(466)] JS call > assumed > > failed, because JS console error(s) found. > > gen/chrome/test/data/webui/settings/cr_settings_browsertest-gen.cc(941): > error: > > Value of: RunJavascriptTestF( true, "CrSettingsMainPageTest", "All") > > Actual: false > > Expected: true > > > > Maybe the actual problem is a race, and the DCHECK was just pointing > that out? > > Is there a more detailed log available? I am not sure that this > failure is related to this CL or the other //net/nqe/ related CLs. > > https://codereview.chromium.org/2146563002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/08/04 19:34:14, Nico wrote: > On > https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes..., > click on "CrSettingsMainPageTest.All" (leading here: > https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... > ) > > The test has been failing consistently since this CL landed ( > https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... > – starting at build 2913), so it looks pretty related. Maybe we could > speculatively revert this and see if it helps? > > On Thu, Aug 4, 2016 at 3:32 PM, <mailto:tbansal@chromium.org> wrote: > > > On 2016/08/04 19:21:56, Nico wrote: > > > Thanks for the change, but the test is still failing, just with a > > different > > > error: > > > > > https://build.chromium.org/p/chromium.fyi/builders/ > > ClangToTWin%28dbg%29%20tester/builds/2919 > > > > > > [3060:6028:0804/100543:ERROR:web_ui_browser_test.cc(466)] JS call > > assumed > > > failed, because JS console error(s) found. > > > gen/chrome/test/data/webui/settings/cr_settings_browsertest-gen.cc(941): > > error: > > > Value of: RunJavascriptTestF( true, "CrSettingsMainPageTest", "All") > > > Actual: false > > > Expected: true > > > > > > Maybe the actual problem is a race, and the DCHECK was just pointing > > that out? > > > > Is there a more detailed log available? I am not sure that this > > failure is related to this CL or the other //net/nqe/ related CLs. > > > > https://codereview.chromium.org/2146563002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Well, the failing test was also added yesterday in a CL that was rolled in the same build # (2913): https://codereview.chromium.org/2185493003/diff/360001/chrome/test/data/webui... https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... But, I can revert if it helps with the triage.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:200001) has been created in https://codereview.chromium.org/2214163002/ by tbansal@chromium.org. The reason for reverting is: Speculative revert to see if this fixes failing tests on ClangToTWin(dbg) tester: https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes....
Message was sent while issue was closed.
Oh, if the test is new then I agree it makes more sense to revert the CL that added the test. |
