Implement Blob.text and Blob.arrayBuffer

• Component: DOM
• Status: RESOLVED DUPLICATE

[Tetsuharu Ohzeki [UTC+9]]

This is a part of bug 196258.
`Blob.stream` needs ReadableStream but it's switched the compile flag `if ENABLE(STREAMS_API)`.

So First, I implements Blob.text and Blob.arrayBuffer which we can implement without ReadableStream.

https://w3c.github.io/FileAPI/#text-method-algo
https://w3c.github.io/FileAPI/#arraybuffer-method-algo

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 401853 [details]
WIP

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 402103 [details]
Patch

[Sam Weinig]

Thanks for tackling this Tetsuharu!

As far as I can tell, ENABLE_STREAMS_API is enabled by default (at least for macOS and iOS, I always have a bit of trouble remembering where to look for other ports).

Given that, does it still make sense to use this implementation strategy? Or would it make more sense to try build these two methods on top of a general ReadableStream based implementation? (It may very well make sense to use this strategy, I am not professing support one way or another, simply asking).

[Tetsuharu Ohzeki [UTC+9]]

(In reply to Sam Weinig from comment #3)
> Thanks for tackling this Tetsuharu!
> 
> As far as I can tell, ENABLE_STREAMS_API is enabled by default (at least for
> macOS and iOS, I always have a bit of trouble remembering where to look for
> other ports).
> 
> Given that, does it still make sense to use this implementation strategy? Or
> would it make more sense to try build these two methods on top of a general
> ReadableStream based implementation? (It may very well make sense to use
> this strategy, I am not professing support one way or another, simply
> asking).

To be honest, I thought (planned) that we (probably I) should refactor these implementation after implementing `Blob.stream`. I'm not sure about that we can build these methods on the top of ReadableStream and its way is simple to implement them and I need to investigate about buiding on general ReadableStream to answer to your question.


---------------


By the way, I seem all(?) ports has enabled ENABLE_STREAMS_API by default.
Cannot we remove this flag? Should I post about it to webkit-dev?


- http://trac.webkit.org/browser/trunk/Source/WTF/wtf/PlatformEnable.h#L465
- http://trac.webkit.org/browser/trunk/Source/cmake/OptionsFTW.cmake#L104
- http://trac.webkit.org/browser/trunk/Source/cmake/OptionsWin.cmake#L58
- http://trac.webkit.org/browser/trunk/Source/cmake/OptionsMac.cmake#L87
- http://trac.webkit.org/browser/trunk/Source/cmake/WebKitFeatures.cmake#L210

[Sam Weinig]

(In reply to Tetsuharu Ohzeki from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Thanks for tackling this Tetsuharu!
> > 
> > As far as I can tell, ENABLE_STREAMS_API is enabled by default (at least for
> > macOS and iOS, I always have a bit of trouble remembering where to look for
> > other ports).
> > 
> > Given that, does it still make sense to use this implementation strategy? Or
> > would it make more sense to try build these two methods on top of a general
> > ReadableStream based implementation? (It may very well make sense to use
> > this strategy, I am not professing support one way or another, simply
> > asking).
> 
> To be honest, I thought (planned) that we (probably I) should refactor these
> implementation after implementing `Blob.stream`. I'm not sure about that we
> can build these methods on the top of ReadableStream and its way is simple
> to implement them and I need to investigate about buiding on general
> ReadableStream to answer to your question.
> 
> 
> ---------------
> 
> 
> By the way, I seem all(?) ports has enabled ENABLE_STREAMS_API by default.
> Cannot we remove this flag? Should I post about it to webkit-dev?

Couldn't hurt to email webkit-dev.

[youenn fablet]

Comment on attachment 402103 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        without `ReadableStream` implementation.

Since the goal is to go with supporting ReadableStream at some point, we should probably extract existing functionality from FetchBodyOwner/FetchBoydConsumer.
Ideally, split FetchBodyOwner in two classes, one that will continue be the base class from FetchRequest and FetchResponse, plus be an ActiveDOMObject.
And the other one which can be reused jointly with FetchBodyConsumer to implement text/arrayBuffer/ReadableStream.

> Source/WebCore/fileapi/BlobPromiseLoader.cpp:65
> +    delete this;

This is a risky pattern.
I do not think we need BlobPromiseLoader to be an ActiveDOMObject.
It might be best to make Blob own the BlobPromiseLoader.

[Tetsuharu Ohzeki [UTC+9]]

(In reply to youenn fablet from comment #6)
> This is a risky pattern.
> I do not think we need BlobPromiseLoader to be an ActiveDOMObject.
> It might be best to make Blob own the BlobPromiseLoader.

text/arrayBuffer/ReadableStream is marked as `[NewObject]` in IDL.

If we make Blob owns the BlobPromiseLoader, we need to hold multiple `DeferredPromise` or `BlobPromiseLoader` in Blob by vector or map, right?

[youenn fablet]

> text/arrayBuffer/ReadableStream is marked as `[NewObject]` in IDL.

For text and arrayBuffer, this is the promise that is new, and this is already handled by the binding generator.

Given the algorithm though, you are correct that text and arrayBuffer will be also brand new object. For ReadableStream, a new one needs to be created indeed.

> If we make Blob owns the BlobPromiseLoader, we need to hold multiple
> `DeferredPromise` or `BlobPromiseLoader` in Blob by vector or map, right?


Right, this could be a HashSet.

Thinking a bit more, BlobLoader in BlobLoader.h might be the one you actually want to use.
It could be easily extended to support String, and could be probably extended to ReadableStream as well.

[Tetsuharu Ohzeki [UTC+9]]

*** This bug has been marked as a duplicate of bug 215663 ***