Bug 56704 – [fileapi/chromium] Fetch platform path using GetMetadata before creating File from FileEntry*

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

The implementation of the filesystem API currently requires that the embedder use real system paths for the underlying implementation.  This precludes implementations that e.g. use a database for underlying storage, in which case there aren't "real" platform paths for directories.

Chromium is about to switch to that type of implementation.  In order to make the WebCore parts portable, I'm going to switch all the cached platform paths to be filesystem URLs instead, as that's guaranteed to be portable.  However, when you create a File from a FileEntry, you need a real path to wherever the data's stored [unless you want to virtualize that too, but I'm not going to go that general until I know somebody actually needs it].

This change adds an alternative way to get the path, just for that call site.  Once this patch is in, I'll add the Chromium code that fills in the data, so it gets used.  Then I'll be able to do a cleanup pass [or 3], both removing the old code path and cleaning up the data types and variable names that refer to platform paths when they should refer to URLs.

[Eric U.]

Created attachment 86274 [details]
Patch to make supplying a platform path optional; once chromium's caught up, it'll be mandatory.

For context, the chromium change is up at http://codereview.chromium.org/6603034/.

[WebKit Review Bot]

Attachment 86274 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/fileapi/DOMFileSystem.cpp:119:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/fileapi/DOMFileSystem.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/fileapi/DOMFileSystem.cpp:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/fileapi/DOMFileSystemSync.cpp:89:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/fileapi/DOMFileSystemSync.cpp:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/fileapi/DOMFileSystemSync.cpp:126:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/fileapi/DOMFileSystemSync.cpp:131:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 30 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 86274 [details]
Patch to make supplying a platform path optional; once chromium's caught up, it'll be mandatory.

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

Nothing huge but a ton of little things so it would be nice to see it one more time.

> WebCore/fileapi/DOMFileSystemSync.cpp:83
> +          : m_failed(false)

4 space indent.

> WebCore/fileapi/DOMFileSystemSync.cpp:89
> +    static PassOwnPtr<GetPathHelper> create(GetPathResult* result) {

{ placement. lots of places.


Should be PassRefPtr<GetPathResult>.

> WebCore/fileapi/DOMFileSystemSync.cpp:95
> +      ASSERT_NOT_REACHED();

indent. Lots of places.

> WebCore/fileapi/DOMFileSystemSync.cpp:107
> +    virtual void didReadDirectoryEntries(bool hasMore)

You'll need to comment the variable name (because it is unused and that is going to give you a compile error on some platforms).  Several places.

> WebCore/fileapi/DOMFileSystemSync.cpp:130
> +    GetPathHelper(GetPathResult* result)

Should be PassRefPtr<GetPathResult>.

> WebCore/fileapi/DOMFileSystemSync.cpp:143
> +    RefPtr<GetPathHelper::GetPathResult> result(adoptRef(new GetPathHelper::GetPathResult()));

adoptRef outside of constructor is a really bad sign. The constructor should be private and there should be a static create method which returns a PassRefPtr.

> WebCore/fileapi/DOMFileSystemSync.cpp:144
> +    m_asyncFileSystem->readMetadata(platformPath, GetPathHelper::create(result.get()));

I wonder why GetPathHelper::GetPathResult even exists.  Why can't the data just be stored in GetPathHelper and get rid of GetPathHelper::GetPathResult?

> WebCore/fileapi/DOMFileSystem.cpp:119
> +    static PassOwnPtr<GetPathCallback> create(DOMFileSystem* filesystem, const String& path, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback) {

{ placement...

> WebCore/fileapi/DOMFileSystem.cpp:132
> +      : FileSystemCallbacksBase(errorCallback)

spacing.

[David Levin]

Comment on attachment 86274 [details]
Patch to make supplying a platform path optional; once chromium's caught up, it'll be mandatory.

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

> WebCore/fileapi/DOMFileSystemSync.cpp:77
> +    struct GetPathResult : public RefCounted<GetPathResult> {

Also for ref counted objects, please consider making the destructor private and making RefCounted<> a friend class.

[Eric U.]

Created attachment 86337 [details]
Fixed stylistic problems

I think I've gotten all the fixes in.  Sorry about all the style kruft--I'm doing this on my Windows box, where I'm not set up to run check-webkit-style, and I've been doing too much Chromium code lately.

The reason we need GetPathResult is that GetPathHelper will be deleted [by the caller] as soon as the callback is called--it's just the way that interface works.

[WebKit Review Bot]

Attachment 86337 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/fileapi/DOMFileSystemSync.cpp:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric U.]

Created attachment 86338 [details]
Fixed the last style failure.

Ah--missed one brace.  Fixed.

[David Levin]

Comment on attachment 86338 [details]
Fixed the last style failure.

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

> WebCore/fileapi/DOMFileSystemSync.cpp:79
> +        bool m_failed;

Usually member variables come after methods.

> WebCore/fileapi/DOMFileSystemSync.cpp:85
> +          return adoptRef(new GetPathResult());

two spaces.

> WebCore/fileapi/DOMFileSystemSync.cpp:92
> +        }

Usually a blank line here.

> WebCore/fileapi/DOMFileSystemSync.cpp:99
> +    static PassOwnPtr<GetPathHelper> create(GetPathResult* result)

Should be PassRefPtr<GetPathResult> result

> WebCore/fileapi/DOMFileSystemSync.cpp:129
> +    // Called when there was an error.

Does this comment add any information? (The method name is didFail.)

> WebCore/fileapi/DOMFileSystemSync.cpp:160
> +    m_asyncFileSystem->readMetadata(platformPath, GetPathHelper::create(result.get()));

I doubt you need result.get().

> WebCore/fileapi/DOMFileSystem.cpp:119
> +    static PassOwnPtr<GetPathCallback> create(DOMFileSystem* filesystem, const String& path, PassRefPtr<FileCallback> successCallback, PassRefPtr<ErrorCallback> errorCallback)

Since you put filesystem in a RefPtr, it should be passed in via a PassRefPtr.

> WebCore/fileapi/DOMFileSystem.cpp:139
> +    }

Usually a blank line here.

> WebCore/fileapi/DOMFileSystem.cpp:142
> +    PassRefPtr<FileCallback> m_successCallback;

Member variables should be RefPtr not PassRefPtr.

[Eric U.]

Created attachment 86344 [details]
Addressed review comments.

[WebKit Commit Bot]

Comment on attachment 86344 [details]
Addressed review comments.

Clearing flags on attachment: 86344

Committed r81599: <http://trac.webkit.org/changeset/81599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81599 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
http/tests/inspector/network/network-size.html