Fix a bug in ImplicitWaitCommand: we must explicitly check if the ms param

was sent as a floating point number after checking for an integer.
Patch by jleyba@chromium.org.
Original review at http://codereview.chromium.org/6532068
BUG=none
TEST=none

Review URL: http://codereview.chromium.org/6541043

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75508 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 136 insertions, 28 deletions

diff --git a/chrome/test/webdriver/commands/command.cc b/chrome/test/webdriver/commands/command.cc
index b333e1f..67e1623 100644
--- a/chrome/test/webdriver/commands/command.cc
+++ b/chrome/test/webdriver/commands/command.cc
@@ -29,6 +29,10 @@ bool Command::Init(Response* const response) {
   return true;
 }
 
+bool Command::HasParameter(const std::string& key) const {
+  return parameters_.get() && parameters_->HasKey(key);
+}
+
 bool Command::IsNullParameter(const std::string& key) const {
   Value* value;
   return parameters_.get() &&
@@ -61,6 +65,10 @@ bool Command::GetIntegerParameter(const std::string& key,
   return parameters_.get() != NULL && parameters_->GetInteger(key, out);
 }
 
+bool Command::GetDoubleParameter(const std::string& key, double* out) const {
+  return parameters_.get() != NULL && parameters_->GetDouble(key, out);
+}
+
 bool Command::GetDictionaryParameter(const std::string& key,
                                      DictionaryValue** out) const {
   return parameters_.get() != NULL && parameters_->GetDictionary(key, out);
diff --git a/chrome/test/webdriver/commands/command.h b/chrome/test/webdriver/commands/command.h
index 7e7e764..2529d58 100644
--- a/chrome/test/webdriver/commands/command.h
+++ b/chrome/test/webdriver/commands/command.h
@@ -53,6 +53,9 @@ class Command {
     return i < path_segments_.size() ? path_segments_.at(i) : "";
   }
 
+  // Returns whether the command has a parameter with the given |key|.
+  bool HasParameter(const std::string& key) const;
+
   // Returns true if the command parameter with the given |key| exists and is
   // a null value.
   bool IsNullParameter(const std::string& key) const;
@@ -77,6 +80,10 @@ class Command {
   // false if there is no such parameter, or if it is not a int.
   bool GetIntegerParameter(const std::string& key, int* out) const;
 
+  // Provides the command parameter with the given |key| as a double. Returns
+  // false if there is no such parameter, or if it is not a dobule.
+  bool GetDoubleParameter(const std::string& key, double* out) const;
+
   // Provides the command parameter with the given |key| as a Dictionary.
   // Returns false if there is no such parameter, or if it is not a Dictionary.
   bool GetDictionaryParameter(const std::string& key,
diff --git a/chrome/test/webdriver/commands/implicit_wait_command.cc b/chrome/test/webdriver/commands/implicit_wait_command.cc
index c115936..484f53d 100644
--- a/chrome/test/webdriver/commands/implicit_wait_command.cc
+++ b/chrome/test/webdriver/commands/implicit_wait_command.cc
@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/test/webdriver/commands/response.h"
 
@@ -14,42 +15,45 @@ namespace webdriver {
 ImplicitWaitCommand::ImplicitWaitCommand(
     const std::vector<std::string>& path_segments,
     const DictionaryValue* const parameters)
-    : WebDriverCommand(path_segments, parameters),
-      ms_to_wait_(0) {}
+    : WebDriverCommand(path_segments, parameters) {}
 
 ImplicitWaitCommand::~ImplicitWaitCommand() {}
 
-bool ImplicitWaitCommand::Init(Response* const response) {
-  if (!(WebDriverCommand::Init(response))) {
-    SET_WEBDRIVER_ERROR(response, "Failure on Init for find element",
-                        kInternalServerError);
-    return false;
-  }
-
-  // Record the requested wait time.
-  if (!GetIntegerParameter("ms", &ms_to_wait_)) {
-    SET_WEBDRIVER_ERROR(response, "Request missing ms parameter",
-                        kBadRequest);
-    return false;
-  }
-
-  return true;
-}
-
 bool ImplicitWaitCommand::DoesPost() {
   return true;
 }
 
 void ImplicitWaitCommand::ExecutePost(Response* const response) {
+  if (!HasParameter("ms")) {
+    SET_WEBDRIVER_ERROR(response, "Request missing ms parameter", kBadRequest);
+    return;
+  }
+
+  int ms_to_wait;
+  if (!GetIntegerParameter("ms", &ms_to_wait)) {
+    // Client may have sent us a floating point number. Since DictionaryValue
+    // will not do a down cast for us, we must explicitly check for it here.
+    // Note webdriver only supports whole milliseconds for a timeout value, so
+    // we are safe to downcast.
+    double ms;
+    if (!GetDoubleParameter("ms", &ms)) {
+      SET_WEBDRIVER_ERROR(response, "ms parameter is not a number",
+                          kBadRequest);
+      return;
+    }
+    ms_to_wait = static_cast<int>(ms);
+  }
+
   // Validate the wait time before setting it to the session.
-  if (ms_to_wait_ < 0) {
-    SET_WEBDRIVER_ERROR(response, "Wait must be non-negative",
-                        kBadRequest);
+  if (ms_to_wait < 0) {
+    SET_WEBDRIVER_ERROR(response,
+        base::StringPrintf("Wait must be non-negative: %d", ms_to_wait).c_str(),
+        kBadRequest);
     return;
   }
 
-  session_->set_implicit_wait(ms_to_wait_);
-  LOG(INFO) << "Implicit wait set to: " << ms_to_wait_ << " ms";
+  session_->set_implicit_wait(ms_to_wait);
+  LOG(INFO) << "Implicit wait set to: " << ms_to_wait << " ms";
 
   response->SetValue(new StringValue("success"));
   response->SetStatus(kSuccess);
diff --git a/chrome/test/webdriver/commands/implicit_wait_command.h b/chrome/test/webdriver/commands/implicit_wait_command.h
index f4e6d3a..24eba1c 100644
--- a/chrome/test/webdriver/commands/implicit_wait_command.h
+++ b/chrome/test/webdriver/commands/implicit_wait_command.h
@@ -25,13 +25,10 @@ class ImplicitWaitCommand : public WebDriverCommand {
                       const DictionaryValue* const parameters);
   virtual ~ImplicitWaitCommand();
 
-  virtual bool Init(Response* const response);
-
   virtual bool DoesPost();
   virtual void ExecutePost(Response* const response);
 
  private:
-  int ms_to_wait_;
   virtual bool RequiresValidTab();
 
   DISALLOW_COPY_AND_ASSIGN(ImplicitWaitCommand);
diff --git a/chrome/test/webdriver/commands/implicit_wait_command_unittest.cc b/chrome/test/webdriver/commands/implicit_wait_command_unittest.cc
new file mode 100644
index 0000000..b0068d7
--- /dev/null
+++ b/chrome/test/webdriver/commands/implicit_wait_command_unittest.cc
@@ -0,0 +1,91 @@
+// Copyright (c) 2011 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 <string>
+#include <vector>
+
+#include "base/scoped_ptr.h"
+#include "base/values.h"
+#include "chrome/test/webdriver/commands/implicit_wait_command.h"
+#include "chrome/test/webdriver/commands/response.h"
+#include "chrome/test/webdriver/error_codes.h"
+#include "chrome/test/webdriver/session.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace webdriver {
+
+namespace {
+
+void AssertIsAbleToInitialize(ImplicitWaitCommand* const command) {
+  Response response;
+  EXPECT_TRUE(command->Init(&response));
+  ASSERT_EQ(kSuccess, response.status()) << response.ToJSON();
+}
+
+void AssertError(ErrorCode expected_status,
+                 const std::string& expected_message,
+                 ImplicitWaitCommand* const command) {
+  Response response;
+  command->ExecutePost(&response);
+  ASSERT_EQ(expected_status,  response.status()) << response.ToJSON();
+
+  const Value* value = response.value();
+  ASSERT_TRUE(value->IsType(Value::TYPE_DICTIONARY));
+
+  const DictionaryValue* dict = static_cast<const DictionaryValue*>(value);
+  std::string actual_message;
+  ASSERT_TRUE(dict->GetString("message", &actual_message));
+  ASSERT_EQ(expected_message, actual_message);
+}
+
+void AssertTimeoutSet(const Session& test_session, int expected_timeout,
+                      ImplicitWaitCommand* const command) {
+  Response response;
+  command->ExecutePost(&response);
+  ASSERT_EQ(kSuccess, response.status()) << response.ToJSON();
+  ASSERT_EQ(expected_timeout, test_session.implicit_wait());
+}
+
+}  // namespace
+
+TEST(ImplicitWaitCommandTest, SettingImplicitWaits) {
+  Session test_session;
+  ASSERT_EQ(0, test_session.implicit_wait()) << "Sanity check failed";
+
+  std::vector<std::string> path_segments;
+  path_segments.push_back("");
+  path_segments.push_back("session");
+  path_segments.push_back(test_session.id());
+  path_segments.push_back("timeouts");
+  path_segments.push_back("implicitly_wait");
+
+  DictionaryValue* parameters = new DictionaryValue;  // Owned by |command|.
+  ImplicitWaitCommand command(path_segments, parameters);
+
+  AssertIsAbleToInitialize(&command);
+
+  AssertError(kBadRequest, "Request missing ms parameter", &command);
+
+  parameters->Set("ms", Value::CreateNullValue());
+  AssertError(kBadRequest, "ms parameter is not a number", &command);
+
+  parameters->SetString("ms", "apples");
+  AssertError(kBadRequest, "ms parameter is not a number", &command);
+
+  parameters->SetInteger("ms", -1);
+  AssertError(kBadRequest, "Wait must be non-negative: -1", &command);
+
+  parameters->SetDouble("ms", -3.0);
+  AssertError(kBadRequest, "Wait must be non-negative: -3", &command);
+
+  parameters->SetInteger("ms", 1);
+  AssertTimeoutSet(test_session, 1, &command);
+  parameters->SetInteger("ms", 2.5);
+  AssertTimeoutSet(test_session, 2, &command);
+  parameters->SetInteger("ms", 0);
+  AssertTimeoutSet(test_session, 0, &command);
+}
+
+}  // namespace webdriver
+
diff --git a/chrome/test/webdriver/session.h b/chrome/test/webdriver/session.h
index 99c8f71..b67fad5 100644
--- a/chrome/test/webdriver/session.h
+++ b/chrome/test/webdriver/session.h
@@ -7,6 +7,7 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/scoped_ptr.h"
 #include "base/string16.h"
@@ -98,7 +99,7 @@ class Session {
 
   inline const std::string& id() const { return id_; }
 
-  inline int implicit_wait() { return implicit_wait_; }
+  inline int implicit_wait() const { return implicit_wait_; }
   inline void set_implicit_wait(const int& timeout) {
     implicit_wait_ = timeout > 0 ? timeout : 0;
   }