Bug 110556 – Update FileReaderLoader to allow specifying a range and reading as a blob.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zachary Kuznia]

Update FileReaderLoader to allow specifying a range and reading as a blob.

[Zachary Kuznia]

Created attachment 189681 [details]
Patch

[Zachary Kuznia]

This is to allow support of the Stream API.  The complete version is at:
https://bugs.webkit.org/show_bug.cgi?id=110194

[Hajime Morrita]

Comment on attachment 189681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189681&action=review

> Source/WebCore/ChangeLog:3
> +        Update FileReaderLoader to allow specifying a range and reading as a blob.

If this is a part of big change, it is worth mentioning here.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Say something about this or just remove this line. The repo reject this patch otherwise.

> Source/WebCore/fileapi/FileReaderLoader.cpp:57
> +const int kDefaultBufferLength = 32768;

WebKit doesn't prefix "k" for constants.
Usually it names each global variable like a local variable, but with more verbose name.

> Source/WebCore/fileapi/FileReaderLoader.cpp:95
> +        request.setHTTPHeaderField("Range", String("bytes=") + String::number(m_rangeStart) + ("-") + String::number(m_rangeEnd));

There is printf-like method called String::format() which you could use. See Source/WTF/wtf/text/WTFString.h.

> Source/WebCore/fileapi/FileReaderLoader.cpp:147
>      if (length > numeric_limits<unsigned>::max()) {

Does this means that we're going to allocate numeric_limits<unsigned>::max() bytes of the array before switching to variable-length approach?

> Source/WebCore/fileapi/FileReaderLoader.cpp:285
> +}

Looks like wre're going to create new Blob instance and copy the data into it  for each blobResult() call, which seems wasteful.
Is this intentional?

[Zachary Kuznia]

Created attachment 189690 [details]
Patch

[Zachary Kuznia]

