[Webkit-unassigned] [Bug 46405] Add idl and mock classes for FileSystemSync for FileSystem API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46405


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69567|review?                     |review-, commit-queue-
               Flag|                            |




--- Comment #11 from David Levin <levin at chromium.org>  2010-10-05 03:29:19 PST ---
(From update of attachment 69567)
View in context: https://bugs.webkit.org/attachment.cgi?id=69567&action=review

> WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:89
> +        flags = toFlags(exec->argument(1));

Up to here it is the same as the previous function.  Please consider a common function to do this stuff.

This appears to be missing:
  if (exec->hadException())
        return jsUndefined();

here.

> WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:56
> +    STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, path, args[0]);

indent.

> WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109
> +    }

Duplicate code. consider a common function.

> WebCore/fileapi/DOMFileSystemSync.cpp:43
> +    return adoptRef(new DOMFileSystemSync(fileSystem->m_name, fileSystem->m_asyncFileSystem.leakPtr()));

This shouldn't be a leakPtr(). This should be a releasePtr.

leakPtr is only use when you want the OwnPtr to stop holding onto to the point and you want the raw pointer. You will manage the memory in some other way.  Here you are passing the point to a PassOwnPtr, so releasePtr is perfect.

> WebCore/fileapi/DOMFileSystemSync.cpp:60
> +} // namespace

These // namespace comments aren't necessary in WebKit as far as I know. However, if you are going to include it, make it correct :).

> WebCore/fileapi/DOMFileSystemSync.h:51
> +    // This call is destructive; the argument fileSystem will become unusable.

This sounds scary. It makes me wonder why it doesn't take a PassOwnPtr for fileSystem. If fileSystem is unusable afterward. is there a way to help callers to do the right thing and not use it again?

> WebCore/fileapi/DOMFileSystemSync.h:62
> +} // namespace

Ditto.

> WebCore/fileapi/DirectoryEntrySync.h:61
> +    DirectoryEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/DirectoryReader.h:60
> +    DirectoryReader(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/DirectoryReaderBase.h:46
> +    void setHasMore(bool hasMore) { m_hasMore = hasMore; }

What does "hasMore" mean? has more what? (My initial reaction is cowbell since that is the current meme.)

> WebCore/fileapi/DirectoryReaderSync.h:54
> +    DirectoryReaderSync(DOMFileSystemBase* fileSystem, const String& fullPath);

fileSystem param name.

> WebCore/fileapi/EntryArraySync.h:53
> +    void set(unsigned index, PassRefPtr<EntrySync> entry);

Please remove variable names which add no information. entry

> WebCore/fileapi/EntrySync.h:59
> +    EntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/FileEntrySync.h:54
> +    FileEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/FileEntrySync.idl:40
> +        // File file() raises (FileException);

Please remove commented out code.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.