Tidy fxjs/cjs_global.cpp
-- remove redundant local creation
-- convert to DCHECK()
-- remove magic numbers
-- consolidate string extraction code in fxv8.cpp
-- use 2-arg ByteString constructor (avoid strlen under covers).
-- remove a TODO that we won't do
Change-Id: I54f053425b35ec0c9bc3ae5a9356774312d3d433
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/76710
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
diff --git a/fxjs/cjs_global.cpp b/fxjs/cjs_global.cpp
index 3ea04bb..f6681a9 100644
--- a/fxjs/cjs_global.cpp
+++ b/fxjs/cjs_global.cpp
@@ -16,17 +16,13 @@
#include "fxjs/cjs_event_context.h"
#include "fxjs/cjs_eventrecorder.h"
#include "fxjs/cjs_object.h"
+#include "fxjs/fxv8.h"
#include "fxjs/js_define.h"
#include "fxjs/js_resources.h"
+#include "third_party/base/check.h"
namespace {
-WideString PropFromV8Prop(v8::Isolate* pIsolate,
- v8::Local<v8::String> property) {
- v8::String::Utf8Value utf8_value(pIsolate, property);
- return WideString::FromUTF8(ByteStringView(*utf8_value, utf8_value.length()));
-}
-
void JSSpecialPropQuery(v8::Local<v8::String> property,
const v8::PropertyCallbackInfo<v8::Integer>& info) {
auto pObj = JSGetObject<CJS_Global>(info.Holder());
@@ -37,10 +33,13 @@
if (!pRuntime)
return;
- CJS_Result result =
- pObj->QueryProperty(PropFromV8Prop(info.GetIsolate(), property).c_str());
+ WideString wsProp = fxv8::ToWideString(info.GetIsolate(), property);
+ CJS_Result result = pObj->QueryProperty(wsProp.c_str());
+ v8::PropertyAttribute attr = !result.HasError()
+ ? v8::PropertyAttribute::DontDelete
+ : v8::PropertyAttribute::None;
- info.GetReturnValue().Set(!result.HasError() ? 4 : 0);
+ info.GetReturnValue().Set(static_cast<int>(attr));
}
void JSSpecialPropGet(v8::Local<v8::String> property,
@@ -53,9 +52,8 @@
if (!pRuntime)
return;
- CJS_Result result = pObj->GetProperty(
- pRuntime, PropFromV8Prop(info.GetIsolate(), property).c_str());
-
+ WideString wsProp = fxv8::ToWideString(info.GetIsolate(), property);
+ CJS_Result result = pObj->GetProperty(pRuntime, wsProp.c_str());
if (result.HasError()) {
pRuntime->Error(
JSFormatErrorString("global", "GetProperty", result.Error()));
@@ -76,9 +74,8 @@
if (!pRuntime)
return;
- CJS_Result result = pObj->SetProperty(
- pRuntime, PropFromV8Prop(info.GetIsolate(), property).c_str(), value);
-
+ WideString wsProp = fxv8::ToWideString(info.GetIsolate(), property);
+ CJS_Result result = pObj->SetProperty(pRuntime, wsProp.c_str(), value);
if (result.HasError()) {
pRuntime->Error(
JSFormatErrorString("global", "PutProperty", result.Error()));
@@ -95,21 +92,13 @@
if (!pRuntime)
return;
- CJS_Result result = pObj->DelProperty(
- pRuntime, PropFromV8Prop(info.GetIsolate(), property).c_str());
-
- if (result.HasError()) {
- // TODO(dsinclair): Should this set the pRuntime->Error result?
- // pRuntime->Error(
- // JSFormatErrorString("global", "DelProperty", result.Error());
- }
+ WideString wsProp = fxv8::ToWideString(info.GetIsolate(), property);
+ pObj->DelProperty(pRuntime, wsProp.c_str()); // Silently ignore error.
}
-template <typename T>
-v8::Local<v8::String> GetV8StringFromProperty(v8::Local<v8::Name> property,
- const T& info) {
- return property->ToString(info.GetIsolate()->GetCurrentContext())
- .ToLocalChecked();
+v8::Local<v8::String> GetV8StringFromName(v8::Isolate* pIsolate,
+ v8::Local<v8::Name> pName) {
+ return pName->ToString(pIsolate->GetCurrentContext()).ToLocalChecked();
}
} // namespace
@@ -134,22 +123,16 @@
void CJS_Global::queryprop_static(
v8::Local<v8::Name> property,
const v8::PropertyCallbackInfo<v8::Integer>& info) {
- ASSERT(property->IsString());
- JSSpecialPropQuery(
- v8::Local<v8::String>::New(info.GetIsolate(),
- GetV8StringFromProperty(property, info)),
- info);
+ DCHECK(property->IsString());
+ JSSpecialPropQuery(GetV8StringFromName(info.GetIsolate(), property), info);
}
// static
void CJS_Global::getprop_static(
v8::Local<v8::Name> property,
const v8::PropertyCallbackInfo<v8::Value>& info) {
- ASSERT(property->IsString());
- JSSpecialPropGet(
- v8::Local<v8::String>::New(info.GetIsolate(),
- GetV8StringFromProperty(property, info)),
- info);
+ DCHECK(property->IsString());
+ JSSpecialPropGet(GetV8StringFromName(info.GetIsolate(), property), info);
}
// static
@@ -157,22 +140,17 @@
v8::Local<v8::Name> property,
v8::Local<v8::Value> value,
const v8::PropertyCallbackInfo<v8::Value>& info) {
- ASSERT(property->IsString());
- JSSpecialPropPut(
- v8::Local<v8::String>::New(info.GetIsolate(),
- GetV8StringFromProperty(property, info)),
- value, info);
+ DCHECK(property->IsString());
+ JSSpecialPropPut(GetV8StringFromName(info.GetIsolate(), property), value,
+ info);
}
// static
void CJS_Global::delprop_static(
v8::Local<v8::Name> property,
const v8::PropertyCallbackInfo<v8::Boolean>& info) {
- ASSERT(property->IsString());
- JSSpecialPropDel(
- v8::Local<v8::String>::New(info.GetIsolate(),
- GetV8StringFromProperty(property, info)),
- info);
+ DCHECK(property->IsString());
+ JSSpecialPropDel(GetV8StringFromName(info.GetIsolate(), property), info);
}
// static
diff --git a/fxjs/fxv8.cpp b/fxjs/fxv8.cpp
index bc7cdc0..551338d 100644
--- a/fxjs/fxv8.cpp
+++ b/fxjs/fxv8.cpp
@@ -109,6 +109,16 @@
.As<v8::Date>();
}
+WideString ToWideString(v8::Isolate* pIsolate, v8::Local<v8::String> pValue) {
+ v8::String::Utf8Value s(pIsolate, pValue);
+ return WideString::FromUTF8(ByteStringView(*s, s.length()));
+}
+
+ByteString ToByteString(v8::Isolate* pIsolate, v8::Local<v8::String> pValue) {
+ v8::String::Utf8Value s(pIsolate, pValue);
+ return ByteString(*s, s.length());
+}
+
int ReentrantToInt32Helper(v8::Isolate* pIsolate, v8::Local<v8::Value> pValue) {
if (pValue.IsEmpty())
return 0;
@@ -148,8 +158,7 @@
if (maybe_string.IsEmpty())
return WideString();
- v8::String::Utf8Value s(pIsolate, maybe_string.ToLocalChecked());
- return WideString::FromUTF8(ByteStringView(*s, s.length()));
+ return ToWideString(pIsolate, maybe_string.ToLocalChecked());
}
ByteString ReentrantToByteStringHelper(v8::Isolate* pIsolate,
@@ -163,8 +172,7 @@
if (maybe_string.IsEmpty())
return ByteString();
- v8::String::Utf8Value s(pIsolate, maybe_string.ToLocalChecked());
- return ByteString(*s);
+ return ToByteString(pIsolate, maybe_string.ToLocalChecked());
}
v8::Local<v8::Object> ReentrantToObjectHelper(v8::Isolate* pIsolate,
diff --git a/fxjs/fxv8.h b/fxjs/fxv8.h
index 00c5a8a..88ff38e 100644
--- a/fxjs/fxv8.h
+++ b/fxjs/fxv8.h
@@ -45,6 +45,11 @@
v8::Local<v8::Object> NewObjectHelper(v8::Isolate* pIsolate);
v8::Local<v8::Date> NewDateHelper(v8::Isolate* pIsolate, double d);
+// Conversion to PDFium type without re-entry from known v8 type.
+WideString ToWideString(v8::Isolate* pIsolate, v8::Local<v8::String> pValue);
+ByteString ToByteString(v8::Isolate* pIsolate, v8::Local<v8::String> pValue);
+
+// Conversion to PDFium type with possible re-entry for coercion.
int32_t ReentrantToInt32Helper(v8::Isolate* pIsolate,
v8::Local<v8::Value> pValue);
bool ReentrantToBooleanHelper(v8::Isolate* pIsolate,
