Fixing getElementProperty to return proper data type.

jimevans · jimevans · commit b9b2f22fa177 · 2018-03-06T08:42:05.000-08:00
Previously, the getElementProperty command would coerce the property
value to a string. This is not correct behavior as specified in the
W3C spec.
diff --git a/cpp/iedriver/CommandHandlers/GetElementAttributeCommandHandler.cpp b/cpp/iedriver/CommandHandlers/GetElementAttributeCommandHandler.cpp
@@ -19,6 +19,7 @@
 #include "../Browser.h"
 #include "../Element.h"
 #include "../IECommandExecutor.h"
+#include "../VariantUtilities.h"
 
 namespace webdriver {
 
@@ -54,22 +55,20 @@ void GetElementAttributeCommandHandler::ExecuteInternal(
     ElementHandle element_wrapper;
     status_code = this->GetElement(executor, element_id, &element_wrapper);
     if (status_code == WD_SUCCESS) {
-      std::string value = "";
-      bool is_null;
+      CComVariant attribute_value;
+      IECommandExecutor& mutable_executor = const_cast<IECommandExecutor&>(executor);
+      Json::Value value;
       status_code = element_wrapper->GetAttributeValue(name,
-        &value,
-        &is_null);
+                                                       &attribute_value);
+      VariantUtilities::VariantAsJsonValue(mutable_executor.element_manager(),
+                                           attribute_value,
+                                           &value);
       if (status_code != WD_SUCCESS) {
         response->SetErrorResponse(status_code, "Unable to get attribute");
         return;
       } else {
-        if (is_null) {
-          response->SetSuccessResponse(Json::Value::null);
-          return;
-        } else {
-          response->SetSuccessResponse(value);
-          return;
-        }
+        response->SetSuccessResponse(value);
+        return;
       }
     } else if (status_code == ENOSUCHELEMENT) {
       response->SetErrorResponse(ERROR_NO_SUCH_ELEMENT, "Invalid internal element ID requested: " + element_id);
diff --git a/cpp/iedriver/CommandHandlers/GetElementPropertyCommandHandler.cpp b/cpp/iedriver/CommandHandlers/GetElementPropertyCommandHandler.cpp
@@ -19,6 +19,7 @@
 #include "../Browser.h"
 #include "../Element.h"
 #include "../IECommandExecutor.h"
+#include "../VariantUtilities.h"
 
 namespace webdriver {
 
@@ -54,22 +55,18 @@ void GetElementPropertyCommandHandler::ExecuteInternal(
     ElementHandle element_wrapper;
     status_code = this->GetElement(executor, element_id, &element_wrapper);
     if (status_code == WD_SUCCESS) {
-      std::string value = "";
-      bool is_null;
+      CComVariant value;
       status_code = element_wrapper->GetPropertyValue(name,
-                                                      &value,
-                                                      &is_null);
+                                                      &value);
       if (status_code != WD_SUCCESS) {
         response->SetErrorResponse(status_code, "Unable to get property");
         return;
       } else {
-        if (is_null) {
-          response->SetSuccessResponse(Json::Value::null);
-          return;
-        } else {
-          response->SetSuccessResponse(value);
-          return;
-        }
+        Json::Value json_value;
+        IECommandExecutor& mutable_executor = const_cast<IECommandExecutor&>(executor);
+        VariantUtilities::VariantAsJsonValue(mutable_executor.element_manager(), value, &json_value);
+        response->SetSuccessResponse(json_value);
+        return;
       }
     } else if (status_code == ENOSUCHELEMENT) {
       response->SetErrorResponse(ERROR_NO_SUCH_ELEMENT, "Invalid internal element ID requested: " + element_id);
diff --git a/cpp/iedriver/CommandHandlers/GetElementTextCommandHandler.cpp b/cpp/iedriver/CommandHandlers/GetElementTextCommandHandler.cpp
@@ -66,13 +66,22 @@ void GetElementTextCommandHandler::ExecuteInternal(
       status_code = script_wrapper.Execute();
 
       if (status_code == WD_SUCCESS) {
-        std::string text = "";
-        bool is_null = script_wrapper.ConvertResultToString(&text);
-        response->SetSuccessResponse(text);
+        Json::Value text_value;
+        IECommandExecutor& mutable_executor = const_cast<IECommandExecutor&>(executor);
+        int is_null = script_wrapper.ConvertResultToJsonValue(mutable_executor.element_manager(), &text_value);
+        if (!text_value.isString()) {
+          // This really should never happen, since we're executing an atom
+          // over which we have complete control. Nevertheless, check for
+          // the error here, just in case.
+          response->SetErrorResponse(ERROR_JAVASCRIPT_ERROR,
+                                     "Atom retrieving text was executed, but did not return a string");
+          return;
+        }
+        response->SetSuccessResponse(text_value.asString());
         return;
       } else {
         response->SetErrorResponse(status_code,
-                                    "Unable to get element text");
+                                   "Unable to get element text");
         return;
       }
     } else if (status_code == ENOSUCHELEMENT) {
diff --git a/cpp/iedriver/Element.cpp b/cpp/iedriver/Element.cpp
@@ -246,8 +246,7 @@ int Element::GetClickLocation(const ElementScrollBehavior scroll_behavior,
 }
 
 int Element::GetAttributeValue(const std::string& attribute_name,
-                               std::string* attribute_value,
-                               bool* value_is_null) {
+                               VARIANT* attribute_value) {
   LOG(TRACE) << "Entering Element::GetAttributeValue";
 
   std::wstring wide_attribute_name = StringUtilities::ToWString(attribute_name);
@@ -268,7 +267,7 @@ int Element::GetAttributeValue(const std::string& attribute_name,
   status_code = script_wrapper.Execute();
   
   if (status_code == WD_SUCCESS) {
-    *value_is_null = !script_wrapper.ConvertResultToString(attribute_value);
+    *attribute_value = script_wrapper.result();
   } else {
     LOG(WARN) << "Failed to determine element attribute";
   }
@@ -277,8 +276,7 @@ int Element::GetAttributeValue(const std::string& attribute_name,
 }
 
 int Element::GetPropertyValue(const std::string& property_name,
-                              std::string* property_value,
-                              bool* value_is_null) {
+                              VARIANT* property_value) {
   LOG(TRACE) << "Entering Element::GetPropertyValue";
 
   std::wstring wide_property_name = StringUtilities::ToWString(property_name);
@@ -294,36 +292,27 @@ int Element::GetPropertyValue(const std::string& property_name,
   if (FAILED(hr)) {
     LOGHR(WARN, hr) << "Unable to get dispatch ID (dispid) for property "
                     << property_name;
-    *property_value = "";
-    *value_is_null = true;
+    property_value->vt = VT_EMPTY;
     return WD_SUCCESS;
   }
 
   // get the value of eval result
-  CComVariant property_value_variant;
   DISPPARAMS no_args_dispatch_parameters = { 0 };
   hr = this->element_->Invoke(dispid_property,
                               IID_NULL,
                               LOCALE_USER_DEFAULT,
                               DISPATCH_PROPERTYGET,
                               &no_args_dispatch_parameters,
-                              &property_value_variant,
+                              property_value,
                               NULL,
                               NULL);
   if (FAILED(hr)) {
     LOGHR(WARN, hr) << "Unable to get result for property "
                     << property_name;
-    *property_value = "";
-    *value_is_null = true;
+    property_value->vt = VT_EMPTY;
     return WD_SUCCESS;
   }
 
-  if (status_code == WD_SUCCESS) {
-    *value_is_null = !VariantUtilities::VariantAsString(property_value_variant, property_value);
-  } else {
-    LOG(WARN) << "Failed to determine element attribute";
-  }
-
   return WD_SUCCESS;
 }
 
@@ -347,13 +336,13 @@ int Element::GetCssPropertyValue(const std::string& property_name,
   status_code = script_wrapper.Execute();
 
   if (status_code == WD_SUCCESS) {
-    std::string raw_value = "";
-    script_wrapper.ConvertResultToString(&raw_value);
+    std::wstring raw_value(script_wrapper.result().bstrVal);
+    std::string value = StringUtilities::ToString(raw_value);
     std::transform(raw_value.begin(),
-                    raw_value.end(),
-                    raw_value.begin(),
-                    tolower);
-    *property_value = raw_value;
+                   raw_value.end(),
+                   raw_value.begin(),
+                   tolower);
+    *property_value = value;
   } else {
     LOG(WARN) << "Failed to get value of CSS property";
   }
@@ -444,8 +433,8 @@ bool Element::IsHiddenByOverflow() {
   script_wrapper.AddArgument(this->element_);
   int status_code = script_wrapper.Execute();
   if (status_code == WD_SUCCESS) {
-    std::string overflow_state = "";
-    script_wrapper.ConvertResultToString(&overflow_state);
+    std::wstring raw_overflow_state(script_wrapper.result().bstrVal);
+    std::string overflow_state = StringUtilities::ToString(raw_overflow_state);
     isOverflow = (overflow_state == "scroll");
   } else {
     LOG(WARN) << "Unable to determine is element hidden by overflow";
@@ -454,7 +443,8 @@ bool Element::IsHiddenByOverflow() {
   return isOverflow;
 }
 
-bool Element::IsLocationVisibleInFrames(const LocationInfo location, const std::vector<LocationInfo> frame_locations) {
+bool Element::IsLocationVisibleInFrames(const LocationInfo location,
+                                        const std::vector<LocationInfo> frame_locations) {
   std::vector<LocationInfo>::const_iterator iterator = frame_locations.begin();
   for (; iterator != frame_locations.end(); ++iterator) {
     if (location.x < iterator->x || 
diff --git a/cpp/iedriver/Element.h b/cpp/iedriver/Element.h
@@ -52,11 +52,9 @@ class Element {
                        LocationInfo* element_location,
                        LocationInfo* click_location);
   int GetAttributeValue(const std::string& attribute_name,
-                        std::string* attribute_value,
-                        bool* value_is_null);
+                        VARIANT* attribute_value);
   int GetPropertyValue(const std::string& property_name,
-                       std::string* property_value,
-                       bool* value_is_null);
+                       VARIANT* property_value);
   int GetCssPropertyValue(const std::string& property_name,
                           std::string* property_value);
 
diff --git a/cpp/iedriver/Script.cpp b/cpp/iedriver/Script.cpp
@@ -555,11 +555,6 @@ int Script::ConvertResultToJsonValue(IElementManager* element_manager,
                                               value);
 }
 
-bool Script::ConvertResultToString(std::string* value) {
-  LOG(TRACE) << "Entering Script::ConvertResultToString";
-  return VariantUtilities::VariantAsString(this->result_, value);
-}
-
 bool Script::CreateAnonymousFunction(VARIANT* result) {
   LOG(TRACE) << "Entering Script::CreateAnonymousFunction";
 
diff --git a/cpp/iedriver/Script.h b/cpp/iedriver/Script.h
@@ -101,7 +101,6 @@ class Script {
                                Json::Value* value);
   int ConvertResultToJsonValue(IElementManager* element_manager,
                                Json::Value* value);
-  bool ConvertResultToString(std::string* value);
 
   std::wstring polling_source_code(void) const { return this->polling_source_code_; }
   void set_polling_source_code(const std::wstring& value) { this->polling_source_code_ = value; }
diff --git a/cpp/iedriver/VariantUtilities.cpp b/cpp/iedriver/VariantUtilities.cpp
@@ -122,54 +122,6 @@ bool VariantUtilities::VariantIsObject(VARIANT value) {
   return false;
 }
 
-bool VariantUtilities::VariantAsString(VARIANT variant_value,
-                                       std::string* value) {
-  VARTYPE type = variant_value.vt;
-  switch (type) {
-
-  case VT_BOOL:
-    LOG(DEBUG) << "Result type is boolean";
-    *value = variant_value.boolVal == VARIANT_TRUE ? "true" : "false";
-    return true;
-
-  case VT_BSTR:
-    LOG(DEBUG) << "Result type is string";
-    if (!variant_value.bstrVal) {
-      *value = "";
-    } else {
-      std::wstring str_value = variant_value.bstrVal;
-      *value = StringUtilities::ToString(str_value);
-    }
-    return true;
-
-  case VT_I4:
-    LOG(DEBUG) << "Result type is int";
-    *value = std::to_string(static_cast<long long>(variant_value.lVal));
-    return true;
-
-  case VT_R4:
-    LOG(DEBUG) << "Result type is real";
-    *value = std::to_string(variant_value.dblVal);
-    return true;
-
-  case VT_EMPTY:
-  case VT_NULL:
-    LOG(DEBUG) << "Result type is empty";
-    *value = "";
-    return false;
-
-    // This is lame
-  case VT_DISPATCH:
-    LOG(DEBUG) << "Result type is dispatch";
-    *value = "";
-    return true;
-
-  default:
-    LOG(DEBUG) << "Result type is unknown: " << type;
-  }
-  return false;
-}
-
 int VariantUtilities::VariantAsJsonValue(IElementManager* element_manager,
                                          VARIANT variant_value,
                                          Json::Value* value) {
diff --git a/cpp/iedriver/VariantUtilities.h b/cpp/iedriver/VariantUtilities.h
@@ -51,8 +51,6 @@ class VariantUtilities {
   static int VariantAsJsonValue(IElementManager* element_manager,
                                 VARIANT variant_value,
                                 Json::Value* value);
-  static bool VariantAsString(VARIANT variant_value,
-                              std::string* value);
   static bool GetVariantObjectPropertyValue(IDispatch* variant_object,
                                             std::wstring property_name,
                                             VARIANT* property_value);
