Add getFileSize support to chromium...

Add getFileSize support to chromium

The current implementation only allows getting file size
if the child process has been granted permission to upload file.
May need to update the policy checking code if getFileSize
is needed in other cases.

Here is the webkit implementation to support this:
https://bugs.webkit.org/show_bug.cgi?id=26521

TEST=none
BUG=9102
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19866

[jam, 2009-06-19 00:20:09]

http://codereview.chromium.org/131082/diff/1017/1023
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/131082/diff/1017/1023#newcode910
Line 910: if (file_util::GetFileSize(path, result))
we avoid doing file access on the IO thread.  You can use
IPC_MESSAGE_HANDLE_DELAY_REPLY in order to pass off this request to the file
thread, then pass the result back to the IO thread.

[Victor Wang, 2009-06-19 23:11:30]

On 2009/06/19 00:20:09, John Abd-El-Malek wrote:
> http://codereview.chromium.org/131082/diff/1017/1023
> File chrome/browser/renderer_host/resource_message_filter.cc (right):
> 
> http://codereview.chromium.org/131082/diff/1017/1023#newcode910
> Line 910: if (file_util::GetFileSize(path, result))
> we avoid doing file access on the IO thread.  You can use
> IPC_MESSAGE_HANDLE_DELAY_REPLY in order to pass off this request to the file
> thread, then pass the result back to the IO thread.

Thanks John for the suggestion. Patch updated to get file size from FILE thread.
Please take a look and let me know of your comments.

[jam, 2009-06-19 23:35:57]

since ResourceMessageFilter is refcounted, why add this new class?  I think
it'll be simpler if you do

PostTask(FROM_HERE, NewRunnableMethod(this, &ResourceMessageFilter::OnX))...

[Victor Wang, 2009-06-19 23:46:12]

On 2009/06/19 23:35:57, John Abd-El-Malek wrote:
> since ResourceMessageFilter is refcounted, why add this new class?  I think
> it'll be simpler if you do
> 
> PostTask(FROM_HERE, NewRunnableMethod(this, &ResourceMessageFilter::OnX))...

I prefer to have a separate class to handle file access and PostTask between IO
and FILE threads. Was discussing this with Darin on where to add these code and
we feel a new class sounds better.

[darin (slow to review), 2009-06-22 19:15:08]

Looking at this now, in its entirety, I suspect it would be simpler to just
subclass Task as we already do at the top of resource_message_filter.cc to do a
one-off thread proxy.

Also, instead of using ChromeThread to get the IO thread, we normally just stash
a pointer to MessageLoop::current().  That way the code is less explicitly
dependent on the IO thread.

[abarth-chromium, 2009-06-22 19:23:58]

http://codereview.chromium.org/131082/diff/3021/3026
File chrome/browser/renderer_host/file_system_accessor.cc (right):

http://codereview.chromium.org/131082/diff/3021/3026#newcode25
Line 25: return false;
This security check is correct, but it should be moved to
resource_message_filter.  In general, we like to do all these security checks in
the IPC handler so we can audit them all at once.

[Victor Wang, 2009-06-23 19:12:46]

On 2009/06/22 19:15:08, darin wrote:
> Looking at this now, in its entirety, I suspect it would be simpler to just
> subclass Task as we already do at the top of resource_message_filter.cc to do
a
> one-off thread proxy.
> 
> Also, instead of using ChromeThread to get the IO thread, we normally just
stash
> a pointer to MessageLoop::current().  That way the code is less explicitly
> dependent on the IO thread.

If we subclass Task instead of using NewRunnableMethod, we need to define two
new classes: one for PostTask from IO to FILE and the other one for PostTask
from FILE back to IO. The task from FILE to IO just calls back to the caller
thread to pass the result. If we have to create two new task classes, I prefer
using NewRunnableMethod.

