[fileapi/Chromium] Clean up filesystem base path code

• Component: WebKit API
• Status: RESOLVED FIXED

[Eric U.]

The current implementation of the FileSystem API forces ports to hold a real file path [or what claims to be a real file path] for the root of the filesystem.  It then assumes that all virtual paths can be made into real paths by tacking the virtual path onto that root path.

This is messy/wrong because their might not actually be a real local path [if the data's actually stored in a database] and the "root path" that gets returned is currently a URL in the only implementation [chromium].

I'm coding up a cleanup that will switch from paths to URLs for portability--we're actually already doing that under the covers, but this will clean up the data types.  It will also push the holding of that root path [root URL, now] out of AsyncFileSystem and down into the implementation [currently only AsyncFileSystemChromium and WorkerAsyncFileSystemChromium], giving embedders more freedom of implementation.

[Eric U.]

Created attachment 92436 [details]
Patch

[Eric U.]

This patch depends on http://codereview.chromium.org/6904063/ going in first.

[WebKit Review Bot]

Attachment 92436 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8557986

[Adam Barth]

(In reply to comment #2)
> This patch depends on http://codereview.chromium.org/6904063/ going in first.

There isn't a way to tell the bot about that.  Sorry for the spam.  :(

[Kinuko Yasuda]

Comment on attachment 92436 [details]
Patch

Thanks for cleaning up the code!

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

> Source/WebCore/platform/AsyncFileSystem.cpp:58
> +    callbacks->didOpenFileSystem("", 0);

Maybe we should just dispatch didFail with NOT_SUPPORTED_ERR here?

> Source/WebCore/platform/AsyncFileSystem.h:70
> +    static PassOwnPtr<AsyncFileSystem> create(Type);

I guess now we can make this class a pure abstract class and get rid of create() and the constructor?

[Eric U.]

Created attachment 93206 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93206 [details]
Patch

Attachment 93206 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8686541

[Darin Fisher (:fishd, Google)]

Comment on attachment 93206 [details]
Patch

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

> Source/WebCore/platform/AsyncFileSystem.cpp:58
> +    callbacks->didFail(NOT_SUPPORTED_ERR);

isn't it risky to run this callback synchronously?  i'd worry about re-entrancy bugs,
but maybe this is all dead code anyways?

> Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:225
> +KURL WorkerAsyncFileSystemChromium::virtualPathToFileSystemURL(const String& virtualPath) const

this looks like duplicated code.  is the intent to eventually delete this function and its replica?

> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:85
> +      return WebKit::WebURL(url);

indentation nit.

also, please note that the copy-constructor performs a shallow copy of the underlying
string containing the URL text.  that shallow copy is implemented using a reference
counting that is not thread safe!  are you sure this is the proper way to copy the
WebURL?  it seems like you should use KURL::copy() here.

> Source/WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:58
> +      return WebKit::WebURL(url);

indentation nit.

duplication of code nit.

same question about thread unsafe reference counting.

[Kinuko Yasuda]

Comment on attachment 93206 [details]
Patch

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

>> Source/WebCore/platform/AsyncFileSystem.cpp:58
>> +    callbacks->didFail(NOT_SUPPORTED_ERR);
> 
> isn't it risky to run this callback synchronously?  i'd worry about re-entrancy bugs,
> but maybe this is all dead code anyways?

The caller of this method (i.e. LocalFileSystem) calls openFileSystem in an async indirect task to avoid re-entrance issue so I hope it'll be safe.

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> (From update of attachment 93206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93206&action=review
> 
> >> Source/WebCore/platform/AsyncFileSystem.cpp:58
> >> +    callbacks->didFail(NOT_SUPPORTED_ERR);
> > 
> > isn't it risky to run this callback synchronously?  i'd worry about re-entrancy bugs,
> > but maybe this is all dead code anyways?
> 
> The caller of this method (i.e. LocalFileSystem) calls openFileSystem in an async indirect task to avoid re-entrance issue so I hope it'll be safe.

I see, so it should be safe.  Again, if the intent is anyways for this code to be deleted, then I really don't care about the way it behaves.  It is just ordinarily bad form for a method which is supposed to complete asynchronously to sometimes complete synchronously via callback.  That can complicate consumer code and lead to bugs.

[Eric U.]

Created attachment 95829 [details]
Patch rolling in code review feedback.

[Eric U.]

Comment on attachment 93206 [details]
Patch

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

>>>> Source/WebCore/platform/AsyncFileSystem.cpp:58
>>>> +    callbacks->didFail(NOT_SUPPORTED_ERR);
>>> 
>>> isn't it risky to run this callback synchronously?  i'd worry about re-entrancy bugs,
>>> but maybe this is all dead code anyways?
>> 
>> The caller of this method (i.e. LocalFileSystem) calls openFileSystem in an async indirect task to avoid re-entrance issue so I hope it'll be safe.
> 
> I see, so it should be safe.  Again, if the intent is anyways for this code to be deleted, then I really don't care about the way it behaves.  It is just ordinarily bad form for a method which is supposed to complete asynchronously to sometimes complete synchronously via callback.  That can complicate consumer code and lead to bugs.

Agreed in general, but in this case this is just placeholder code for a potential future WebCore implementation of FileSystem, and will get replaced before anyone ever compiles it in.

>> Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:225
>> +KURL WorkerAsyncFileSystemChromium::virtualPathToFileSystemURL(const String& virtualPath) const
> 
> this looks like duplicated code.  is the intent to eventually delete this function and its replica?

If you're referring to just virtualPathToFileSystemURL, which now lives in two places: it's needed and will not be deleted.
We could get by with a single copy of it if we extracted a common base from WorkerAsyncFileSystemChromium and AsyncFileSystemChromium.  The method moved from a common base that no longer has the member [m_filesystemRootURL] on which it depends.  But it's a very small amount of code, and it would actually end up making the code bigger to pull that class out.  For now I think we should just accept the small duplication.

>> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:85
>> +      return WebKit::WebURL(url);
> 
> indentation nit.
> 
> also, please note that the copy-constructor performs a shallow copy of the underlying
> string containing the URL text.  that shallow copy is implemented using a reference
> counting that is not thread safe!  are you sure this is the proper way to copy the
> WebURL?  it seems like you should use KURL::copy() here.

Done, thanks for catching that.

I see that we can only use that code if WEBKIT_IMPLEMENTATION.  Where is that valid?

>> Source/WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:58
>> +      return WebKit::WebURL(url);
> 
> indentation nit.
> 
> duplication of code nit.
> 
> same question about thread unsafe reference counting.

Is there a good place to put this code already, or should I just make a new file in WebKit/chromium/public?

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> (From update of attachment 93206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93206&action=review
> 
> >>>> Source/WebCore/platform/AsyncFileSystem.cpp:58
> >>>> +    callbacks->didFail(NOT_SUPPORTED_ERR);
> >>> 
> >>> isn't it risky to run this callback synchronously?  i'd worry about re-entrancy bugs,
> >>> but maybe this is all dead code anyways?
> >> 
> >> The caller of this method (i.e. LocalFileSystem) calls openFileSystem in an async indirect task to avoid re-entrance issue so I hope it'll be safe.
> > 
> > I see, so it should be safe.  Again, if the intent is anyways for this code to be deleted, then I really don't care about the way it behaves.  It is just ordinarily bad form for a method which is supposed to complete asynchronously to sometimes complete synchronously via callback.  That can complicate consumer code and lead to bugs.
> 
> Agreed in general, but in this case this is just placeholder code for a potential future WebCore implementation of FileSystem, and will get replaced before anyone ever compiles it in.

OK


> >> Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:225
> >> +KURL WorkerAsyncFileSystemChromium::virtualPathToFileSystemURL(const String& virtualPath) const
> > 
> > this looks like duplicated code.  is the intent to eventually delete this function and its replica?
> 
> If you're referring to just virtualPathToFileSystemURL, which now lives in two places: it's needed and will not be deleted.
> We could get by with a single copy of it if we extracted a common base from WorkerAsyncFileSystemChromium and AsyncFileSystemChromium.  The method moved from a common base that no longer has the member [m_filesystemRootURL] on which it depends.  But it's a very small amount of code, and it would actually end up making the code bigger to pull that class out.  For now I think we should just accept the small duplication.

Yeah, I was referring to that function.  OK.  Duplicated code tends to really
annoy me, so I will complain each time I see it :)  I don't have enough context
to suggest a better solution.


> 
> >> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:85
> >> +      return WebKit::WebURL(url);
> > 
> > indentation nit.
> > 
> > also, please note that the copy-constructor performs a shallow copy of the underlying
> > string containing the URL text.  that shallow copy is implemented using a reference
> > counting that is not thread safe!  are you sure this is the proper way to copy the
> > WebURL?  it seems like you should use KURL::copy() here.
> 
> Done, thanks for catching that.
> 
> I see that we can only use that code if WEBKIT_IMPLEMENTATION.  Where is that valid?

It is defined when compiling files under chromium/src/.


> 
> >> Source/WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:58
> >> +      return WebKit::WebURL(url);
> > 
> > indentation nit.
> > 
> > duplication of code nit.
> > 
> > same question about thread unsafe reference counting.
> 
> Is there a good place to put this code already, or should I just make a new file in WebKit/chromium/public?

Stepping back, I wonder why we need to define CrossThreadCopier for WebURL.  Why can't
we just leverage the one defined for KURL?  WebURL can be implicitly converted to KURL.
It is not clear what code causes you to need this CrossThreadCopier.

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> > >> Source/WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:58
> > >> +      return WebKit::WebURL(url);
> > > 
> > > indentation nit.
> > > 
> > > duplication of code nit.
> > > 
> > > same question about thread unsafe reference counting.
> > 
> > Is there a good place to put this code already, or should I just make a new file in WebKit/chromium/public?
> 
> Stepping back, I wonder why we need to define CrossThreadCopier for WebURL.  Why can't
> we just leverage the one defined for KURL?  WebURL can be implicitly converted to KURL.
> It is not clear what code causes you to need this CrossThreadCopier.

Ah, initOnMainThread should just take a KURL parameter instead of a WebURL parameter.
In general, WebURL (and WebString) are just types used with API calls.  You should
always quickly convert over to WebCore / WTF types once you are inside the implementation
of WebKit.  That way you can benefit from the machinery that already exists for WebCore /
WTF types.  This is why WebURL (and WebString) auto-convert to KURL (and String).

[Eric U.]

Created attachment 97051 [details]
Removed CrossThreadCopier specializations

I've removed the CrossThreadCopier specializations; thanks for the tip.

[Eric U.]

Comment on attachment 93206 [details]
Patch

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

>>>> Source/WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:58
>>>> +      return WebKit::WebURL(url);
>>> 
>>> indentation nit.
>>> 
>>> duplication of code nit.
>>> 
>>> same question about thread unsafe reference counting.
>> 
>> Is there a good place to put this code already, or should I just make a new file in WebKit/chromium/public?
> 
> Stepping back, I wonder why we need to define CrossThreadCopier for WebURL.  Why can't
> we just leverage the one defined for KURL?  WebURL can be implicitly converted to KURL.
> It is not clear what code causes you to need this CrossThreadCopier.

Changed a bunch of WebURLs to KURL, and removed the CrossThreadCopiers.

[Kinuko Yasuda]

Comment on attachment 93206 [details]
Patch

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

>>>> Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:225
>>>> +KURL WorkerAsyncFileSystemChromium::virtualPathToFileSystemURL(const String& virtualPath) const
>>> 
>>> this looks like duplicated code.  is the intent to eventually delete this function and its replica?
>> 
>> If you're referring to just virtualPathToFileSystemURL, which now lives in two places: it's needed and will not be deleted.
>> We could get by with a single copy of it if we extracted a common base from WorkerAsyncFileSystemChromium and AsyncFileSystemChromium.  The method moved from a common base that no longer has the member [m_filesystemRootURL] on which it depends.  But it's a very small amount of code, and it would actually end up making the code bigger to pull that class out.  For now I think we should just accept the small duplication.
> 
> Yeah, I was referring to that function.  OK.  Duplicated code tends to really
> annoy me, so I will complain each time I see it :)  I don't have enough context
> to suggest a better solution.

Maybe this could be defined as a static method or a regular function so that it can be used (with additional root url parameter) in both places?

[Darin Fisher (:fishd, Google)]

Comment on attachment 97051 [details]
Removed CrossThreadCopier specializations

R=me, but please explore Kinuko's suggestion if that can help us avoid the code duplication.

[Eric U.]

(In reply to comment #18)
> (From update of attachment 97051 [details])
> R=me, but please explore Kinuko's suggestion if that can help us avoid the code duplication.

It's a decent idea in the general case, but I think here it doesn't work.  It would remove one 3-line method, but add a parameter to 26 call sites, leading to bulkier and [I think] uglier code overall.  If this method should ever get more complicated, we should revisit the idea, though.

[WebKit Review Bot]

Comment on attachment 97051 [details]
Removed CrossThreadCopier specializations

Clearing flags on attachment: 97051

Committed r88844: <http://trac.webkit.org/changeset/88844>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.