Handle has_more correctly

Handle has_more correctly

Return callbacksId from WebFileSystem::readDirectory().
Add WebFileSystem::waitForAdditionalResult().
Rewrite DirectoryReader and DirectoryReaderSync.

BUG=347900
TEST=run_webkit_tests.sh http/tests/filesystem/\*
TEST=run_webkit_tests.sh http/tests/inspector/filesystem/\*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168780

[kinuko, 2014-03-06 13:13:37]

lgtm

https://codereview.chromium.org/178333009/diff/80001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryReader.cpp (right):

https://codereview.chromium.org/178333009/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:70: RefPtr<DirectoryReader>
m_reader;
So in the current impl this callback (and DirectoryReader and DOMFileSystem)
will be kept alive as long as browser side's returning entries with
hasMore=true, whether it's desirable or not, is that right?  The new behavior is
at least much better than the current one, but it's probably worth having some
FIXME (and file a bug) to make it cancellable?

https://codereview.chromium.org/178333009/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:92: if (m_entriesCallback) {
nit: can you add a comment that non-null m_entriesCallback indicates multiple
concurrent readEntries() calls and we disallow it?

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
File public/platform/WebFileSystem.h (right):

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
public/platform/WebFileSystem.h:39: // TODO(hashimoto): Remove this #define.
nit: "TODO(hashimoto):" -> "FIXME:" in blink

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
public/platform/WebFileSystem.h:141: // WebFileSystemCallbacks::didFail() must
be called otherwise.
Can you add a comment about the return value?

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
public/platform/WebFileSystem.h:160: // |callbacksId| must be the value returned
by the original method call.
Can you add a comment about the return value? (Though it looks we don't use it)

[hashimoto, 2014-03-07 05:21:26]

https://codereview.chromium.org/178333009/diff/80001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryReader.cpp (right):

https://codereview.chromium.org/178333009/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:70: RefPtr<DirectoryReader>
m_reader;
On 2014/03/06 13:13:38, kinuko wrote:
> So in the current impl this callback (and DirectoryReader and DOMFileSystem)
> will be kept alive as long as browser side's returning entries with
> hasMore=true, whether it's desirable or not, is that right?  The new behavior
is
> at least much better than the current one, but it's probably worth having some
> FIXME (and file a bug) to make it cancellable?

Done.

Please note that EntriesCallbacks (beware of 's', not EntriesCallback inherited
above) also keeps RefPtr<DirectoryReaderBase>.

https://codereview.chromium.org/178333009/diff/80001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:92: if (m_entriesCallback) {
On 2014/03/06 13:13:38, kinuko wrote:
> nit: can you add a comment that non-null m_entriesCallback indicates multiple
> concurrent readEntries() calls and we disallow it?

Done.

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
File public/platform/WebFileSystem.h (right):

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
public/platform/WebFileSystem.h:39: // TODO(hashimoto): Remove this #define.
On 2014/03/06 13:13:38, kinuko wrote:
> nit: "TODO(hashimoto):" -> "FIXME:" in blink

Done.

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
public/platform/WebFileSystem.h:141: // WebFileSystemCallbacks::didFail() must
be called otherwise.
On 2014/03/06 13:13:38, kinuko wrote:
> Can you add a comment about the return value?

Done.

https://codereview.chromium.org/178333009/diff/80001/public/platform/WebFileS...
public/platform/WebFileSystem.h:160: // |callbacksId| must be the value returned
by the original method call.
On 2014/03/06 13:13:38, kinuko wrote:
> Can you add a comment about the return value? (Though it looks we don't use
it)

Done.

[hashimoto, 2014-03-07 05:23:48]

tkent@, could you take a look at public/platform/WebFileSystem.h as an owner?

[tkent, 2014-03-07 06:10:20]

public/platform lgtm

[tkent, 2014-03-07 06:10:26]

https://codereview.chromium.org/178333009/diff/110001/Source/modules/filesyst...
File Source/modules/filesystem/DirectoryReader.h (right):

https://codereview.chromium.org/178333009/diff/110001/Source/modules/filesyst...
Source/modules/filesystem/DirectoryReader.h:69: Vector<RefPtr<Entry>  >
m_entries;
Too many spaces between > and >

[hashimoto, 2014-03-07 07:12:10]

btw, it seems I forgot to include the link for the corresponding chrome-side
patch. (https://codereview.chromium.org/188533002/)
though, you may have already found this.
Sorry for any inconvenience.

https://codereview.chromium.org/178333009/diff/110001/Source/modules/filesyst...
File Source/modules/filesystem/DirectoryReader.h (right):

https://codereview.chromium.org/178333009/diff/110001/Source/modules/filesyst...
Source/modules/filesystem/DirectoryReader.h:69: Vector<RefPtr<Entry>  >
m_entries;
On 2014/03/07 06:10:27, tkent wrote:
> Too many spaces between > and >
Good catch, thanks.
Done.

[hashimoto, 2014-03-07 16:27:21]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:27:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/178333009/130001

[commit-bot: I haz the power, 2014-03-07 16:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:28:54]

Try jobs failed on following builders: linux_blink_rel

[hashimoto, 2014-03-08 04:21:43]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-03-08 04:21:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/178333009/130001

[commit-bot: I haz the power, 2014-03-08 09:33:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 09:33:08]

Try jobs failed on following builders: linux_blink_rel

[hashimoto, 2014-03-08 10:02:59]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-03-08 10:04:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/178333009/130001

[commit-bot: I haz the power, 2014-03-08 10:48:18]

Change committed as 168780

[haraken, 2014-03-10 03:54:11]

This CL was reverted in r168786 because it broke all oilpan builds. I fixed the
oilpan issue and relanded this CL in r168810.

I'm sorry that oilpan disturbed your development.

https://codereview.chromium.org/178333009/diff/130001/Source/modules/filesyst...
File Source/modules/filesystem/DirectoryReader.h (right):

https://codereview.chromium.org/178333009/diff/130001/Source/modules/filesyst...
Source/modules/filesystem/DirectoryReader.h:70: RefPtr<FileError> m_error;

Since FileError is on the oilpan heap, this should be
RefPtrWillBePersistent<FileError>.

[hashimoto, 2014-03-10 04:46:14]

https://codereview.chromium.org/178333009/diff/130001/Source/modules/filesyst...
File Source/modules/filesystem/DirectoryReader.h (right):

https://codereview.chromium.org/178333009/diff/130001/Source/modules/filesyst...
Source/modules/filesystem/DirectoryReader.h:70: RefPtr<FileError> m_error;
On 2014/03/10 03:54:12, haraken wrote:
> 
> Since FileError is on the oilpan heap, this should be
> RefPtrWillBePersistent<FileError>.

Thank you for the fix!

It seems that what I should have done is to look at FileError.h and follow the
class's inheritance chain until it reaches 
RefCountedWillBeGarbageCollectedFinalized, then look up the table at
http://www.chromium.org/blink/blink-gc.
Though it'd be even nicer if this could be caught by the compiler with something
like static assertions.