Bug 60218 - [fileapi/Chromium] Clean up filesystem base path code
Summary: [fileapi/Chromium] Clean up filesystem base path code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-04 15:29 PDT by Eric U.
Modified: 2011-06-14 13:01 PDT (History)
5 users (show)

See Also:


Attachments
Patch (78.45 KB, patch)
2011-05-05 11:35 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Patch (78.46 KB, patch)
2011-05-11 16:18 PDT, Eric U.
fishd: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch rolling in code review feedback. (78.43 KB, patch)
2011-06-02 16:08 PDT, Eric U.
no flags Details | Formatted Diff | Diff
Removed CrossThreadCopier specializations (77.04 KB, patch)
2011-06-13 18:16 PDT, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2011-05-04 15:29:58 PDT
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.
Comment 1 Eric U. 2011-05-05 11:35:22 PDT
Created attachment 92436 [details]
Patch
Comment 2 Eric U. 2011-05-05 11:35:47 PDT
This patch depends on http://codereview.chromium.org/6904063/ going in first.
Comment 3 WebKit Review Bot 2011-05-05 12:51:17 PDT
Attachment 92436 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8557986
Comment 4 Adam Barth 2011-05-05 13:28:10 PDT
(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.  :(
Comment 5 Kinuko Yasuda 2011-05-06 00:41:17 PDT
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?
Comment 6 Eric U. 2011-05-11 16:18:37 PDT
Created attachment 93206 [details]
Patch
Comment 7 WebKit Review Bot 2011-05-11 16:54:46 PDT
Comment on attachment 93206 [details]
Patch

Attachment 93206 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8686541
Comment 8 Darin Fisher (:fishd, Google) 2011-05-16 15:03:08 PDT
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.
Comment 9 Kinuko Yasuda 2011-05-16 18:51:47 PDT
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.
Comment 10 Darin Fisher (:fishd, Google) 2011-05-17 09:10:34 PDT
(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.
Comment 11 Eric U. 2011-06-02 16:08:32 PDT
Created attachment 95829 [details]
Patch rolling in code review feedback.
Comment 12 Eric U. 2011-06-02 16:09:25 PDT
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?
Comment 13 Darin Fisher (:fishd, Google) 2011-06-13 14:17:02 PDT
(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.
Comment 14 Darin Fisher (:fishd, Google) 2011-06-13 14:19:23 PDT
(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).
Comment 15 Eric U. 2011-06-13 18:16:03 PDT
Created attachment 97051 [details]
Removed CrossThreadCopier specializations

I've removed the CrossThreadCopier specializations; thanks for the tip.
Comment 16 Eric U. 2011-06-13 18:17:54 PDT
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.
Comment 17 Kinuko Yasuda 2011-06-13 20:10:56 PDT
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?
Comment 18 Darin Fisher (:fishd, Google) 2011-06-14 10:49:40 PDT
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.
Comment 19 Eric U. 2011-06-14 12:19:53 PDT
(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.
Comment 20 WebKit Review Bot 2011-06-14 13:01:32 PDT
Comment on attachment 97051 [details]
Removed CrossThreadCopier specializations

Clearing flags on attachment: 97051

Committed r88844: <http://trac.webkit.org/changeset/88844>
Comment 21 WebKit Review Bot 2011-06-14 13:01:38 PDT
All reviewed patches have been landed.  Closing bug.