[chromedriver] Add Network Presets to Network Throttling feature

chrome/test/chromedriver/window_commands.cc

 // Copyright (c) 2013 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 "chrome/test/chromedriver/window_commands.h"
 
 #include <list>
 #include <string>
 
 #include "base/callback.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/test/chromedriver/basic_types.h"
 #include "chrome/test/chromedriver/chrome/automation_extension.h"
 #include "chrome/test/chromedriver/chrome/chrome.h"
 #include "chrome/test/chromedriver/chrome/chrome_desktop_impl.h"
 #include "chrome/test/chromedriver/chrome/devtools_client.h"
 #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"
 #include "chrome/test/chromedriver/element_util.h"
 #include "chrome/test/chromedriver/session.h"
 #include "chrome/test/chromedriver/util.h"
 
 namespace {
 
 Status GetMouseButton(const base::DictionaryValue& params,
@@ skipping 833 matching lines @@
   if (status.IsOk())
     session->overridden_geoposition.reset(new Geoposition(geoposition));
   return status;
 }
 
 Status ExecuteSetNetworkConditions(
     Session* session,
     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'");
+  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 '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) ||
-        !conditions->GetDouble("upload_throughput",
-                                &network_conditions.upload_throughput))
+    // 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 'download_throughput' or 'upload_throughput'");
+                    "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'");
+    } else {
+      network_conditions->offline = false;
+    }
   } 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'");
-  } else {
-    network_conditions.offline = false;
+                  "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);
 }
 
 Status ExecuteTakeHeapSnapshot(
     Session* session,
     WebView* web_view,
     const base::DictionaryValue& params,
     scoped_ptr<base::Value>* value) {
   return web_view->TakeHeapSnapshot(value);
 }