Hello aa,
I'd like you to do a code review. Please execute
g4 diff -c 8280959
or point your web browser to
http://mondrian/8280959
to review the following code:
Change 8280959 by [EMAIL PROTECTED] on 2008/09/16 18:36:41 *pending*
Remove the JSPARAM_COM_MODULE / ModuleImplBaseClassVirtual special
case for JsCallContext::SetReturnValue.
PRESUBMIT=passed
R=aa
[EMAIL PROTECTED]
DELTA=32 (0 added, 17 deleted, 15 changed)
OCL=8280959
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_types.h#35 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#18 edit
... //depot/googleclient/gears/opensource/gears/console/console.cc#6 edit
... //depot/googleclient/gears/opensource/gears/database/result_set.cc#4 edit
... //depot/googleclient/gears/opensource/gears/httprequest/httprequest.cc#22
edit
...
//depot/googleclient/gears/opensource/gears/httprequest/httprequest_upload.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/localserver/localserver_module.cc#4
edit
... //depot/googleclient/gears/opensource/gears/media/audio.cc#8 edit
... //depot/googleclient/gears/opensource/gears/media/audio_recorder.cc#8 edit
... //depot/googleclient/gears/opensource/gears/notifier/notification.cc#16 edit
32 delta lines: 0 added, 17 deleted, 15 changed
Also consider running:
g4 lint -c 8280959
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 8280959 by [EMAIL PROTECTED] on 2008/09/16 18:36:41 *pending*
Remove the JSPARAM_COM_MODULE / ModuleImplBaseClassVirtual special
case for JsCallContext::SetReturnValue.
OCL=8280959
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_types.h#35 edit
... //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#18 edit
... //depot/googleclient/gears/opensource/gears/console/console.cc#6 edit
... //depot/googleclient/gears/opensource/gears/database/result_set.cc#4 edit
... //depot/googleclient/gears/opensource/gears/httprequest/httprequest.cc#22
edit
...
//depot/googleclient/gears/opensource/gears/httprequest/httprequest_upload.cc#3
edit
...
//depot/googleclient/gears/opensource/gears/localserver/localserver_module.cc#4
edit
... //depot/googleclient/gears/opensource/gears/media/audio.cc#8 edit
... //depot/googleclient/gears/opensource/gears/media/audio_recorder.cc#8 edit
... //depot/googleclient/gears/opensource/gears/notifier/notification.cc#16 edit
==== //depot/googleclient/gears/opensource/gears/base/common/js_types.h#35 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/base/common/js_types.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_types.h 2008-09-12
18:31:09.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/js_types.h 2008-09-16
18:30:14.000000000 +1000
@@ -552,23 +552,7 @@
bool GetArgumentAsString(int i, std::string16 *out, bool coerce=false);
// Sets the value to be returned to the calling JavaScript.
- //
- // The ModuleImplBaseClass* version should only be used when returning a
- // JSPARAM_COM_MODULE. It exists because passing a derived class through a
- // void* and then casting to the base class is not safe - the compiler won't
- // be able to correctly adjust the pointer offset.
- //
- // The int version is for use with JSPARAM_NULL, to avoid conflicting with
the
- // ModuleImplBaseClass version (works because NULL is defined as 0).
void SetReturnValue(JsParamType type, const void *value_ptr);
- void SetReturnValue(JsParamType type, const ModuleImplBaseClass *value_ptr) {
- assert(type == JSPARAM_MODULE);
- SetReturnValue(type, reinterpret_cast<const void*>(value_ptr));
- }
- void SetReturnValue(JsParamType type, int) {
- assert(type == JSPARAM_NULL);
- SetReturnValue(type, reinterpret_cast<const void*>(NULL));
- }
// Sets an exception to be thrown to the calling JavaScript. Setting an
// exception overrides any previous exception and any return values.
==== //depot/googleclient/gears/opensource/gears/canvas/canvas.cc#18 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/canvas/canvas.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/canvas/canvas.cc 2008-09-16
18:39:44.000000000 +1000
+++ googleclient/gears/opensource/gears/canvas/canvas.cc 2008-09-16
18:33:35.000000000 +1000
@@ -324,7 +324,7 @@
if (context->is_exception_set())
return;
if (context_id != STRING16(L"gears-2d")) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
// As per the HTML5 canvas spec.
return;
}
==== //depot/googleclient/gears/opensource/gears/console/console.cc#6 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/console/console.cc
====
# action=edit type=xtext
--- googleclient/gears/opensource/gears/console/console.cc 2008-09-16
18:39:44.000000000 +1000
+++ googleclient/gears/opensource/gears/console/console.cc 2008-09-16
18:33:40.000000000 +1000
@@ -78,7 +78,7 @@
void GearsConsole::GetOnLog(JsCallContext *context) {
JsRootedCallback *callback = callback_backend_->GetCallback();
if (callback == NULL) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
} else {
context->SetReturnValue(JSPARAM_FUNCTION, callback);
}
==== //depot/googleclient/gears/opensource/gears/database/result_set.cc#4 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/database/result_set.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/database/result_set.cc 2008-09-16
18:39:44.000000000 +1000
+++ googleclient/gears/opensource/gears/database/result_set.cc 2008-09-16
18:33:25.000000000 +1000
@@ -150,7 +150,7 @@
break;
}
case SQLITE_NULL:
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
break;
case SQLITE_BLOB:
// TODO(miket): figure out right way to pass around blobs in variants.
==== //depot/googleclient/gears/opensource/gears/httprequest/httprequest.cc#22
-
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/httprequest/httprequest.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/httprequest/httprequest.cc
2008-09-15 12:24:10.000000000 +1000
+++ googleclient/gears/opensource/gears/httprequest/httprequest.cc
2008-09-16 18:33:15.000000000 +1000
@@ -359,7 +359,7 @@
void GearsHttpRequest::GetOnProgress(JsCallContext *context) {
JsRootedCallback *callback = onprogresshandler_.get();
if (callback == NULL) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
} else {
context->SetReturnValue(JSPARAM_FUNCTION, callback);
}
@@ -385,7 +385,7 @@
// SetReturnValue(JSPARAM_FUNCTION, NULL) to get a (JavaScript) null, rather
// than having to explicitly check for a NULL JsRootedCallback*.
if (callback == NULL) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
} else {
context->SetReturnValue(JSPARAM_FUNCTION, callback);
}
@@ -415,7 +415,7 @@
return;
}
if (!IsValidResponse()) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
scoped_refptr<BlobInterface> unused_blob;
====
//depot/googleclient/gears/opensource/gears/httprequest/httprequest_upload.cc#3
-
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/httprequest/httprequest_upload.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/httprequest/httprequest_upload.cc
2008-09-16 18:39:44.000000000 +1000
+++ googleclient/gears/opensource/gears/httprequest/httprequest_upload.cc
2008-09-16 18:32:57.000000000 +1000
@@ -43,7 +43,7 @@
void GearsHttpRequestUpload::GetOnProgress(JsCallContext *context) {
JsRootedCallback *callback = onprogress_handler_.get();
if (callback == NULL) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
} else {
context->SetReturnValue(JSPARAM_FUNCTION, callback);
}
====
//depot/googleclient/gears/opensource/gears/localserver/localserver_module.cc#4
-
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/localserver/localserver_module.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/localserver/localserver_module.cc
2008-09-16 18:39:45.000000000 +1000
+++ googleclient/gears/opensource/gears/localserver/localserver_module.cc
2008-09-16 18:32:51.000000000 +1000
@@ -151,7 +151,7 @@
name.c_str(),
required_cookie.c_str(),
&existing_store_id)) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
@@ -195,7 +195,7 @@
name.c_str(),
required_cookie.c_str(),
&existing_store_id)) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
@@ -279,7 +279,7 @@
name.c_str(),
required_cookie.c_str(),
&existing_store_id)) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
==== //depot/googleclient/gears/opensource/gears/media/audio.cc#8 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/media/audio.cc ====
# action=edit type=text
--- googleclient/gears/opensource/gears/media/audio.cc 2008-09-16
18:39:45.000000000 +1000
+++ googleclient/gears/opensource/gears/media/audio.cc 2008-09-16
18:32:33.000000000 +1000
@@ -40,7 +40,7 @@
void GearsAudio::GetError(JsCallContext *context) {
int last_error = media_data_->GetLastError();
if (last_error == MediaConstants::MEDIA_NO_ERROR) {
- context->SetReturnValue(JSPARAM_NULL, MediaConstants::MEDIA_NO_ERROR);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
==== //depot/googleclient/gears/opensource/gears/media/audio_recorder.cc#8 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/media/audio_recorder.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/media/audio_recorder.cc 2008-09-16
18:39:45.000000000 +1000
+++ googleclient/gears/opensource/gears/media/audio_recorder.cc 2008-09-16
18:32:25.000000000 +1000
@@ -121,8 +121,7 @@
void GearsAudioRecorder::GetError(JsCallContext *context) {
if (last_error_ == AudioRecorderConstants::AUDIO_RECORDER_NO_ERROR) {
- context->SetReturnValue(JSPARAM_NULL,
- AudioRecorderConstants::AUDIO_RECORDER_NO_ERROR);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
JsRunnerInterface *js_runner = this->GetJsRunner();
@@ -401,7 +400,7 @@
void GearsAudioRecorder::GetBlob(JsCallContext *context) {
// TODO(vamsikrishna): Return the blob based on the 'type' attribute.
if (recording_) {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
return;
}
==== //depot/googleclient/gears/opensource/gears/notifier/notification.cc#16 -
/home/nigeltao/srcgears1/googleclient/gears/opensource/gears/notifier/notification.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/notifier/notification.cc
2008-09-16 18:39:45.000000000 +1000
+++ googleclient/gears/opensource/gears/notifier/notification.cc
2008-09-16 18:31:59.000000000 +1000
@@ -291,7 +291,7 @@
scoped_ptr<JsObject> obj(js_runner->NewDate(display_at_time_ms_));
context->SetReturnValue(JSPARAM_OBJECT, obj.get());
} else {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
}
}
@@ -317,7 +317,7 @@
scoped_ptr<JsObject> obj(js_runner->NewDate(display_until_time_ms_));
context->SetReturnValue(JSPARAM_OBJECT, obj.get());
} else {
- context->SetReturnValue(JSPARAM_NULL, 0);
+ context->SetReturnValue(JSPARAM_NULL, NULL);
}
}