A couple of updates in the new patch:
-. Added a pointer to store the caller loop so the code does not rely on the IO
thread. 
-. Added code to create ref counted FileSystemAccessor instance inside
GetFileSize so no need to have it as a member of esourceMessageFilter.
-. Added code to update the ref count of ResourceMessageFilter to ensure it
won't be destroyed before the callback is done in any case.
-. Added some members to FileSystemAccessor so no need to pass them around in
function call to make the code little simpler.

I thought about creating a more generic class to handle the request like this.
It seems more code are added if we want to make the class generic enough and
GetFileSize is the only one that is using it,  maybe we could consider this
later when there are other similar tasks?

[Victor Wang, 2009-06-23 19:13:48]

On 2009/06/22 19:23:58, abarth wrote:
> http://codereview.chromium.org/131082/diff/3021/3026
> File chrome/browser/renderer_host/file_system_accessor.cc (right):
> 
> http://codereview.chromium.org/131082/diff/3021/3026#newcode25
> Line 25: return false;
> This security check is correct, but it should be moved to
> resource_message_filter.  In general, we like to do all these security checks
in
> the IPC handler so we can audit them all at once.

done. new patch uploaded.

[darin (slow to review), 2009-06-25 23:16:14]

http://codereview.chromium.org/131082/diff/4001/5005
File chrome/browser/renderer_host/file_system_accessor.cc (right):

http://codereview.chromium.org/131082/diff/4001/5005#newcode17
Line 17: };
nit: extraneous semicolon here

http://codereview.chromium.org/131082/diff/4001/5008
File chrome/browser/renderer_host/file_system_accessor.h (right):

http://codereview.chromium.org/131082/diff/4001/5008#newcode22
Line 22: typedef Callback2<int64, IPC::Message*>::Type FileSizeCallback;
It may be slightly cleaner if our public section were like this:

  typedef Callback2<int64, void*>::Type FileSizeCallback;

  static void RequestFileSize(
      const FilePath& path, void* param, FileSizeCallback* callback);

The implementation of RequestFileSize would create the FileSystemAccessor
instance and PostTask to the FILE thread.  Then on completion it would PostTask
back, and run the callback.

Instead of IPC::Message*, I suggest just using a generic void* so that this
class could be used by other consumers that may wish to asynchonously query file
size information.

http://codereview.chromium.org/131082/diff/4001/5008#newcode24
Line 24: FileSystemAccessor(MessageLoop* worker_loop,
I think it is OK to hide the details of the selection of the worker_loop within
the FileSystemAccessor class.  The consumer doesn't need to tell us to use the
FILE thread.  We can just do that implicitly.

[Victor Wang, 2009-06-26 00:17:52]

http://codereview.chromium.org/131082/diff/4001/5005
File chrome/browser/renderer_host/file_system_accessor.cc (right):

http://codereview.chromium.org/131082/diff/4001/5005#newcode17
Line 17: };
On 2009/06/25 23:16:14, darin wrote:
> nit: extraneous semicolon here
removed.

http://codereview.chromium.org/131082/diff/4001/5008
File chrome/browser/renderer_host/file_system_accessor.h (right):

http://codereview.chromium.org/131082/diff/4001/5008#newcode22
Line 22: typedef Callback2<int64, IPC::Message*>::Type FileSizeCallback;
On 2009/06/25 23:16:14, darin wrote:
> It may be slightly cleaner if our public section were like this:
> 
>   typedef Callback2<int64, void*>::Type FileSizeCallback;
> 
>   static void RequestFileSize(
>       const FilePath& path, void* param, FileSizeCallback* callback);
> 
> The implementation of RequestFileSize would create the FileSystemAccessor
> instance and PostTask to the FILE thread.  Then on completion it would
PostTask
> back, and run the callback.
> 
> Instead of IPC::Message*, I suggest just using a generic void* so that this
> class could be used by other consumers that may wish to asynchonously query
file
> size information.

good suggestion. done.

