Bug 47044 - Add FileSystemSync implementation for Worker
Summary: Add FileSystemSync implementation for Worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 46405
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-01 21:59 PDT by Kinuko Yasuda
Modified: 2010-10-06 16:09 PDT (History)
3 users (show)

See Also:


Attachments
Patch (88.33 KB, patch)
2010-10-02 00:23 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (87.54 KB, patch)
2010-10-02 01:05 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (88.41 KB, patch)
2010-10-05 00:09 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (93.29 KB, patch)
2010-10-06 01:32 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (93.25 KB, patch)
2010-10-06 01:50 PDT, Kinuko Yasuda
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>