Bug 85738 – Cleanup: Move FileSystem API type definitions into a separate header file

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

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

[Kinuko Yasuda]

Created attachment 140424 [details]
Patch

[Kinuko Yasuda]

Another patch separated out from http://webkit.org/b/84135 (for cross-filesystem support).

[Kinuko Yasuda]

Created attachment 140429 [details]
Patch

[David Levin]

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.

[David Levin]

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.)

[Kinuko Yasuda]

(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).

[Kinuko Yasuda]

Committed r116280: <http://trac.webkit.org/changeset/116280>

[Eric U.]

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.

[David Levin]

(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!

[Kinuko Yasuda]

Thanks Eric, I'm uploading a follow-up fix.

[Kinuko Yasuda]

(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.