http://codereview.chromium.org/131082/diff/4001/5008#newcode24
Line 24: FileSystemAccessor(MessageLoop* worker_loop,
On 2009/06/25 23:16:14, darin wrote:
> I think it is OK to hide the details of the selection of the worker_loop
within
> the FileSystemAccessor class.  The consumer doesn't need to tell us to use the
> FILE thread.  We can just do that implicitly.

Done.

[darin (slow to review), 2009-06-26 03:33:44]

More comments... sorry for not catching all of these earlier.

http://codereview.chromium.org/131082/diff/6001/5038
File chrome/browser/renderer_host/file_system_accessor.h (right):

http://codereview.chromium.org/131082/diff/6001/5038#newcode4
Line 4: 
nit: please add a comment up here describing what this file contains.  also, i
think it is a good idea to write a unit test (sorry for not mentioning that
earlier).

in the unit test, you'll need to create a temporary ChromeThread::FILE.

http://codereview.chromium.org/131082/diff/6001/5038#newcode20
Line 20: FileSystemAccessor(void* param, FileSizeCallback* callback);
i think the constructor can be made private now.

http://codereview.chromium.org/131082/diff/6001/5038#newcode23
Line 23: // Request to get file size.
might want to explain the "param" parameter.  also, if the file does not exist
or there is some error accessing the file, you should describe what the callback
will see.

http://codereview.chromium.org/131082/diff/6001/5037
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/131082/diff/6001/5037#newcode318
Line 318: 
nit: we don't usually put blank lines in the IPC message map.

http://codereview.chromium.org/131082/diff/6001/5034
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/131082/diff/6001/5034#newcode87
Line 87: virtual bool getFileSize(const WebString& path, long long& result) = 0;
how about just having this function return 'long long' and comment that -1 means
there was an error?

http://codereview.chromium.org/131082/diff/6001/5033
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/131082/diff/6001/5033#newcode148
Line 148: bool ChromiumBridge::getFileSize(const String& path, long long&
result)
I think the ChromiumBridge method should also return 'long long'.  Sorry to ask
you to update the WebKit patch!  I'll happily R+ that once you do.

http://codereview.chromium.org/131082/diff/6001/5032
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/131082/diff/6001/5032#newcode101
Line 101: return false;
I think it would be good to implement this one since that may allow some layout
tests to pass (assuming there are layout tests testing this stuff)!  This one
does not need to proxy to a background thread.  It can just call
file_util::GetFileSize directly.

[Victor Wang, 2009-06-27 00:27:21]

Hi Darin,

Unit test has been added in the new patch. Please take a look and let me know
your comments.

Thanks,
Victor

http://codereview.chromium.org/131082/diff/6001/5038
File chrome/browser/renderer_host/file_system_accessor.h (right):

http://codereview.chromium.org/131082/diff/6001/5038#newcode4
Line 4: 
On 2009/06/26 03:33:44, darin wrote:
> nit: please add a comment up here describing what this file contains.  also, i
> think it is a good idea to write a unit test (sorry for not mentioning that
> earlier).
> 
> in the unit test, you'll need to create a temporary ChromeThread::FILE.

Not a problem. Both comments and unit test are added in new patch. Thanks for
good suggestions!

http://codereview.chromium.org/131082/diff/6001/5038#newcode20
Line 20: FileSystemAccessor(void* param, FileSizeCallback* callback);
On 2009/06/26 03:33:44, darin wrote:
> i think the constructor can be made private now.

Done.

http://codereview.chromium.org/131082/diff/6001/5038#newcode23
Line 23: // Request to get file size.
On 2009/06/26 03:33:44, darin wrote:
> might want to explain the "param" parameter.  also, if the file does not exist
> or there is some error accessing the file, you should describe what the
callback
> will see.

Done.

http://codereview.chromium.org/131082/diff/6001/5037
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/131082/diff/6001/5037#newcode318
Line 318: 
On 2009/06/26 03:33:44, darin wrote:
> nit: we don't usually put blank lines in the IPC message map.
removed.

http://codereview.chromium.org/131082/diff/6001/5034
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/131082/diff/6001/5034#newcode87
Line 87: virtual bool getFileSize(const WebString& path, long long& result) = 0;
On 2009/06/26 03:33:44, darin wrote:
> how about just having this function return 'long long' and comment that -1
means
> there was an error?
see comment in ChromiumBridge.cpp

http://codereview.chromium.org/131082/diff/6001/5033
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/131082/diff/6001/5033#newcode148
Line 148: bool ChromiumBridge::getFileSize(const String& path, long long&
result)
On 2009/06/26 03:33:44, darin wrote:
> I think the ChromiumBridge method should also return 'long long'.  Sorry to
ask
> you to update the WebKit patch!  I'll happily R+ that once you do.

not a problem if we decided to change:), but I think we may want to keep the
current implementation to match the getFileSize signatures in webkit and other
chromium code base. All webkit FileSystem related code defines "bool
getFileSize(...)" and the same in base/file_util.cc in chromium. In this patch,
I am using the same signature if possible. The only exception is message handler
code in resource_message_filter and file_system_accessor that return void. So
think may be better to return bool to be consistent with Webkit code base and
file_util, what do you think?

