Bug 26521 – Expose file size to chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Victor Wang]

Need to implement getFileSize in WebCore/platform/chromium/FileSystemChromium.cpp.

[Victor Wang]

Created attachment 31515 [details]
Expose file size to chromium

[Tony Chang]

What is this method used for?  I'm worried that an 0wned renderer can start reading to see if certain files exist or not.

[Victor Wang]

(In reply to comment #2)
> What is this method used for?  I'm worried that an 0wned renderer can start
> reading to see if certain files exist or not.
> 

This is only allowed in uploading file. see bug: http://code.google.com/p/chromium/issues/detail?id=9102

Here is the Chromium patch that supports this:
http://codereview.chromium.org/131082

[Tony Chang]

Comment on attachment 31515 [details]
Expose file size to chromium

I see, we validate the filename on the browser side.

Seems like it would be nice if we could get the filesize information when the user picks a file and pass it through the IPC then (the FileChooser in the RenderFileUploadControl would get filename, size pairs).  It would avoid the extra calls IPC calls back to the browser, but this seems like the easier change to make.

[Victor Wang]

I was thinking about getting the filesize info and passing it through IPC with file name user selected, but the FileChooser allows selecting multiple files and more changes on the existing IPC. Discussed with Darin and think having a separate IPC to browser seems easier.

(In reply to comment #4)
> (From update of attachment 31515 [details] [review])
> I see, we validate the filename on the browser side.
> 
> Seems like it would be nice if we could get the filesize information when the
> user picks a file and pass it through the IPC then (the FileChooser in the
> RenderFileUploadControl would get filename, size pairs).  It would avoid the
> extra calls IPC calls back to the browser, but this seems like the easier
> change to make.
> 

[Eric Seidel (no email)]

Comment on attachment 31515 [details]
Expose file size to chromium

Looks fine to me.

[Victor Wang]

(In reply to comment #6)
> (From update of attachment 31515 [details] [review])
> Looks fine to me.
> 

could you hold landing this patch? would like to wait for the change in Chromium codebase to be reviewed. Will update this bug once the Chromium one is ready.

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
...
> Seems like it would be nice if we could get the filesize information when the
> user picks a file and pass it through the IPC then (the FileChooser in the
> RenderFileUploadControl would get filename, size pairs).  It would avoid the
> extra calls IPC calls back to the browser, but this seems like the easier
> change to make.

This seems like an interesting idea to me.  It would mean changing FileChooser
to hand back not just a list of file names but also the sizes of those files.

One downside is that the file size returned to script would not be dynamic.  That
may actually be a deal breaker since I think it is normally valid for users to
change the file to be uploaded before actually submitting the form.

[Darin Adler]

(In reply to comment #8)
> I think it is normally valid for users to
> change the file to be uploaded before actually submitting the form.

Yes, it normally is.

[Eric Seidel (no email)]

Can this be landed now?

[Victor Wang]

Not yet. Still waiting for the patch in chromium.

[Eric Seidel (no email)]

Looks like http://codereview.chromium.org/131082 was just approved by fishd.  Let me know when this should go in.

Any commiter can easily land this with:

bugzilla-tool land-patches 26521 --no-build

(--no-build because there is no way to build these chromium-only changes in WebKit).

[Victor Wang]

Thanks Eric!

Both patches are ready, but we need to sync the webkit patch with the Chromium change. Dimitri is the sheriff for Chromium webkit integration and he is going to land this one while he is doing the merging.

(In reply to comment #12)
> Looks like http://codereview.chromium.org/131082 was just approved by fishd. 
> Let me know when this should go in.
> 
> Any commiter can easily land this with:
> 
> bugzilla-tool land-patches 26521 --no-build
> 
> (--no-build because there is no way to build these chromium-only changes in
> WebKit).
> 

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/45494.