Bug 47044

Summary: Add FileSystemSync implementation for Worker
Product: WebKit Reporter: Kinuko Yasuda <kinuko>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ericu, levin, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 46405    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch levin: review+

Description Kinuko Yasuda 2010-10-01 21:59:06 PDT
Add FileSystemSync implementation for Worker.
Comment 1 Kinuko Yasuda 2010-10-02 00:23:00 PDT
Created attachment 69568 [details]
Patch
Comment 2 Kinuko Yasuda 2010-10-02 01:05:45 PDT
Created attachment 69570 [details]
Patch
Comment 3 Kinuko Yasuda 2010-10-05 00:09:27 PDT
Created attachment 69761 [details]
Patch

Fixed nits.
Comment 4 David Levin 2010-10-05 03:36:36 PDT
Comment on attachment 69761 [details]
Patch

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

Just the comments from my phase 1 pass: nits.

> WebCore/fileapi/DOMFileSystemBase.h:48
> +class EntriesCallback;

out of order.

> WebCore/fileapi/DOMFileSystemBase.h:74
> +    bool readDirectory(DirectoryReaderBase* reader, const String& path, PassRefPtr<EntriesCallback>, PassRefPtr<ErrorCallback>);

s/src/source/
Remove param names that add no information (entry, base, reader).

> WebCore/fileapi/Entry.h:45
> +class EntrySync;

out of order.

> WebCore/fileapi/EntryArraySync.h:51
> +    static PassRefPtr<EntryArraySync> create(EntryArray* entries);

remove param name.

> WebCore/fileapi/EntryBase.h:28
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.  */

Mistaken change?

> WebCore/fileapi/EntrySync.h:50
> +    static PassRefPtr<EntrySync> create(EntryBase* entry);

param name.
Comment 5 David Levin 2010-10-05 14:28:49 PDT
Comment on attachment 69761 [details]
Patch

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

Just a few things to address.

> WebCore/fileapi/DirectoryEntry.cpp:54
> +    filesystem()->scheduleCallback(errorCallback, FileError::create(error));

Please get rid of the macro.

> WebCore/fileapi/Entry.cpp:51
> +    filesystem()->scheduleCallback(errorCallback, FileError::create(error));

Please get rid of the macro.

>> WebCore/fileapi/EntrySync.h:50
>> +    static PassRefPtr<EntrySync> create(EntryBase* entry);
> 
> param name.

(remove the param name)

> WebCore/fileapi/FileEntry.h:-42
> -class DOMFileSystem;

Why didn't this become DOMFileSystemBase?

> WebCore/platform/AsyncFileSystem.h:63
> +    // This should return false if there're no pending operations.

s/there're/there are/

> WebCore/workers/WorkerContext.h:51
> +    class DOMFileSystemSync;

out of order.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:77
> +        return false;

It seems like m_bridgeForLastOperation should be cleared at the end of this.

If that is true, then consider
RefPtr<> bridge = m_bridge.release();

and use bridge. Now bridge will get deref'ed when this function is exited.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:62
> +    virtual bool waitCompletion();

s/waitCompletion/waitForOperationToComplete/

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:82
> +    RefPtr<WebKit::WorkerFileSystemCallbacksBridge> m_bridgeForLastOperation;

m_bridgeForCurrentOperation?
Comment 6 Kinuko Yasuda 2010-10-06 01:32:43 PDT
Created attachment 69899 [details]
Patch
Comment 7 Kinuko Yasuda 2010-10-06 01:50:56 PDT
Created attachment 69901 [details]
Patch
Comment 8 David Levin 2010-10-06 01:59:18 PDT
Comment on attachment 69901 [details]
Patch

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

Just a few nits to address.

> WebCore/fileapi/DOMFileSystemBase.h:72
> +    bool getFile(const EntryBase* base, const String& path, PassRefPtr<Flags>, PassRefPtr<EntryCallback>, PassRefPtr<ErrorCallback>);

remove "base"

> WebCore/fileapi/DOMFileSystemBase.h:73
> +    bool getDirectory(const EntryBase* base, const String& path, PassRefPtr<Flags>, PassRefPtr<EntryCallback>, PassRefPtr<ErrorCallback>);

remove "base"

> WebCore/fileapi/FileEntry.h:-42
> -class DOMFileSystem;

Please consider adding this back but as Base.
Comment 9 Kinuko Yasuda 2010-10-06 16:09:10 PDT
Committed r69249: <http://trac.webkit.org/changeset/69249>