[chromedriver] Add Network Presets to Network Throttling feature

chrome/test/chromedriver/window_commands.cc

Index: chrome/test/chromedriver/window_commands.cc
diff --git a/chrome/test/chromedriver/window_commands.cc b/chrome/test/chromedriver/window_commands.cc
index 70273317d553390e4e8ecaa80c9ec82821227925..74072e54672611884cbc59fefa978eb7b876ec8b 100644
--- a/chrome/test/chromedriver/window_commands.cc
+++ b/chrome/test/chromedriver/window_commands.cc
@@ -21,6 +21,7 @@
 #include "chrome/test/chromedriver/chrome/geoposition.h"
 #include "chrome/test/chromedriver/chrome/javascript_dialog_manager.h"
 #include "chrome/test/chromedriver/chrome/js.h"
+#include "chrome/test/chromedriver/chrome/network_conditions.h"
 #include "chrome/test/chromedriver/chrome/status.h"
 #include "chrome/test/chromedriver/chrome/ui_events.h"
 #include "chrome/test/chromedriver/chrome/web_view.h"
@@ -874,44 +875,58 @@ Status ExecuteSetNetworkConditions(
     WebView* web_view,
     const base::DictionaryValue& params,
     scoped_ptr<base::Value>* value) {
+  std::string network_name;
   const base::DictionaryValue* conditions = NULL;
-  NetworkConditions network_conditions;
-  // |latency| is required.
-  if (!params.GetDictionary("network_conditions", &conditions) ||
-      !conditions->GetDouble("latency", &network_conditions.latency))
-    return Status(kUnknownError, "missing or invalid 'network_conditions'");
-
-  // Either |throughput| or the pair |download_throughput| and
-  // |upload_throughput| is required.
-  if (conditions->HasKey("throughput")) {
-    if (!conditions->GetDouble("throughput",
-                                &network_conditions.download_throughput))
-      return Status(kUnknownError, "invalid 'throughput'");
-    conditions->GetDouble("throughput", &network_conditions.upload_throughput);
-  } else if (conditions->HasKey("download_throughput") &&
-             conditions->HasKey("upload_throughput")) {
-    if (!conditions->GetDouble("download_throughput",
-                                &network_conditions.download_throughput) ||
-        !conditions->GetDouble("upload_throughput",
-                                &network_conditions.upload_throughput))
+  scoped_ptr<NetworkConditions> network_conditions;
+  if (params.GetString("network_name", &network_name)) {
+    // Get conditions from preset list.
+    Status status = FindPresetNetwork(network_name, &network_conditions);

[samuong, 2015/03/13 22:30:51]

I think your code would be simpler if you made FindPresetNetwork() take a raw
pointer, rather than a scoped_ptr.

Rather than allocate memory in network_conditions.cc:78 as well as
window_commands.cc:887, you could just do it once at window_commands.cc:880.

[srawlins, 2015/03/16 18:26:28]

Good call. Done.

[samuong, 2015/03/16 18:43:10]

Oh, I meant just change FindPresetNetwork() to use a raw pointer, but in this
function you should continue to use a scoped_ptr. Otherwise we'll leak the
NetworkConditions object if we error out mid-way through (e.g. if the params are
not in the right format).

[srawlins, 2015/03/16 19:23:11]

Done.

+    if (status.IsError())
+      return status;
+  } else if (params.GetDictionary("network_conditions", &conditions)) {
+    network_conditions.reset(new NetworkConditions());
+    // |latency| is required.
+    if (!conditions->GetDouble("latency", &network_conditions->latency))
       return Status(kUnknownError,
-                    "invalid 'download_throughput' or 'upload_throughput'");
-  } else {
-    return Status(kUnknownError,
-                  "invalid 'network_conditions' is missing 'throughput' or "
-                  "'download_throughput'/'upload_throughput' pair");
-  }
+                    "invalid 'network_conditions' is missing 'latency'");
+
+    // Either |throughput| or the pair |download_throughput| and
+    // |upload_throughput| is required.
+    if (conditions->HasKey("throughput")) {
+      if (!conditions->GetDouble("throughput",
+                                  &network_conditions->download_throughput))
+        return Status(kUnknownError, "invalid 'throughput'");
+      conditions->GetDouble("throughput",
+                            &network_conditions->upload_throughput);
+    } else if (conditions->HasKey("download_throughput") &&
+               conditions->HasKey("upload_throughput")) {
+      if (!conditions->GetDouble("download_throughput",
+                                  &network_conditions->download_throughput) ||

[samuong, 2015/03/13 22:30:51]

nit: unindent by 1 char

[srawlins, 2015/03/16 18:26:28]

Done.

+          !conditions->GetDouble("upload_throughput",
+                                  &network_conditions->upload_throughput))

[samuong, 2015/03/13 22:30:51]

nit: unindent by 1 char

[srawlins, 2015/03/16 18:26:28]

Done.

+        return Status(kUnknownError,
+                      "invalid 'download_throughput' or 'upload_throughput'");
+    } else {
+      return Status(kUnknownError,
+                    "invalid 'network_conditions' is missing 'throughput' or "
+                    "'download_throughput'/'upload_throughput' pair");
+    }
 
-  // |offline| is optional.
-  if (conditions->HasKey("offline")) {
-    if (!conditions->GetBoolean("offline", &network_conditions.offline))
-      return Status(kUnknownError, "invalid 'offline'");
+    // |offline| is optional.
+    if (conditions->HasKey("offline")) {
+      if (!conditions->GetBoolean("offline", &network_conditions->offline))
+        return Status(kUnknownError, "invalid 'offline'");
+    } else {
+      network_conditions->offline = false;
+    }
   } else {
-    network_conditions.offline = false;
+    return Status(kUnknownError,
+                  "either 'network_conditions' or 'network_name' must be "
+                  "supplied");
   }
 
   session->overridden_network_conditions.reset(
-      new NetworkConditions(network_conditions));
+      new NetworkConditions(*network_conditions.release()));
   return web_view->OverrideNetworkConditions(
       *session->overridden_network_conditions);
 }