Bug 85738 - Cleanup: Move FileSystem API type definitions into a separate header file
Summary: Cleanup: Move FileSystem API type definitions into a separate header file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-06 10:28 PDT by Kinuko Yasuda
Modified: 2012-05-07 21:13 PDT (History)
1 user (show)

See Also:


Attachments
Patch (43.42 KB, patch)
2012-05-06 10:40 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (41.53 KB, patch)
2012-05-06 11:21 PDT, Kinuko Yasuda
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-05-06 10:28:56 PDT
Cleanup: Move FileSystem API type definitions into a separate header file.

Currently they are defined in AsyncFileSystem.h (and AsyncFileSystemChromium.cpp for chromium), but by moving them into a separate header file we can:
- make it clearer that all FileSystem API types can be found in the single separate header file
- save including entire AsyncFileSystem definition just for referring type enum values
Comment 1 Kinuko Yasuda 2012-05-06 10:40:52 PDT
Created attachment 140424 [details]
Patch
Comment 2 Kinuko Yasuda 2012-05-06 10:41:53 PDT
Another patch separated out from http://webkit.org/b/84135 (for cross-filesystem support).
Comment 3 Kinuko Yasuda 2012-05-06 11:21:18 PDT
Created attachment 140429 [details]
Patch
Comment 4 David Levin 2012-05-06 11:35:33 PDT
Comment on attachment 140429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140429&action=review

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:59
>  const size_t externalPathPrefixLength = sizeof(externalPathPrefix) - 1;

I think the one place in the code that used this went away. Feel free to delete it.
Comment 5 David Levin 2012-05-06 11:39:26 PDT
Comment on attachment 140429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140429&action=review

> Source/WebCore/platform/FileSystemType.h:1
> +/*

I'm surprised that there aren't some build file changes to go along with this.

it would be nice to include it in build files. (As long as this not being the those files doesn't break anything and it doesn't seem to, I would be fine with that as a follow up patch.)
Comment 6 Kinuko Yasuda 2012-05-06 21:18:56 PDT
(In reply to comment #5)
> (From update of attachment 140429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140429&action=review
> 
> > Source/WebCore/platform/FileSystemType.h:1
> > +/*
> 
> I'm surprised that there aren't some build file changes to go along with this.
> 
> it would be nice to include it in build files. (As long as this not being the those files doesn't break anything and it doesn't seem to, I would be fine with that as a follow up patch.)

Yes I should have added the new header to the build files.  I'm going to make a follow up patch (unless this breaks other builds on waterfall).
Comment 7 Kinuko Yasuda 2012-05-07 00:38:59 PDT
Committed r116280: <http://trac.webkit.org/changeset/116280>
Comment 8 Eric U. 2012-05-07 12:57:50 PDT
Comment on attachment 140429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140429&action=review

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:124
>      return true;

This cleanup didn't make it through your CL-splitting quite right.  Now if we get an invalid URL that has no inner_url, we'll return true; it should be false.
Comment 9 David Levin 2012-05-07 13:11:57 PDT
(In reply to comment #8)
> (From update of attachment 140429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140429&action=review
> 
> > Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:124
> >      return true;
> 
> This cleanup didn't make it through your CL-splitting quite right.  Now if we get an invalid URL that has no inner_url, we'll return true; it should be false.

Thanks for looking it over again Eric!
Comment 10 Kinuko Yasuda 2012-05-07 20:36:00 PDT
Thanks Eric, I'm uploading a follow-up fix.
Comment 11 Kinuko Yasuda 2012-05-07 21:13:25 PDT
(In reply to comment #10)
> Thanks Eric, I'm uploading a follow-up fix.

Looks like the new patch (which is implementing crackFileSystemURL in DOMFileSystemChromium) seems to be handling this part correctly.  (Returning earlier if url.innerURL() is null)
Closing this again... thanks for the catch.