net: Make UploadData::GetContentLength() asynchronous.

net/base/upload_data.h

Index: net/base/upload_data.h
diff --git a/net/base/upload_data.h b/net/base/upload_data.h
index 31c1958a6966c9e321f554754819b54f0c98726e..134081c0dd24ad044df4615a52c9bb70657f9c8f 100644
--- a/net/base/upload_data.h
+++ b/net/base/upload_data.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
@@ -163,8 +164,22 @@ class NET_EXPORT UploadData : public base::RefCounted<UploadData> {
   void set_is_chunked(bool set) { is_chunked_ = set; }
   bool is_chunked() const { return is_chunked_; }
 
+  // Gets the total size in bytes of the data to upload. Computing the
+  // content length can result in performing file IO hence the operation is
+  // done asynchronously. Runs the callback with the content length once the
+  // computation is done.
+  typedef base::Callback<void(uint64 content_length)> ContentLengthCallback;
+  void GetContentLength(const ContentLengthCallback& callback);

[robertshield, 2012/02/04 05:39:04]

This only hits the IO thread when there are elements of TYPE_FILE afaict. It
seems odd to force all clients to use a (slightly more complicated) async
interface to GetContentLength when only some types of UploadData really require
it. 

Would you consider keeping a GetContentLengthSync() method that works on
UploadData instances that don't refer to file data?

[satorux1, 2012/02/04 07:18:32]

Sounds reasonable. I originally thought it'd be simpler to provide only the
async version but since Chrome Frame will need the sync version, and the sync
version is useful in tests (FWIW, ResourceDispatcherHost and UploadDataStream
are the only non-test clients in Chrome), I'd keep GetContentLengthSync() 
without the thread IO restrictions allowed.

+
   // Returns the total size in bytes of the data to upload.
-  uint64 GetContentLength();
+  // This version may perform file IO on the current thread.
+  // TODO(satorux): Remove this once clients are gone.

[robertshield, 2012/02/04 05:39:04]

Are you planning on attempting to convert the Urlmon client of this in Chrome
Frame to using the asynchronous interface? That seems like it would be hard to
get right. My understanding was that the synchronous interface would remain.

[satorux1, 2012/02/04 07:18:32]

No. I don't plan to make a major change to chrome_frame code. Besides, since
Chrome Frame clients are not affected by the thread io restrictions, there is no
need to make them async.

So I changed the code to have two sync versions:


GetContentLengthSync() - without AllowIO. used by chrome frame and tests
GetContentLengthSyncHack() - with AllowIO. will be removed.

[robertshield, 2012/02/04 21:10:56]

Thanks! That looks good to me. 

+  uint64 GetContentLengthSyncHack();
+
+  // Returns the total size in bytes of the data to upload, for testing.
+  // This version may perform file IO on the current thread.
+  // Should only be used in tests where file IO is allowed.
+  uint64 GetContentLengthSyncForTesting();
 
   // Returns true if the upload data is entirely in memory (i.e. the
   // upload data is not chunked, and all elemnts are of TYPE_BYTES).
@@ -187,6 +202,9 @@ class NET_EXPORT UploadData : public base::RefCounted<UploadData> {
   int64 identifier() const { return identifier_; }
 
  private:
+  // Helper function for GetContentLength().
+  void DoGetContentLength(uint64* content_length);
+
   friend class base::RefCounted<UploadData>;
 
   ~UploadData();