[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





--- Comment #14 from Kinuko Yasuda <kinuko at chromium.org>  2010-10-05 05:25:19 PST ---
(In reply to comment #11)
> (From update of attachment 69567 [details])
> 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.

Fixed.  (Introduced a static getFlags function)

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

Fixed.

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

Fixed.

> > WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109
> > +    }
> 
> Duplicate code. consider a common function.

Fixed.  (Introduced a static getFlags 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.

Got it.  (For now I removed this code -- I'll think over the constructor.)

> > 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 :).

Removed the comment.

> > 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?

I removed this constructor for now.

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

Fixed.

> > WebCore/fileapi/DirectoryEntrySync.h:61
> > +    DirectoryEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);
> 
> Please remove variable names which add no information. fileSystem

Fixed.

> > WebCore/fileapi/DirectoryReader.h:60
> > +    DirectoryReader(DOMFileSystemBase* fileSystem, const String& fullPath);
> 
> Please remove variable names which add no information. fileSystem

Fixed.

> > 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.)

Changed the method and variable names to hasMoreEntries.

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

Fixed.

> > WebCore/fileapi/EntryArraySync.h:53
> > +    void set(unsigned index, PassRefPtr<EntrySync> entry);
> 
> Please remove variable names which add no information. entry

Fixed.  (Actually set() wasn't implemented anywhere.  Removed)

> > WebCore/fileapi/EntrySync.h:59
> > +    EntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);
> 
> Please remove variable names which add no information. fileSystem

Fixed.

> > WebCore/fileapi/FileEntrySync.h:54
> > +    FileEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);
> 
> Please remove variable names which add no information. fileSystem

Fixed.

> > WebCore/fileapi/FileEntrySync.idl:40
> > +        // File file() raises (FileException);
> 
> Please remove commented out code.

Fixed.

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