(In reply to comment #3)
> (From update of attachment 189681 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189681&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Update FileReaderLoader to allow specifying a range and reading as a blob.
> 
> If this is a part of big change, it is worth mentioning here.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> Say something about this or just remove this line. The repo reject this patch otherwise.

Done.

> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:57
> > +const int kDefaultBufferLength = 32768;
> 
> WebKit doesn't prefix "k" for constants.
> Usually it names each global variable like a local variable, but with more verbose name.

Changed to defaultBufferLength

> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:95
> > +        request.setHTTPHeaderField("Range", String("bytes=") + String::number(m_rangeStart) + ("-") + String::number(m_rangeEnd));
> 
> There is printf-like method called String::format() which you could use. See Source/WTF/wtf/text/WTFString.h.

Done.

> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:147
> >      if (length > numeric_limits<unsigned>::max()) {
> 
> Does this means that we're going to allocate numeric_limits<unsigned>::max() bytes of the array before switching to variable-length approach?

No.  The variable length method is used if a content length header isn't received.  The internal buffer can't be larger than numeric_limits<unsigned>::max(), and so the read fails if that is exceeded.

> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:285
> > +}
> 
> Looks like wre're going to create new Blob instance and copy the data into it  for each blobResult() call, which seems wasteful.
> Is this intentional?

Changed it to cache the value.

[Kinuko Yasuda]

Comment on attachment 189690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189690&action=review

> Source/WebCore/ChangeLog:7
> +        https://bugs.webkit.org/show_bug.cgi?id=110194

It's better to explicitly say you'll add tests.

> Source/WebCore/fileapi/FileReaderLoader.cpp:-141
> -        return;

(As we chatted offline) when length > unsigned max we'not returning here but may fail at line 183. This feels weird without the comment why we're triggering 'variableLength' mode. If we have some comments and/or compare the length to the special value when we should trigger 'variable' mode that'd be more understandable.

[Zachary Kuznia]

Created attachment 189695 [details]
Patch

[Zachary Kuznia]

(In reply to comment #6)
> (From update of attachment 189690 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189690&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        https://bugs.webkit.org/show_bug.cgi?id=110194
> 
> It's better to explicitly say you'll add tests.

Done.

> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:-141
> > -        return;
> 
> (As we chatted offline) when length > unsigned max we'not returning here but may fail at line 183. This feels weird without the comment why we're triggering 'variableLength' mode. If we have some comments and/or compare the length to the special value when we should trigger 'variable' mode that'd be more understandable.

Done.

[Kinuko Yasuda]

Comment on attachment 189695 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189695&action=review

> Source/WebCore/fileapi/FileReaderLoader.cpp:189
> +        if (m_variableLength >= numeric_limits<unsigned>::max()) {

This looks wrong?

[Zachary Kuznia]

Created attachment 189697 [details]
Patch

[Zachary Kuznia]

(In reply to comment #9)
> (From update of attachment 189695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189695&action=review
> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:189
> > +        if (m_variableLength >= numeric_limits<unsigned>::max()) {
> 
> This looks wrong?

Fixed.

[Kinuko Yasuda]

Comment on attachment 189697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189697&action=review

> Source/WebCore/fileapi/FileReaderLoader.cpp:224
> +        memcpy(static_cast<char*>(newData->data()), static_cast<char*>(m_rawData->data()), m_bytesLoaded);

Can we use m_rawData->slice() instead to create newData?

> Source/WebCore/fileapi/FileReaderLoader.cpp:293
> +        blobData->appendData(rawData, 0, BlobDataItem::toEndOfFile);

toEndOfFile -> size? (Or is it intentional?)

[Zachary Kuznia]

Created attachment 189702 [details]
Patch

[Zachary Kuznia]

(In reply to comment #12)
> (From update of attachment 189697 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189697&action=review
> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:224
> > +        memcpy(static_cast<char*>(newData->data()), static_cast<char*>(m_rawData->data()), m_bytesLoaded);

Done.

> 
> Can we use m_rawData->slice() instead to create newData?
> 
> > Source/WebCore/fileapi/FileReaderLoader.cpp:293
> > +        blobData->appendData(rawData, 0, BlobDataItem::toEndOfFile);
> 
> toEndOfFile -> size? (Or is it intentional?)

Done.

[Kinuko Yasuda]

Comment on attachment 189702 [details]
Patch

This looks good/reasonable to me.

[Hajime Morrita]

Comment on attachment 189702 [details]
Patch

Let me stamp this then.

[WebKit Review Bot]

Comment on attachment 189702 [details]
Patch

Rejecting attachment 189702 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Hajime Morrita]

Comment on attachment 189702 [details]
Patch

Oops sorry for that. I should've set the cq+.

[WebKit Review Bot]

Comment on attachment 189702 [details]
Patch

Clearing flags on attachment: 189702

Committed r143706: <http://trac.webkit.org/changeset/143706>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

(In reply to comment #20)
> All reviewed patches have been landed.  Closing bug.

I landed a 32 bit build fix in http://trac.webkit.org/changeset/143733.

[Alexey Proskuryakov]

> This is to allow support of the Stream API.

This adds a ton of code. Why is it not behind an ENABLE ifdef?

[Hajime Morrita]

(In reply to comment #22)
> > This is to allow support of the Stream API.
> 

> This adds a ton of code. Why is it not behind an ENABLE ifdef?
Good point. Which flag should we use though.
Is this specific for the stream API? Or is this applicable for non-Stream use?

[Zachary Kuznia]

(In reply to comment #23)
> (In reply to comment #22)
> > > This is to allow support of the Stream API.
> > 
> 
> > This adds a ton of code. Why is it not behind an ENABLE ifdef?
> Good point. Which flag should we use though.
> Is this specific for the stream API? Or is this applicable for non-Stream use?

I'm putting the Stream code behind the ENABLE_STREAM flag.  I deferred the changes to this class that are Stream specific, and will have them protected by the STREAM flag.

Would you like me to wrap setRange() and blobResult() with the flag as well?

[Hajime Morrita]

> I'm putting the Stream code behind the ENABLE_STREAM flag.  I deferred the changes to this class that are Stream specific, and will have them protected by the STREAM flag.
> 
> Would you like me to wrap setRange() and blobResult() with the flag as well?

Let's do so. It will clarify that the API is used from the Stream implementation.