More filesystem cleanup: convert URL-encoded-as-FilePath to actual URL, where

More filesystem cleanup: convert URL-encoded-as-FilePath to actual URL, where
possible without WebKit API changes.  The WebKit changes will happen in another
CL.

BUG=none
TEST=none

[ericu, 2011-04-02 01:36:48]

This passes on Windows, but I need to fix posix; it's undoubtedly a one-liner
having to do with path separators, so I'm going to go ahead and ask for a review
now while I chase it down.  I'm sorry this is so big--I just started pulling on
a thread, and the whole thing unraveled.  It's really a much smaller change than
it looks, though, as the vast majority of the changes are just swapping the data
type on a parameter or two from FilePath to GURL.  There's a bit more to do
later in the pepper section, as I've discussed with Trung offline.

Could you each please review the following sections:

Trung: Pepper/flash stuff
Zelidrag: extension stuff
Kinuko: filesystem stuff [pretty much everything else]
Jam: whatever it takes to get owner approval on the content directories

?

Thanks!

	Eric

[kinuko, 2011-04-04 13:38:11]

lgtm

On 2011/04/02 01:36:48, ericu wrote:
> This passes on Windows, but I need to fix posix; it's undoubtedly a one-liner
> having to do with path separators, so I'm going to go ahead and ask for a
review
> now while I chase it down.  I'm sorry this is so big--I just started pulling
on
> a thread, and the whole thing unraveled.  It's really a much smaller change
than
> it looks, though, as the vast majority of the changes are just swapping the
data
> type on a parameter or two from FilePath to GURL.  There's a bit more to do
> later in the pepper section, as I've discussed with Trung offline.
> 
> Could you each please review the following sections:
> 
> Trung: Pepper/flash stuff
> Zelidrag: extension stuff
> Kinuko: filesystem stuff [pretty much everything else]
> Jam: whatever it takes to get owner approval on the content directories
> 
> ?
> 
> Thanks!
> 
> 	Eric

[ericu, 2011-04-05 00:06:35]

I've made a trivial change to file_system_operation_unittest.cc that I believe
fixes the bug I mentioned [it's already passed on my local Win+OSX machines, and
on the linux trybot, and the other bots are running now].  Zel, vtl, jam, please
take a look.

Thanks,

	Eric

[zel, 2011-04-06 21:43:00]

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (left):

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:150:
!local_provider_->IsAccessAllowed(local_url)) {
where do we do these checks now?

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (left):

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:75: bool CrackFileSystemPath(const
FilePath& path,
eric, my new CL relies heavily on this function - for both path parsing and
permission checks. I see that you are using CrackFileSystemURL() instead, but
that one does not check permissions - right?

[viettrungluu, 2011-04-06 22:38:25]

LGTM on the Pepper bits.

http://codereview.chromium.org/6767010/diff/28009/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

http://codereview.chromium.org/6767010/diff/28009/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:341: return
file_system_->root_url().Resolve(virtual_path_.substr(1));
Should you check that virtual_path_ isn't empty (maybe in the above if)? (I
suppose the old version had this potential issue too.)

[ericu, 2011-04-06 23:30:36]

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (left):

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:150:
!local_provider_->IsAccessAllowed(local_url)) {
On 2011/04/06 21:43:00, zel wrote:
> where do we do these checks now?

Ah, good catch.  I've added a new method to FileSystemPathManager that just does
this, and called it from FileSystemOperation::VerifyFileSystemPathForRead/Write.

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.h (left):

http://codereview.chromium.org/6767010/diff/28009/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.h:75: bool CrackFileSystemPath(const
FilePath& path,
On 2011/04/06 21:43:00, zel wrote:
> eric, my new CL relies heavily on this function - for both path parsing and
> permission checks. I see that you are using CrackFileSystemURL() instead, but
> that one does not check permissions - right?

Right--now that we've only got URLs, not paths, there's just the one method. 
Rather than put the new permission-check method in here, I decided it was
cleaner just to let this stay a pure cracking function, so if you want the
security checks, you'll have to call the new method directly.

http://codereview.chromium.org/6767010/diff/28009/webkit/plugins/ppapi/ppb_fi...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

http://codereview.chromium.org/6767010/diff/28009/webkit/plugins/ppapi/ppb_fi...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:341: return
file_system_->root_url().Resolve(virtual_path_.substr(1));
On 2011/04/06 22:38:25, viettrungluu wrote:
> Should you check that virtual_path_ isn't empty (maybe in the above if)? (I
> suppose the old version had this potential issue too.)

Fixed, thanks.

[zel, 2011-04-07 16:34:41]

LGTM

[jam, 2011-04-07 17:49:08]

content lgtm

[achuithb, 2011-04-07 22:40:36]

Not sure why this issue didn't get automatically closed:
http://src.chromium.org/viewvc/chrome?view=rev&revision=80833

[ericu, 2011-04-08 00:37:12]

I had a cygwin shell choke on an invisible password prompt during
commit.  However, the CL bounced due to an unrelated problem, so I'll
close it when the commit sticks.

On Thu, Apr 7, 2011 at 3:40 PM,  <achuith@chromium.org> wrote:
> Not sure why this issue didn't get automatically closed:
> http://src.chromium.org/viewvc/chrome?view=rev&revision=80833
>
> http://codereview.chromium.org/6767010/
>