[webkit-reviews] review denied: [Bug 32993] Blob.slice support : [Attachment 48447] Proposed Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 9 20:45:33 PST 2010


Dmitry Titov <dimich at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 32993: Blob.slice support
https://bugs.webkit.org/show_bug.cgi?id=32993

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
> diff --git
a/LayoutTests/http/tests/local/resources/send-sliced-dragged-file.js
b/LayoutTests/http/tests/local/resources/send-sliced-dragged-file.js
> +function onUnstableFileDrop(file)
> +
> +    // The file modification time is second-based. So delay changing the
file in order to get the new modification time for the file.
> +    setTimeout("onModifyFile()", 500);

Ouch. Maybe better to say "The file modification time has resolution of one
second.", or something like that, to explain such a delay...

> --- a/WebCore/ChangeLog

> +	   1) File.slice() does a synchronous IO to captures the current size
and

"to capture"

> diff --git a/WebCore/html/Blob.cpp b/WebCore/html/Blob.cpp
> index 0b677ea..acf6142 100644
> --- a/WebCore/html/Blob.cpp
> +++ b/WebCore/html/Blob.cpp
> @@ -37,17 +37,72 @@ namespace WebCore {
>  
>  Blob::Blob(const String& path)
>      : m_path(path)
> +    , m_start(0)
> +    , m_length(-1)
> +    , m_snapshotSize(-1)
> +    , m_snapshotModificationTime(0)

These 4 variables seems all have 'special values' like 0 or -1 and this creates
a matrix of possible behaviors. It's better to have a specific state expressed
as bool variable ("m_isPathOnly" or "m_snapshotCaptured") and have Asserts in
accessor functions so that nobody could consume things like length() if the
length is not actually valid. At least we should be able to enumerate and
document all possible combinations and have asserts to verify that we are not
getting out of valid space. For example, is it possible to have start(100),
length(-1)? Probably not.

>  unsigned long long Blob::size() const
>  {
> +    return static_cast<unsigned long long>((m_snapshotSize == -1) ?
getFileSizeHelper() : m_length); 

This doesn't seem to need a cast.

> +long long Blob::getFileSizeHelper() const
> +{
>      // FIXME: JavaScript cannot represent sizes as large as unsigned long
long, we need to
>      // come up with an exception to throw if file size is not represetable.
>      long long size;
>      if (!getFileSize(m_path, size))
>	   return 0;

This 0 means the file does not exist or is unreadable. It is not the same as
having a file of size 0. Should the code differentiate?

> +time_t Blob::getFileModificationTimeHelper() const
> +{
> +    time_t modificationTime;
> +    if (!getFileModificationTime(m_path, modificationTime))
> +	   return 0;

Same thing here - the file likely does not exist (and may appear before the
page will attempt to send it, which is a valid scenario).

> diff --git a/WebCore/platform/network/FormData.h
b/WebCore/platform/network/FormData.h
> -    FormDataElement(const String& filename, bool shouldGenerateFile) :
m_type(encodedFile), m_filename(filename),
m_shouldGenerateFile(shouldGenerateFile) { }
> +    FormDataElement(const String& filename, int64_t fileStart, int64_t
fileLength, time_t fileModificationTime, bool shouldGenerateFile) :
m_type(encodedFile), m_filename(filename), m_fileStart(fileStart),
m_fileLength(fileLength), m_fileModificationTime(fileModificationTime),
m_shouldGenerateFile(shouldGenerateFile) { }

shouldn't use int64. long long is better in WebCore.

> +    int64_t m_fileStart;
> +    int64_t m_fileLength;

Ditto.

> +static bool advanceCurrentStream(FormStreamFields *form)

> +	   if (nextInput.m_fileModificationTime) {
> +	       time_t modificationTime;
> +	       if (getFileModificationTime(nextInput.m_filename,
modificationTime) && modificationTime != nextInput.m_fileModificationTime)
> +		   return false;
> +	   }

I think this check should be more complex. For example, we might have scenario
when file did not exist before and now it does, and Blob is still 'floating'
(no captured snapshot).
Also, getFileModificationTime() may return false if there is no file and we
might want to fail operation (if we had captured snapshot before).

>	   form->currentStream = CFReadStreamCreateWithFile(0, fileURL.get());
> +	   if (nextInput.m_fileStart > 0) {
> +	       CFNumberRef position = CFNumberCreate(0, kCFNumberLongLongType,
&nextInput.m_fileStart);
> +	       CFReadStreamSetProperty(form->currentStream,
kCFStreamPropertyFileCurrentOffset, position);

This code will probably need to be looked at by someone who udnerstands CF
better...

Because of int64 and need to clean up the code around 4 variales with 'special
values' I mark it r-.
But, very good progress!


More information about the webkit-reviews mailing list