Hello andreip, nicolasroard,
I'd like you to do a code review. Please execute
g4 diff -c 9387431
or point your web browser to
http://mondrian/9387431
(this changelist has been uploaded to Mondrian)
to review the following code:
Change 9387431 by jrip...@jripley-gears-trunk on 2008/12/12 13:54:29 *pending*
Add progress messages to HttpRequestAndroid.
This implements both download and upload progress messages for
HttpRequests on Android.
Due to the transfers actually occurring on a background
thread, this requires that the message is actually performed
by the main thread via an AsyncFunctor.
If the HttpRequest was created by a worker thread, this is
forwarded again to the worker via the wrapper in
SafeHttpRequest.
There is some message rate mitigation done by making sure only
one progress AsyncFunctor is in flight. The reported
sent/received count is atomically updated while in-flight.
PRESUBMIT=passed
BUG=1492015
R=andreip,nicolasroard
[email protected]
DELTA=121 (121 added, 0 deleted, 0 changed)
OCL=9387431
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc#4
edit
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h#2
edit
121 delta lines: 121 added, 0 deleted, 0 changed
The issue description(s) relevant to this code can be found at:
http://b/issue?id=1492015
Also consider running:
g4 lint -c 9387431
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 9387431 by jrip...@jripley-gears-trunk on 2008/12/12 13:54:29 *pending*
Add progress messages to HttpRequestAndroid.
This implements both download and upload progress messages for
HttpRequests on Android.
Due to the transfers actually occurring on a background
thread, this requires that the message is actually performed
by the main thread via an AsyncFunctor.
If the HttpRequest was created by a worker thread, this is
forwarded again to the worker via the wrapper in
SafeHttpRequest.
There is some message rate mitigation done by making sure only
one progress AsyncFunctor is in flight. The reported
sent/received count is atomically updated while in-flight.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc#4
edit
...
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h#2
edit
====
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc#4
-
/usr/local/google/home/jripley/gears-trunk/googleclient/gears/opensource/gears/localserver/android/http_request_android.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/android/http_request_android.cc
2008-12-11 18:48:25.000000000 +0000
+++
googleclient/gears/opensource/gears/localserver/android/http_request_android.cc
2008-12-12 14:49:56.000000000 +0000
@@ -193,6 +193,11 @@
java_object_(),
http_listener_(NULL),
listener_enable_data_available_(false),
+ bytes_received_(0),
+ bytes_sent_(0),
+ total_bytes_to_send_(0),
+ listener_updating_received_(0),
+ listener_updating_sent_(0),
asynchronous_(true),
was_aborted_(false),
was_aborted_mutex_(),
@@ -514,6 +519,8 @@
if (!IsOpen()) return false;
if (IsPostOrPut()) {
post_blob_.reset(blob ? blob : new EmptyBlob());
+ total_bytes_to_send_ = std::max(post_blob_->Length(),
+ static_cast<int64_t>(0));
} else if (blob) {
return false;
}
@@ -976,6 +983,19 @@
byte_array.Get(),
static_cast<int>(to_send))) {
pos += to_send;
+ {
+ MutexLock lock(&mutex_);
+ if (listener_enable_data_available_ && http_listener_ != NULL) {
+ bytes_sent_ = pos;
+ if (!listener_updating_sent_) {
+ // Inform the listener that data was sent. This will be
+ // marshaled to the correct thread by SafeHttpRequest as
+ // necessary.
+ listener_updating_sent_ = true;
+ SendProgressFunctor();
+ }
+ }
+ }
// Loop.
} else {
LOG(("Sending post data failed\n"));
@@ -1166,6 +1186,19 @@
}
env->ReleaseByteArrayElements(byte_array.Get(), pinned_array, 0);
total_bytes += got;
+ {
+ MutexLock lock(&mutex_);
+ if (listener_enable_data_available_ && http_listener_ != NULL) {
+ bytes_received_ = total_bytes;
+ if (!listener_updating_received_) {
+ // Inform the listener that data arrived. This will be
+ // marshaled to the correct thread by SafeHttpRequest as
+ // necessary.
+ listener_updating_received_ = true;
+ SendProgressFunctor();
+ }
+ }
+ }
}
}
MutexLock lock(&mutex_);
@@ -1394,6 +1427,56 @@
#endif
return false;
}
+}
+
+void HttpRequestAndroid::SendProgressFunctor() {
+ assert(IsChildThread());
+ LOG(("Sending progress functor to main thread\n"));
+ // Maintain an extra reference while in-flight, so the HttpRequest
+ // cannot be deleted until these messages have been processed.
+ Ref();
+ AsyncRouter::GetInstance()->CallAsync(
+ main_thread_id_, new ProgressFunctor(this));
+}
+
+void HttpRequestAndroid::OnProgress() {
+ assert(IsMainThread());
+ // This is executed on the main thread, so the listener cannot be
+ // changed asynchronously.
+ if (!listener_enable_data_available_ ||
+ http_listener_ == NULL ||
+ was_aborted_) {
+ // If there's nobody interested in the progress update, drop it.
+ Unref();
+ return;
+ }
+ int64_t received = -1;
+ int64_t sent = -1;
+ {
+ MutexLock lock(&mutex_);
+ if (listener_updating_received_) {
+ listener_updating_received_ = false;
+ received = bytes_received_;
+ }
+ if (listener_updating_sent_) {
+ listener_updating_sent_ = false;
+ sent = bytes_sent_;
+ }
+ }
+ LOG(("Progress: %d %d\n",
+ static_cast<int>(received),
+ static_cast<int>(sent)));
+ // Send the progress update event on to the listener. If this is
+ // wrapped by SafeHttpRequest, it will be forwarded to the worker
+ // thread.
+ if (received >= 0) {
+ http_listener_->DataAvailable(this, received);
+ }
+ if (sent >= 0) {
+ http_listener_->UploadProgress(this, sent, total_bytes_to_send_);
+ }
+ // Release the reference.
+ Unref();
}
#ifdef DEBUG
====
//depot/googleclient/gears/opensource/gears/localserver/android/http_request_android.h#2
-
/usr/local/google/home/jripley/gears-trunk/googleclient/gears/opensource/gears/localserver/android/http_request_android.h
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/android/http_request_android.h
2008-12-11 18:48:25.000000000 +0000
+++
googleclient/gears/opensource/gears/localserver/android/http_request_android.h
2008-12-11 18:49:43.000000000 +0000
@@ -233,6 +233,20 @@
State new_state_;
};
+ // Functor which forwards progress updates to the main thread, on
+ // behalf of the child thread.
+ class ProgressFunctor : public AsyncFunctor {
+ public:
+ ProgressFunctor(HttpRequestAndroid *instance) : instance_(instance) { }
+
+ virtual void Run() {
+ instance_->OnProgress();
+ }
+
+ private:
+ HttpRequestAndroid *instance_;
+ };
+
// No public constructors. These Create() functions are used
// instead.
friend bool HttpRequest::Create(scoped_refptr<HttpRequest> *);
@@ -332,6 +346,15 @@
assert(IsMainThread());
state_ = state;
}
+ // Called on the child thread. This sends a message to the main
+ // thread to update received/sent progress. This may then be
+ // forwarded to another child thread if this is being wrapped by
+ // SafeHttpRequest.
+ void SendProgressFunctor();
+ // Called on the main thread after being sent by an AsyncFunctor
+ // from the child thread. Sends an updated sent/received byte count
+ // as necessary.
+ void OnProgress();
// Method IDs. Must match the order in java_methods_.
enum JavaMethod {
@@ -399,6 +422,21 @@
// If true, the listener will be notified as soon as data is first
// available.
bool listener_enable_data_available_;
+ // The total number of bytes received by the child thread so far,
+ // protected by mutex_.
+ int64_t bytes_received_;
+ // The total number of bytes sent by the child thread so far,
+ // protected by mutex_.
+ int64_t bytes_sent_;
+ // The total number of bytes that will be by the child thread, or 0
+ // if this cannot be determined.
+ int64_t total_bytes_to_send_;
+ // True if there is an AsyncFunctor in-flight to the main thread to
+ // update the received byte count.
+ bool listener_updating_received_;
+ // True if there is an AsyncFunctor in-flight to the main thread to
+ // update the sent byte count.
+ bool listener_updating_sent_;
// True if this is an asynchronous (non-blocking) request.
bool asynchronous_;
// True if Abort() was used on this instance at any point, false