http://codereview.chromium.org/131082/diff/6001/5032
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/131082/diff/6001/5032#newcode101
Line 101: return false;
On 2009/06/26 03:33:44, darin wrote:
> I think it would be good to implement this one since that may allow some
layout
> tests to pass (assuming there are layout tests testing this stuff)!  This one
> does not need to proxy to a background thread.  It can just call
> file_util::GetFileSize directly.

Done.

[darin (slow to review), 2009-06-27 03:30:34]

http://codereview.chromium.org/131082/diff/6001/5033
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/131082/diff/6001/5033#newcode148
Line 148: bool ChromiumBridge::getFileSize(const String& path, long long&
result)
> So
> think may be better to return bool to be consistent with Webkit code base and
> file_util, what do you think?

OK... that makes sense.

[darin (slow to review), 2009-06-27 03:34:48]

OK, LGTM

http://codereview.chromium.org/131082/diff/6017/5087
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/131082/diff/6017/5087#newcode31
Line 31: virtual bool getFileSize(const WebKit::WebString& path, int64& result);
nit: int64 -> "long long" to match the way it is declared in WebKitClient.h

http://codereview.chromium.org/131082/diff/6017/5077
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/131082/diff/6017/5077#newcode101
Line 101: virtual bool getFileSize(const WebKit::WebString& path, int64& result)
{
nit: int64 -> "long long" to match the way it is declared in WebKitClient.h

[Victor Wang, 2009-06-27 23:35:53]

http://codereview.chromium.org/131082/diff/6017/5087
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/131082/diff/6017/5087#newcode31
Line 31: virtual bool getFileSize(const WebKit::WebString& path, int64& result);
On 2009/06/27 03:34:48, darin wrote:
> nit: int64 -> "long long" to match the way it is declared in WebKitClient.h

Done.

http://codereview.chromium.org/131082/diff/6017/5077
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/131082/diff/6017/5077#newcode101
Line 101: virtual bool getFileSize(const WebKit::WebString& path, int64& result)
{
On 2009/06/27 03:34:48, darin wrote:
> nit: int64 -> "long long" to match the way it is declared in WebKitClient.h

Done.

[Victor Wang, 2009-06-29 18:46:56]

A new WebKit client "BrowserWebKitClientImpl" was checked in recently, add
getFileSize to this class.

[darin (slow to review), 2009-06-29 19:50:17]

OK, LGTM

[Victor Wang, 2009-07-02 20:18:46]

some minor updates so it can work for mac and linux, sorry for not catching this
earlier.
-. added getFileSize to linux SandboxIPCProcess.
-. pass FilePath param in GetFileSize in test_shell_webkit_init.h.
-. remove directory test from FileSystemAccessorTest. Directory size test on
different OS file system could cause the test flaky. Think it is OK to not do it
in this unittest. If we need to add such test, it should go to
file_util:GetFileSize instead of this one.

[darin (slow to review), 2009-07-02 20:27:13]

LGTM