Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9089628
or point your web browser to
http://mondrian/9089628
to review the following code:
Change 9089628 by [EMAIL PROTECTED] on 2008/11/20 16:10:47 *pending*
Adds getters and setters for int64 to JsObject and JsArray.
These will be required for the Geolocation timeout option, which is
specified as an unsigned long in the W3C spec.
R=andreip
[EMAIL PROTECTED]
DELTA=184 (181 added, 0 deleted, 3 changed)
OCL=9089628
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_types.cc#52 edit
... //depot/googleclient/gears/opensource/gears/base/common/js_types.h#39 edit
... //depot/googleclient/gears/opensource/gears/cctests/test.cc#88 edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#42
edit
184 delta lines: 181 added, 0 deleted, 3 changed
Also consider running:
g4 lint -c 9089628
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 9089628 by [EMAIL PROTECTED] on 2008/11/20 16:10:47 *pending*
Adds getters and setters for int64 to JsObject and JsArray.
These will be required for the Geolocation timeout option, which is
specified as an unsigned long in the W3C spec.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_types.cc#52 edit
... //depot/googleclient/gears/opensource/gears/base/common/js_types.h#39 edit
... //depot/googleclient/gears/opensource/gears/cctests/test.cc#88 edit
...
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#42
edit
==== //depot/googleclient/gears/opensource/gears/base/common/js_types.cc#52 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/base/common/js_types.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_types.cc 2008-11-20
17:26:07.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/js_types.cc 2008-11-20
16:10:06.000000000 +0000
@@ -140,6 +140,7 @@
virtual bool GetElementAsBool(int index, bool *out) const;
virtual bool GetElementAsInt(int index, int *out) const;
+ virtual bool GetElementAsInt64(int index, int64 *out) const;
virtual bool GetElementAsDouble(int index, double *out) const;
virtual bool GetElementAsString(int index, std::string16 *out) const;
virtual bool GetElementAsArray(int index, JsArray **out) const;
@@ -153,6 +154,7 @@
virtual bool SetElementBool(int index, bool value);
virtual bool SetElementInt(int index, int value);
+ virtual bool SetElementInt64(int index, int64 value);
virtual bool SetElementDouble(int index, double value);
virtual bool SetElementString(int index, const std::string16 &value);
virtual bool SetElementArray(int index, JsArray *value);
@@ -423,6 +425,13 @@
return JsTokenToInt_NoCoerce(token, js_context_, out);
}
+bool JsArrayImpl::GetElementAsInt64(int index, int64 *out) const {
+ JsScopedToken token;
+ if (!GetElement(index, &token)) return false;
+
+ return JsTokenToInt64_NoCoerce(token, js_context_, out);
+}
+
bool JsArrayImpl::GetElementAsDouble(int index, double *out) const {
JsScopedToken token;
if (!GetElement(index, &token)) return false;
@@ -474,6 +483,14 @@
return JsTokenGetType(token, js_context_);
}
+bool JsArrayImpl::SetElementInt64(int index, int64 value) {
+ JsScopedToken jsval;
+ if (!Int64ToJsToken(js_context_, value, &jsval)) {
+ return false;
+ }
+ return SetElement(index, jsval);
+}
+
bool JsArrayImpl::SetElementArray(int index, JsArray *value) {
return SetElement(index, value->token());
}
@@ -526,6 +543,8 @@
bool *out) const;
virtual bool GetPropertyAsInt(const std::string16 &name,
int *out) const;
+ virtual bool GetPropertyAsInt64(const std::string16 &name,
+ int64 *out) const;
virtual bool GetPropertyAsDouble(const std::string16 &name,
double *out) const;
virtual bool GetPropertyAsString(const std::string16 &name,
@@ -543,6 +562,7 @@
virtual bool SetPropertyBool(const std::string16& name, bool value);
virtual bool SetPropertyInt(const std::string16 &name, int value);
+ virtual bool SetPropertyInt64(const std::string16 &name, int64 value);
virtual bool SetPropertyDouble(const std::string16& name, double value);
virtual bool SetPropertyString(const std::string16 &name,
const std::string16 &value);
@@ -573,6 +593,14 @@
DISALLOW_EVIL_CONSTRUCTORS(JsObjectImpl);
};
+bool JsObjectImpl::SetPropertyInt64(const std::string16 &name, int64 value) {
+ JsScopedToken jsval;
+ if (!Int64ToJsToken(js_context_, value, &jsval)) {
+ return false;
+ }
+ return SetProperty(name, jsval);
+}
+
#if BROWSER_FF
JsObjectImpl::JsObjectImpl() : js_context_(NULL), js_object_(0) {
@@ -863,6 +891,14 @@
if (!GetProperty(name, &token)) return false;
return JsTokenToInt_NoCoerce(token, js_context_, out);
+}
+
+bool JsObjectImpl::GetPropertyAsInt64(const std::string16 &name,
+ int64 *out) const {
+ JsScopedToken token;
+ if (!GetProperty(name, &token)) return false;
+
+ return JsTokenToInt64_NoCoerce(token, js_context_, out);
}
bool JsObjectImpl::GetPropertyAsDouble(const std::string16 &name,
@@ -1088,6 +1124,20 @@
if (!DoubleIsInt64(dbl)) { return false; }
*out = static_cast<int64>(dbl);
return true;
+}
+
+bool Int64ToJsToken(JsContextPtr context, int64 value, JsScopedToken *out) {
+ // If we can represent the value as an integer, do so.
+ if (value >= kint32min && value <= kint32max) {
+ return IntToJsToken(context, static_cast<int>(value), out);
+ }
+ // If we can represent the value as a double without loss of precision, do
so.
+ // fail.
+ if (value >= JS_INT_MIN && value <= JS_INT_MAX) {
+ return DoubleToJsToken(context, static_cast<double>(value), out);
+ }
+ // Otherwise, fail.
+ return false;
}
==== //depot/googleclient/gears/opensource/gears/base/common/js_types.h#39 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/base/common/js_types.h ====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_types.h 2008-11-10
14:29:57.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/js_types.h 2008-11-20
16:11:55.000000000 +0000
@@ -301,6 +301,7 @@
bool BoolToJsToken(JsContextPtr context, bool value, JsScopedToken *out);
bool IntToJsToken(JsContextPtr context, int value, JsScopedToken *out);
+bool Int64ToJsToken(JsContextPtr context, int64 value, JsScopedToken *out);
bool StringToJsToken(JsContextPtr context, const char16 *value,
JsScopedToken *out);
bool DoubleToJsToken(JsContextPtr context, double value, JsScopedToken *out);
@@ -372,6 +373,7 @@
// does not exist.
virtual bool GetElementAsBool(int index, bool *out) const = 0;
virtual bool GetElementAsInt(int index, int *out) const = 0;
+ virtual bool GetElementAsInt64(int index, int64 *out) const = 0;
virtual bool GetElementAsDouble(int index, double *out) const = 0;
virtual bool GetElementAsString(int index, std::string16 *out) const = 0;
virtual bool GetElementAsArray(int index, JsArray **out) const = 0;
@@ -389,6 +391,7 @@
virtual bool SetElementBool(int index, bool value) = 0;
virtual bool SetElementInt(int index, int value) = 0;
+ virtual bool SetElementInt64(int index, int64 value) = 0;
virtual bool SetElementDouble(int index, double value) = 0;
virtual bool SetElementString(int index, const std::string16 &value) = 0;
virtual bool SetElementArray(int index, JsArray *value) = 0;
@@ -417,6 +420,8 @@
bool *out) const = 0;
virtual bool GetPropertyAsInt(const std::string16 &name,
int *out) const = 0;
+ virtual bool GetPropertyAsInt64(const std::string16 &name,
+ int64 *out) const = 0;
virtual bool GetPropertyAsDouble(const std::string16 &name,
double *out) const = 0;
virtual bool GetPropertyAsString(const std::string16 &name,
@@ -445,6 +450,8 @@
bool value) = 0;
virtual bool SetPropertyInt(const std::string16 &name,
int value) = 0;
+ virtual bool SetPropertyInt64(const std::string16 &name,
+ int64 value) = 0;
virtual bool SetPropertyDouble(const std::string16& name,
double value) = 0;
virtual bool SetPropertyString(const std::string16 &name,
==== //depot/googleclient/gears/opensource/gears/cctests/test.cc#88 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/cctests/test.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/cctests/test.cc 2008-11-20
17:26:07.000000000 +0000
+++ googleclient/gears/opensource/gears/cctests/test.cc 2008-11-20
16:57:41.000000000 +0000
@@ -212,8 +212,11 @@
JsRunnerInterface* js_runner,
JsObject* out);
void CreateObjectInt(JsCallContext* context,
- JsRunnerInterface* js_runner,
- JsObject* out);
+ JsRunnerInterface* js_runner,
+ JsObject* out);
+void CreateObjectInt64(JsCallContext* context,
+ JsRunnerInterface* js_runner,
+ JsObject* out);
void CreateObjectDouble(JsCallContext* context,
JsRunnerInterface* js_runner,
JsObject* out);
@@ -236,6 +239,7 @@
void TestObjectBool(JsCallContext* context, const JsObject* obj);
void TestObjectInt(JsCallContext* context, const JsObject* obj);
+void TestObjectInt64(JsCallContext* context, const JsObject* obj);
void TestObjectDouble(JsCallContext* context, const JsObject* obj);
void TestObjectString(JsCallContext* context, const JsObject* obj);
void TestObjectArray(JsCallContext* context,
@@ -542,6 +546,9 @@
TestObjectInt(context, obj.get());
if (context->is_exception_set()) return;
+ TestObjectInt64(context, obj.get());
+ if (context->is_exception_set()) return;
+
TestObjectDouble(context, obj.get());
if (context->is_exception_set()) return;
@@ -577,6 +584,9 @@
if (context->is_exception_set()) return;
CreateObjectInt(context, js_runner, js_object.get());
+ if (context->is_exception_set()) return;
+
+ CreateObjectInt64(context, js_runner, js_object.get());
if (context->is_exception_set()) return;
CreateObjectDouble(context, js_runner, js_object.get());
@@ -1510,7 +1520,7 @@
{ \
if (!(b)) { \
char tmp[256]; \
- snprintf(tmp, sizeof(tmp), "TestObject - failed ( %u: %s\n", \
+ snprintf(tmp, sizeof(tmp), "TestObject - failed (%u: %s)\n", \
__LINE__, __FILE__); \
LOG(("%s", tmp)); \
std::string16 message; \
@@ -1602,6 +1612,75 @@
property_value = -1;
TEST_ASSERT(arr->GetElementAsInt(4, &property_value));
TEST_ASSERT(property_value == -int_large);
+}
+
+void TestObjectInt64(JsCallContext* context, const JsObject* obj) {
+ // When converting a C++ int64 value to a JavaScript value, we can only
+ // convert values in the range [JS_INT_MIN, JS_INT_MAX] = [-2^53 2^53], as
+ // this is the range of values that can be stored in a JavaScript number
+ // without loss of precision.
+ int64 property_value = -1;
+ TEST_ASSERT(obj->GetPropertyAsInt64(STRING16(L"int64_0"), &property_value));
+ TEST_ASSERT(property_value == 0);
+
+ property_value = -1;
+ TEST_ASSERT(obj->GetPropertyAsInt64(STRING16(L"int64_1"), &property_value));
+ TEST_ASSERT(property_value == 1);
+
+ property_value = -1;
+ TEST_ASSERT(obj->GetPropertyAsInt64(STRING16(L"int64_js_int_max"),
+ &property_value));
+ TEST_ASSERT(property_value == JS_INT_MAX);
+
+ TEST_ASSERT(!obj->GetPropertyAsInt64(STRING16(L"int64_too_large"),
+ &property_value));
+
+ property_value = 1;
+ TEST_ASSERT(obj->GetPropertyAsInt64(STRING16(L"int64_negative_1"),
+ &property_value));
+ TEST_ASSERT(property_value == -1);
+
+ property_value = 1;
+ TEST_ASSERT(obj->GetPropertyAsInt64(STRING16(L"int64_js_int_min"),
+ &property_value));
+ TEST_ASSERT(property_value == JS_INT_MIN);
+
+ TEST_ASSERT(!obj->GetPropertyAsInt64(STRING16(L"int64_too_small"),
+ &property_value));
+
+ scoped_ptr<JsArray> arr;
+ TEST_ASSERT(obj->GetPropertyAsArray(STRING16(L"int64_array"),
+ as_out_parameter(arr)));
+
+ int length = -1;
+ TEST_ASSERT(arr->GetLength(&length));
+ TEST_ASSERT(length == 7);
+
+ property_value = -1;
+ TEST_ASSERT(arr->GetElementAsInt64(0, &property_value));
+ TEST_ASSERT(property_value == 0);
+
+ property_value = -1;
+ TEST_ASSERT(arr->GetElementAsInt64(1, &property_value));
+ TEST_ASSERT(property_value == 1);
+
+ property_value = -1;
+ TEST_ASSERT(arr->GetElementAsInt64(2, &property_value));
+ TEST_ASSERT(property_value == JS_INT_MAX);
+
+ // int64_too_large
+ TEST_ASSERT(!arr->GetElementAsInt64(3, &property_value));
+
+ property_value = 1;
+ TEST_ASSERT(arr->GetElementAsInt64(4, &property_value));
+ TEST_ASSERT(property_value == -1);
+
+ property_value = -1;
+ TEST_ASSERT(arr->GetElementAsInt64(5, &property_value));
+ TEST_ASSERT(property_value == JS_INT_MIN);
+
+ // int64_too_small
+ TEST_ASSERT(!arr->GetElementAsInt64(6, &property_value));
}
// Magic number is from:
@@ -1886,6 +1965,30 @@
TEST_ASSERT(int_array->SetElementInt(3, -1));
TEST_ASSERT(int_array->SetElementInt(4, -int_large));
TEST_ASSERT(out->SetPropertyArray(STRING16(L"int_array"), int_array.get()));
+}
+
+void CreateObjectInt64(JsCallContext* context,
+ JsRunnerInterface* js_runner,
+ JsObject* out) {
+ TEST_ASSERT(out->SetPropertyInt64(STRING16(L"int64_0"), 0));
+ TEST_ASSERT(out->SetPropertyInt64(STRING16(L"int64_1"), 1));
+ TEST_ASSERT(out->SetPropertyInt64(STRING16(L"int64_js_int_max"),
JS_INT_MAX));
+ TEST_ASSERT(!out->SetPropertyInt64(STRING16(L"int64_too_large"),
+ JS_INT_MAX + 2));
+ TEST_ASSERT(out->SetPropertyInt64(STRING16(L"int64_negative_1"), -1));
+ TEST_ASSERT(out->SetPropertyInt64(STRING16(L"int64_js_int_min"),
JS_INT_MIN));
+ TEST_ASSERT(!out->SetPropertyInt64(STRING16(L"int64_too_small"),
+ JS_INT_MIN - 2));
+ scoped_ptr<JsArray> int_array(js_runner->NewArray());
+ TEST_ASSERT(int_array.get());
+ TEST_ASSERT(int_array->SetElementInt64(0, 0));
+ TEST_ASSERT(int_array->SetElementInt64(1, 1));
+ TEST_ASSERT(int_array->SetElementInt64(2, JS_INT_MAX));
+ TEST_ASSERT(!int_array->SetElementInt64(3, JS_INT_MAX + 2));
+ TEST_ASSERT(int_array->SetElementInt64(4, -1));
+ TEST_ASSERT(int_array->SetElementInt64(5, JS_INT_MIN));
+ TEST_ASSERT(!int_array->SetElementInt64(6, JS_INT_MIN - 2));
+ TEST_ASSERT(out->SetPropertyArray(STRING16(L"int64_array"),
int_array.get()));
}
void CreateObjectDouble(JsCallContext* context,
====
//depot/googleclient/gears/opensource/gears/test/testcases/internal_tests.js#42
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/test/testcases/internal_tests.js
====
# action=edit type=text
--- googleclient/gears/opensource/gears/test/testcases/internal_tests.js
2008-11-20 17:26:07.000000000 +0000
+++ googleclient/gears/opensource/gears/test/testcases/internal_tests.js
2008-11-20 17:15:03.000000000 +0000
@@ -203,6 +203,11 @@
if (!isFirefoxWorker) {
var testObject = new TestObject();
var createdObject = internalTests.testCreateObject(createTestFunction());
+ // int64_too_large and int64_too_small can not be set from C++,
obviously.
+ createdObject.int64_too_large = testObject.int64_too_large;
+ createdObject.int64_too_small = testObject.int64_too_small;
+ createdObject.int64_array[3] = testObject.int64_array[3];
+ createdObject.int64_array[6] = testObject.int64_array[6];
// assertObjectEqual does not compare function results
assertObjectEqual(testObject, createdObject);
assertEqual(testObject.array_many_types[6](),
@@ -376,6 +381,22 @@
this.int_array = [this.int_0, this.int_1, this.int_large,
this.int_negative_1, this.int_negative_large];
+ // int64
+ var JS_INT_MAX = 9007199254740992; // 2**53
+ var JS_INT_MIN = -9007199254740992; // -2**53
+ this.int64_0 = 0;
+ this.int64_1 = 1;
+ this.int64_js_int_max = JS_INT_MAX
+ // The next largest number that can be distinguished from JS_INT_MAX
+ this.int64_too_large = JS_INT_MAX + 2;
+ this.int64_negative_1 = -1;
+ this.int64_js_int_min = JS_INT_MIN;
+ // The next smallest number that can be distinguished from JS_INT_MIN
+ this.int64_too_small = JS_INT_MIN - 2;
+ this.int64_array = [this.int64_0, this.int64_1, this.int64_js_int_max,
+ this.int64_too_large, this.int64_negative_1, this.int64_js_int_min,
+ this.int64_too_small];
+
// double (assumed to be 64-bit IEEE double precision float point)
this.double_0 = 0.01; // 0.0 becomes an int
this.double_1 = 1.01;
