[webkit-reviews] review denied: [Bug 43985] Change FileStream implementation to prepare for blob resource handling : [Attachment 64369] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 43985: Change FileStream implementation to prepare for blob resource
handling
https://bugs.webkit.org/show_bug.cgi?id=43985

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

------- Additional Comments from David Levin <levin at chromium.org>
Just a few issues to address.


Not the focus of this patch but how does lifetime of FileStream work? I see
tasks posted from FileStreamProxy to be handled by FileStream on a different
thread in WebCore/html/FileStreamProxy.cpp

It looks like the lifetime of FileStream held by FileStreamProxy which is held
by FileReader (which is basically held by js), so the lifetime seems to be
managed on the ScriptExecutionContext thread (but the class is used on another
thread).


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 4f69af6..4d0ec7e 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,42 @@
> +2010-08-13  Jian Li	<jianli at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Change FileStream implementation to prepare for blob resource
handling.
> +	   https://bugs.webkit.org/show_bug.cgi?id=43985
> +
> +	   Enhance FileStream implementation to support both synchronous and
> +	   asynchronous usages. This is needed in preparation to use FileStream

> +	   in upcoming blob resource handling change. Update the existing
FileReader
> +	   code to adapt to this change. 
> +
> +	   * html/FileReader.cpp:
> +	   (WebCore::FileReader::didStart): 

Account for refactoring of the openForRead method, by now getting the size
which the openForRead method used to do. ?


> +	   (WebCore::FileReader::didGetSize):
> +	   (WebCore::FileReader::didOpen):
> +	   (WebCore::FileReader::didRead):
> +	   * html/FileReader.h:
> +	   * html/FileStream.cpp:

Allow m_client to be 0 when not on the main thread to enable ???

> +	   (WebCore::FileStream::start):
> +	   (WebCore::FileStream::stop):
> +	   (WebCore::FileStream::getSize):
> +	   (WebCore::FileStream::openForRead):
> +	   (WebCore::FileStream::openForWrite):
> +	   (WebCore::FileStream::close):
> +	   (WebCore::FileStream::read):
> +	   (WebCore::FileStream::write):
> +	   (WebCore::FileStream::truncate):
> +	   * html/FileStream.h:
> +	   * html/FileStreamClient.h:
> +	   (WebCore::FileStreamClient::didOpen):
Added to allow ???

> +	   * html/FileStreamProxy.cpp:
Connect the new methods across threads.

> +	   (WebCore::FileStreamProxy::getSize):
> +	   (WebCore::FileStreamProxy::openForRead):
> +	   (WebCore::notifyOpenOnContext):
> +	   (WebCore::FileStreamProxy::didOpen):

> +	   * html/FileStreamProxy.h:
Account for FileStreamClient changes and added getSize/openForRead methods.

> +	   * html/FileThreadTask.h:

Added return value for to allow calling FileStream::openForRead.

Some per item comments would be useful in understanding the patch.....I'll be
adding my own as I go through it.


> diff --git a/WebCore/html/FileReader.cpp b/WebCore/html/FileReader.cpp

> +    ASSERT(m_fileBlob->items().size() == 1 &&
m_fileBlob->items().at(0)->toFileBlobItem());
> +    const FileRangeBlobItem* fileRangeItem =
m_fileBlob->items().at(0)->toFileRangeBlobItem();
> +    long long start = fileRangeItem ? fileRangeItem->start() : 0;
> +
> +    m_totalBytes = fileRangeItem ? fileRangeItem->size() : size;

When does fileRangeItem->size() != size ?




> diff --git a/WebCore/html/FileStream.cpp b/WebCore/html/FileStream.cpp

Why not have a AsyncFileStream that has a client and a FileStream that doesn't?
It looks like AsyncFileStream could enforce that m_client is not 0 and just a a
thin layer on top of FileStream.


> +long long FileStream::getSize(const String& path, double
expectedModificationTime)
> +
> +    // Check the modificationt time for the possible file change.

typo: modificationt


> +ExceptionCode FileStream::openForWrite(const String&)
>  {
> -    ASSERT(!isMainThread());
> +    ASSERT(!m_client || !isMainThread());
>      // FIXME: to be implemented.
> +    return 0;

Seems like it should return a failure code.


> +int FileStream::write(Blob*, long long, int)
>  {
> +    ASSERT(!m_client || !isMainThread());
>      // FIXME: to be implemented.
> +    return 0;

Seems like it should return a failure code.

> +ExceptionCode FileStream::truncate(long long)
>  {
> +    ASSERT(!m_client || !isMainThread());
>      // FIXME: to be implemented.
> +    return 0;

Seems like it should return a failure code.



WebCore/html/FileStream.h:62
 +	// Gets the size of a file. Also validates if the file has been changed
or not if the expected modification time is provided.
"if the expected modification time is not 0."

(It looks like it always must be provided since it is just a double being
passed in.)