Skip to content

Commit

Permalink
Fixing getElementProperty to return proper data type.
Browse files Browse the repository at this point in the history 
Previously, the getElementProperty command would coerce the property
value to a string. This is not correct behavior as specified in the
W3C spec.
  • Loading branch information
@jimevans
jimevans committed Mar 6, 2018
1 parent 6d505cc commit b9b2f22
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 112 deletions.
21 changes: 10 additions & 11 deletions cpp/iedriver/CommandHandlers/GetElementAttributeCommandHandler.cpp
View file
Expand Up @@ -19,6 +19,7 @@
#include "../Browser.h"
#include "../Element.h"
#include "../IECommandExecutor.h"
#include "../VariantUtilities.h"

namespace webdriver {

Expand Down Expand Up @@ -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);
Expand Down
19 changes: 8 additions & 11 deletions cpp/iedriver/CommandHandlers/GetElementPropertyCommandHandler.cpp
View file
Expand Up @@ -19,6 +19,7 @@
#include "../Browser.h"
#include "../Element.h"
#include "../IECommandExecutor.h"
#include "../VariantUtilities.h"

namespace webdriver {

Expand Down Expand Up @@ -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);
Expand Down
17 changes: 13 additions & 4 deletions cpp/iedriver/CommandHandlers/GetElementTextCommandHandler.cpp
View file
Expand Up @@ -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) {
Expand Down
42 changes: 16 additions & 26 deletions cpp/iedriver/Element.cpp
View file
Expand Up @@ -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);
Expand All @@ -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";
}
Expand All @@ -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);
Expand All @@ -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;
}

Expand All @@ -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";
}
Expand Down Expand Up @@ -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";
Expand All @@ -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 ||
Expand Down
6 changes: 2 additions & 4 deletions cpp/iedriver/Element.h
View file
Expand Up @@ -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);

Expand Down
5 changes: 0 additions & 5 deletions cpp/iedriver/Script.cpp
View file
Expand Up @@ -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";

Expand Down
1 change: 0 additions & 1 deletion cpp/iedriver/Script.h
View file
Expand Up @@ -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; }
Expand Down
48 changes: 0 additions & 48 deletions cpp/iedriver/VariantUtilities.cpp
View file
Expand Up @@ -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) {
Expand Down
2 changes: 0 additions & 2 deletions cpp/iedriver/VariantUtilities.h
View file
Expand Up @@ -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);
Expand Down

0 comments on commit b9b2f22

Please sign in to comment.