NetInfo: Slight update to the default network quality value

content/browser/net_info_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <cmath>  // For std::modf.
 #include <map>
 #include <string>
 
 #include "base/command_line.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/histogram_tester.h"
 #include "build/build_config.h"
 #include "content/browser/net/network_quality_observer_impl.h"
 #include "content/public/common/content_switches.h"
@@ skipping 73 matching lines @@
     bool data;
     EXPECT_TRUE(ExecuteScriptAndExtractBool(shell(), script, &data));
     return data;
   }
 
   double RunScriptExtractDouble(const std::string& script) {
     double data = 0.0;
     EXPECT_TRUE(base::StringToDouble(RunScriptExtractString(script), &data));
     return data;
   }
+
+  int RunScriptExtractInt(const std::string& script) {
+    int data = 0;
+    EXPECT_TRUE(base::StringToInt(RunScriptExtractString(script), &data));

[nasko, 2017/06/01 22:16:13]

ExecuteScriptAndExtractInt?

[tbansal1, 2017/06/01 23:26:46]

Done.

+    return data;
+  }
 };
 
 // Make sure the type is correct when the page is first opened.
 IN_PROC_BROWSER_TEST_F(NetInfoBrowserTest, VerifyNetworkStateInitialized) {
   SetConnectionType(net::NetworkChangeNotifier::CONNECTION_ETHERNET,
                     net::NetworkChangeNotifier::SUBTYPE_GIGABIT_ETHERNET);
   NavigateToURL(shell(), content::GetTestUrl("", "net_info.html"));
   EXPECT_TRUE(RunScriptExtractBool("getOnLine()"));
   EXPECT_EQ("ethernet", RunScriptExtractString("getType()"));
   EXPECT_EQ(net::NetworkChangeNotifier::GetMaxBandwidthForConnectionSubtype(
@@ skipping 49 matching lines @@
 
   // Open the same page in a new window on the same process.
   EXPECT_TRUE(ExecuteScript(shell(), "window.open(\"net_info.html\")"));
 
   // The network state should not have reinitialized to what it was when opening
   // the first window (online).
   EXPECT_FALSE(RunScriptExtractBool("getOnLine()"));
 }
 
 // Verify that when the network quality notifications are not sent, the
-// Javascript API returns invalid estimate.
+// Javascript API returns a valid estimate that is multiple of 25 msec and 25
+// kbps.
 IN_PROC_BROWSER_TEST_F(NetInfoBrowserTest,
                        NetworkQualityEstimatorNotInitialized) {
   base::HistogramTester histogram_tester;
   net::TestNetworkQualityEstimator estimator(
       nullptr, std::map<std::string, std::string>(), false, false, true, true,
       base::MakeUnique<net::BoundTestNetLog>());
   NetworkQualityObserverImpl impl(&estimator);
 
   EXPECT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(
       NavigateToURL(shell(), embedded_test_server()->GetURL("/net_info.html")));
 
-  EXPECT_EQ(0, RunScriptExtractDouble("getRtt()"));
-  EXPECT_EQ(std::numeric_limits<double>::infinity(),
-            RunScriptExtractDouble("getDownlink()"));
+  EXPECT_EQ(0, RunScriptExtractInt("getRtt()"));

[nasko, 2017/06/01 22:16:12]

Why can't ExecuteScriptAndExtractInt be used?

[tbansal1, 2017/06/01 23:26:46]

Can be. RunScriptExtractInt() is slightly easier to use since it directly
returns the int value.

+  EXPECT_EQ(0, RunScriptExtractInt("getRtt()") % 25);
+
+  double downlink_mbps = RunScriptExtractDouble("getDownlink()");

[nasko, 2017/06/01 22:16:13]

Similarly ExecuteScriptAndExtractDouble?

[tbansal1, 2017/06/01 23:26:46]

Same as above. Helper method RunScriptExtractDouble() makes the code slightly
more readable here.

+  EXPECT_LE(0, downlink_mbps);
+
+  double fraction_part, int_part;
+  fraction_part = std::modf(downlink_mbps, &int_part);
+  // If |fraction_part| is 0, it implies |downlink_mbps| is a multiple of 1
+  // Mbps, and hence it is a multiple of 25 kbps.
+  EXPECT_TRUE(static_cast<int>(downlink_mbps * 1000) % 25 == 0 ||

[nasko, 2017/06/01 22:16:12]

Shouldn't we be dividing the mbps to get kbps instead of multiplying it to get
gbps?

[tbansal1, 2017/06/01 23:26:46]

nope, multiplying it by 1000 would give kbps.
We want to verify if getDownlink() is a multiple of 0.025 mbps (i.e., 25 kbps).
So, e.g., if getDownlink() returns 1.275, then we multiple by 1000 and then take
mod 25.

[nasko, 2017/06/03 00:09:47]

Well, when you have mbps and multiply by 1000, you get gbps, don't you? Maybe we
are using different terminology.
That statement will always be true though. X * 1000 % 25 is guaranteed to be 0.
It is so, be cause it can be represented as (X * 40 * 25) % 25. Any multiple of
25 will be 0 mod 25.

[tbansal1, 2017/06/05 13:52:20]

We probably are.
|downlink_mbps| is a double. Let's say calling the netinfo API returns that
bandwidth is 1.23456 mbps. 
So, |downlink_mbps| would be set to 1.23456. If the caller wants to know the
speed in kbps, it needs to multiple |downlink_mbps| by 1000, which will give us
1234.56 kbps (same as 1.23456 mbps).
Right but only if X was a int which is not the case here. Here, X is a double.
e.g., if X is 1.23456, then 
static_cast<int>(X * 1000)  will be 1234 which is not a multiple of 25.
Let me know if you have suggestions for improving the readability. Sorry for the
confusion.

[nasko, 2017/06/05 23:43:01]

Thanks for the explanation. If you were to only multiply the fractional part, it
probably would've been a bit more clear, as 1mb is always multiple of 25kb.
Also, including in a comment an example is always helpful to see.

+              fraction_part == 0);
+
   EXPECT_EQ("4g", RunScriptExtractString("getEffectiveType()"));
 }
 
 // Make sure the changes in the effective connection typeare notified to the
 // render thread.
 IN_PROC_BROWSER_TEST_F(NetInfoBrowserTest,
                        EffectiveConnectionTypeChangeNotfied) {
   base::HistogramTester histogram_tester;
   net::TestNetworkQualityEstimator estimator(
       nullptr, std::map<std::string, std::string>(), false, false, true, true,
@@ skipping 49 matching lines @@
 
   EXPECT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(
       NavigateToURL(shell(), embedded_test_server()->GetURL("/net_info.html")));
 
   FetchHistogramsFromChildProcesses();
   EXPECT_FALSE(
       histogram_tester.GetAllSamples("NQE.RenderThreadNotified").empty());
 
   EXPECT_EQ(network_quality_1.transport_rtt().InMilliseconds(),
-            RunScriptExtractDouble("getRtt()"));
+            RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(
       static_cast<double>(network_quality_1.downstream_throughput_kbps()) /
           1000,
       RunScriptExtractDouble("getDownlink()"));
 
   // Verify that the network quality change is accessible via Javascript API.
   net::nqe::internal::NetworkQuality network_quality_2(
       base::TimeDelta::FromSeconds(10), base::TimeDelta::FromSeconds(20), 3000);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_2);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(network_quality_2.transport_rtt().InMilliseconds(),
-            RunScriptExtractDouble("getRtt()"));
+            RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(
       static_cast<double>(network_quality_2.downstream_throughput_kbps()) /
           1000,
       RunScriptExtractDouble("getDownlink()"));
 }
 
 // Make sure the changes in the network quality are rounded to the nearest
 // 25 milliseconds or 25 kbps.
 IN_PROC_BROWSER_TEST_F(NetInfoBrowserTest, NetworkQualityChangeRounded) {
   base::HistogramTester histogram_tester;
   net::TestNetworkQualityEstimator estimator(
       std::unique_ptr<net::ExternalEstimateProvider>(),
       std::map<std::string, std::string>(), false, false, true, true,
       base::MakeUnique<net::BoundTestNetLog>());
   NetworkQualityObserverImpl impl(&estimator);
 
   // Verify that the network quality is rounded properly.
   net::nqe::internal::NetworkQuality network_quality_1(
       base::TimeDelta::FromMilliseconds(123),
       base::TimeDelta::FromMilliseconds(212), 303);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_1);
 
   EXPECT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(
       NavigateToURL(shell(), embedded_test_server()->GetURL("/net_info.html")));
-  EXPECT_EQ(200, RunScriptExtractDouble("getRtt()"));
+  EXPECT_EQ(200, RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(0.300, RunScriptExtractDouble("getDownlink()"));
 
   net::nqe::internal::NetworkQuality network_quality_2(
       base::TimeDelta::FromMilliseconds(123),
       base::TimeDelta::FromMilliseconds(1217), 1317);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_2);
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1225, RunScriptExtractDouble("getRtt()"));
+  EXPECT_EQ(1225, RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(1.325, RunScriptExtractDouble("getDownlink()"));
 
   net::nqe::internal::NetworkQuality network_quality_3(
       base::TimeDelta::FromMilliseconds(12),
       base::TimeDelta::FromMilliseconds(12), 12);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_3);
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(0, RunScriptExtractDouble("getRtt()"));
+  EXPECT_EQ(0, RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(0, RunScriptExtractDouble("getDownlink()"));
 }
 
 // Make sure the minor changes (<10%) in the network quality are not notified.
 IN_PROC_BROWSER_TEST_F(NetInfoBrowserTest, NetworkQualityChangeNotNotified) {
   base::HistogramTester histogram_tester;
   net::TestNetworkQualityEstimator estimator(
       nullptr, std::map<std::string, std::string>(), false, false, true, true,
       base::MakeUnique<net::BoundTestNetLog>());
   NetworkQualityObserverImpl impl(&estimator);
 
   // Verify that the network quality is rounded properly.
   net::nqe::internal::NetworkQuality network_quality_1(
       base::TimeDelta::FromMilliseconds(1123),
       base::TimeDelta::FromMilliseconds(1212), 1303);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_1);
 
   EXPECT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(
       NavigateToURL(shell(), embedded_test_server()->GetURL("/net_info.html")));
-  EXPECT_EQ(1200, RunScriptExtractDouble("getRtt()"));
+  EXPECT_EQ(1200, RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(1.300, RunScriptExtractDouble("getDownlink()"));
 
   // All the 3 metrics change by less than 10%. So, the observers are not
   // notified.
   net::nqe::internal::NetworkQuality network_quality_2(
       base::TimeDelta::FromMilliseconds(1223),
       base::TimeDelta::FromMilliseconds(1312), 1403);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_2);
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1200, RunScriptExtractDouble("getRtt()"));
+  EXPECT_EQ(1200, RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(1.300, RunScriptExtractDouble("getDownlink()"));
 
   // Transport RTT has changed by more than 10% from the last notified value of
   // |network_quality_1|. The observers should be notified.
   net::nqe::internal::NetworkQuality network_quality_3(
       base::TimeDelta::FromMilliseconds(1223),
       base::TimeDelta::FromMilliseconds(2312), 1403);
   estimator.NotifyObserversOfRTTOrThroughputEstimatesComputed(
       network_quality_3);
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(2300, RunScriptExtractDouble("getRtt()"));
+  EXPECT_EQ(2300, RunScriptExtractInt("getRtt()"));
   EXPECT_EQ(1.400, RunScriptExtractDouble("getDownlink()"));
 }
 
 }  // namespace content