[webkit-reviews] review granted: [Bug 47535] DOMFileSystem's reference should be kept while there're any active Entries/callbacks : [Attachment 71966] Patch

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has granted Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 47535: DOMFileSystem's reference should be kept while there're any active
Entries/callbacks
https://bugs.webkit.org/show_bug.cgi?id=47535

Attachment 71966: Patch
https://bugs.webkit.org/attachment.cgi?id=71966&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71966&action=review

r=me, but please address my comments before submitting.

> LayoutTests/fast/filesystem/script-tests/filesystem-reference.js:44
> +function runTest1(root)

nit/optional: i'd make runTest2() come after runTest1(), and runTest3() come
after runTest2(). this way, it's easier (i think) to read the test from top to
bottom.

> WebCore/fileapi/DOMFileSystemBase.cpp:38
> +#include "DirectoryReaderBase.h"

why do we need this #include? Changing DirectoryReaderBase* to
PassRefPtr<DirectoryReaderBase> shouldn't require any additional #includes (and
the list of #includes in DOMFileSystemBase.h didn't change either).

> WebCore/fileapi/DirectoryEntry.cpp:51
> +    return DirectoryReader::create(filesystem(), m_fullPath);

i think we should be consistent in how we use the fields declared in base
classes (EntryBase): either keep them protected, and use m_fileSystem.get() and
m_fullPath here, or we should make them private and add public/protected
getters and use filesystem() and fullPath() here.

> WebCore/fileapi/DirectoryEntrySync.cpp:51
> +    return DirectoryReaderSync::create(filesystem(), m_fullPath);

same comment: let's make this consistent; either use the members directly, or
provide getters for all of them.

> WebCore/fileapi/EntrySync.cpp:50
> +	   return adoptRef(new FileEntrySync(entry->filesystem(),
entry->m_fullPath));

ditto.

> WebCore/fileapi/EntrySync.cpp:102
> +    return DirectoryEntrySync::create(filesystem(), parentPath);

ditto.