Bug 164056 – BufferSource should behave as an union

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

BufferSource should behave as an union

[Zan Dobersek]

Created attachment 293006 [details]
Patch

[Zan Dobersek]

This was 'inspired' by an equivalent typedef and the underlying code for BufferDataSource in WebGLRenderingContextBase.

[youenn fablet]

Comment on attachment 293006 [details]
Patch

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

> Source/WebCore/bindings/js/BufferSource.h:51
> +        brigand::for_each<Sequence>([&](auto&& type) {

First time I encounter brigand for_each.
What is the advantage compared to directly using holds_alternative?

> Source/WebCore/bindings/js/BufferSource.h:81
> +    WTF::Variant<RefPtr<JSC::ArrayBufferView>, RefPtr<JSC::ArrayBuffer>> m_variant;

There is a change of behavior here: BufferSource is refing the underlying array.
This ensures the data is not collected as long as BufferSource is there.
But I guess this does some additional count churning.

Are there use-cases where this refing is useful?

[Zan Dobersek]

Comment on attachment 293006 [details]
Patch

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

>> Source/WebCore/bindings/js/BufferSource.h:51
>> +        brigand::for_each<Sequence>([&](auto&& type) {
> 
> First time I encounter brigand for_each.
> What is the advantage compared to directly using holds_alternative?

It's used in JSConverter::convert() when converting WTF::Variant<> to JSValue. holds_alternative would probably work better here.

>> Source/WebCore/bindings/js/BufferSource.h:81
>> +    WTF::Variant<RefPtr<JSC::ArrayBufferView>, RefPtr<JSC::ArrayBuffer>> m_variant;
> 
> There is a change of behavior here: BufferSource is refing the underlying array.
> This ensures the data is not collected as long as BufferSource is there.
> But I guess this does some additional count churning.
> 
> Are there use-cases where this refing is useful?

This mirrors the behavior of BufferDataSource in WebGLRenderingContextBase, but I understand the concern.

Regarding the use-case -- I hit the limit of the previous implementation when trying to get the iterable<BufferSource, enum MediaKeyStatus> property on the EME's MediaKeyStatusMap interface to work. That would mean converting BufferSource objects (created by the implementation) from the native types to JSValues. I suppose the simplest way to support this use-case would be to simply offload the RefPtr<ArrayBuffer> object management straight into the BufferSource class.
http://w3c.github.io/encrypted-media/#mediakeystatusmap-interface
http://w3c.github.io/encrypted-media/#update-key-statuses

But in terms of current behavior, IDLUnion ends up using RefPtr<Interface> as the implementation type for interfaces that are specified as variant alternatives, so IDLInterface or IDLType would need ArrayBuffer-specific specializations to work around that and enable using Variant<RefPtr<ArrayBufferView>, ArrayBuffer*> (which at the moment doesn't compile).

[Chris Dumez]

Comment on attachment 293006 [details]
Patch

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

> Source/WebCore/bindings/generic/IDLTypes.h:141
> +using IDLBufferSource = IDLUnion<IDLInterface<JSC::ArrayBufferView>, IDLInterface<JSC::ArrayBuffer>>;

Why do we need an IDLBufferSource type at all? If it is a union, then it would be a typedef to a union in the IDL where this is used and we should not need an IDLBufferSource type on native side.

[Chris Dumez]

Comment on attachment 293006 [details]
Patch

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

>> Source/WebCore/bindings/generic/IDLTypes.h:141
>> +using IDLBufferSource = IDLUnion<IDLInterface<JSC::ArrayBufferView>, IDLInterface<JSC::ArrayBuffer>>;
> 
> Why do we need an IDLBufferSource type at all? If it is a union, then it would be a typedef to a union in the IDL where this is used and we should not need an IDLBufferSource type on native side.

Oh, I see now that BufferSource is actually part of the WebIDL spec:
https://heycam.github.io/webidl/#BufferSource

I guess this is acceptable then.

[Chris Dumez]

Comment on attachment 293006 [details]
Patch

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

>>> Source/WebCore/bindings/js/BufferSource.h:51
>>> +        brigand::for_each<Sequence>([&](auto&& type) {
>> 
>> First time I encounter brigand for_each.
>> What is the advantage compared to directly using holds_alternative?
> 
> It's used in JSConverter::convert() when converting WTF::Variant<> to JSValue. holds_alternative would probably work better here.

Would something like this work?

return WTF::visit([](auto& buffer) -> const uint8_t {
    return buffer.data();
}, variant);

[Chris Dumez]

Comment on attachment 293006 [details]
Patch

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

> Source/WebCore/bindings/js/BufferSource.h:68
> +        brigand::for_each<Sequence>([&](auto&& type) {

Ditto here:
return WTF::visit([](auto& buffer) -> const uint8_t {
    return buffer.byteLength();
}, m_variant);

[Chris Dumez]

Comment on attachment 293006 [details]
Patch

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

> Source/WebCore/bindings/js/BufferSource.h:40
>  class BufferSource {

Personally, I think we don't need this class at all, it should just be:
using BufferSource = WTF::Variant<RefPtr<JSC::ArrayBufferView>, RefPtr<JSC::ArrayBuffer>>;

>>> Source/WebCore/bindings/js/BufferSource.h:81
>>> +    WTF::Variant<RefPtr<JSC::ArrayBufferView>, RefPtr<JSC::ArrayBuffer>> m_variant;
>> 
>> There is a change of behavior here: BufferSource is refing the underlying array.
>> This ensures the data is not collected as long as BufferSource is there.
>> But I guess this does some additional count churning.
>> 
>> Are there use-cases where this refing is useful?
> 
> This mirrors the behavior of BufferDataSource in WebGLRenderingContextBase, but I understand the concern.
> 
> Regarding the use-case -- I hit the limit of the previous implementation when trying to get the iterable<BufferSource, enum MediaKeyStatus> property on the EME's MediaKeyStatusMap interface to work. That would mean converting BufferSource objects (created by the implementation) from the native types to JSValues. I suppose the simplest way to support this use-case would be to simply offload the RefPtr<ArrayBuffer> object management straight into the BufferSource class.
> http://w3c.github.io/encrypted-media/#mediakeystatusmap-interface
> http://w3c.github.io/encrypted-media/#update-key-statuses
> 
> But in terms of current behavior, IDLUnion ends up using RefPtr<Interface> as the implementation type for interfaces that are specified as variant alternatives, so IDLInterface or IDLType would need ArrayBuffer-specific specializations to work around that and enable using Variant<RefPtr<ArrayBufferView>, ArrayBuffer*> (which at the moment doesn't compile).

Yes, we currently translate unions of wrapper types into a WTF::Variant of RefPtrs. I think this is fine.

[youenn fablet]

> Personally, I think we don't need this class at all, it should just be:
> using BufferSource = WTF::Variant<RefPtr<JSC::ArrayBufferView>,
> RefPtr<JSC::ArrayBuffer>>;

Helper functions that hide Variant are generally more readable, I did that for FetchBody

In that particular case, we search for the same thing (a pointer and a length), so this seems useful to me.

[Chris Dumez]

(In reply to comment #10)
> > Personally, I think we don't need this class at all, it should just be:
> > using BufferSource = WTF::Variant<RefPtr<JSC::ArrayBufferView>,
> > RefPtr<JSC::ArrayBuffer>>;
> 
> Helper functions that hide Variant are generally more readable, I did that
> for FetchBody
> 
> In that particular case, we search for the same thing (a pointer and a
> length), so this seems useful to me.

In some cases, I would agree. In this particular instance, the code the access the data and length is 3 lines using WTF::visit() and a lambda (as I commented earlier). I think it is simple enough that we don't need the extra abstraction. That said, I don't feel that strongly.

[Zan Dobersek]

(In reply to comment #11)
> (In reply to comment #10)
> > > Personally, I think we don't need this class at all, it should just be:
> > > using BufferSource = WTF::Variant<RefPtr<JSC::ArrayBufferView>,
> > > RefPtr<JSC::ArrayBuffer>>;
> > 
> > Helper functions that hide Variant are generally more readable, I did that
> > for FetchBody
> > 
> > In that particular case, we search for the same thing (a pointer and a
> > length), so this seems useful to me.
> 
> In some cases, I would agree. In this particular instance, the code the
> access the data and length is 3 lines using WTF::visit() and a lambda (as I
> commented earlier). I think it is simple enough that we don't need the extra
> abstraction. That said, I don't feel that strongly.

I understand this reasoning, and I like to keep things simple, but these WTF::visit() calls to retrieve data pointer and size will spread as BufferSource gets used in more places, and they will likely differ in style from file to file.

I think a minimal abstraction over this specific variant that's easy for the compiler to inline wouldn't do harm. OTOH the variant object will have to be exposed through a getter when converting BufferSource to a JS value.

So overall I as well don't have strong preference here, but would personally prefer the abstraction.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > Personally, I think we don't need this class at all, it should just be:
> > > > using BufferSource = WTF::Variant<RefPtr<JSC::ArrayBufferView>,
> > > > RefPtr<JSC::ArrayBuffer>>;
> > > 
> > > Helper functions that hide Variant are generally more readable, I did that
> > > for FetchBody
> > > 
> > > In that particular case, we search for the same thing (a pointer and a
> > > length), so this seems useful to me.
> > 
> > In some cases, I would agree. In this particular instance, the code the
> > access the data and length is 3 lines using WTF::visit() and a lambda (as I
> > commented earlier). I think it is simple enough that we don't need the extra
> > abstraction. That said, I don't feel that strongly.
> 
> I understand this reasoning, and I like to keep things simple, but these
> WTF::visit() calls to retrieve data pointer and size will spread as
> BufferSource gets used in more places, and they will likely differ in style
> from file to file.
> 
> I think a minimal abstraction over this specific variant that's easy for the
> compiler to inline wouldn't do harm. OTOH the variant object will have to be
> exposed through a getter when converting BufferSource to a JS value.
> 
> So overall I as well don't have strong preference here, but would personally
> prefer the abstraction.

ok

[Zan Dobersek]

Created attachment 293031 [details]
Patch

Keeps the abstraction-natured BufferSource class, uses WTF::visit().

[Chris Dumez]

Comment on attachment 293031 [details]
Patch

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

seems fine, r=me with a few nits.

> Source/WebCore/bindings/js/BufferSource.h:30
> +#include <wtf/Brigand.h>

I don't think we need this anymore.

> Source/WebCore/bindings/js/BufferSource.h:31
> +#include <wtf/Optional.h>

Is  this really needed?

> Source/WebCore/bindings/js/BufferSource.h:36
> +class ArrayBuffer;

You include them at the top some I doubt we need those forward declarations.

[Zan Dobersek]

Comment on attachment 293031 [details]
Patch

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

>> Source/WebCore/bindings/js/BufferSource.h:36
>> +class ArrayBuffer;
> 
> You include them at the top some I doubt we need those forward declarations.

My bad, forgot to remove these and the includes.

Thanks for the review.

[Zan Dobersek]

Created attachment 293033 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293033 [details]
Patch for landing

Rejecting attachment 293033 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 293033, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 207995 = e143a49787bbfc043c52fa0f6c63e1152d061637
r207998 = d04bf7db981cc8f6c353fd1d44c1a2d6ba616c18
r207999 = d444a846405f03e2808822bbf70064b677448721
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/2387929

[WebKit Commit Bot]

Comment on attachment 293033 [details]
Patch for landing

Clearing flags on attachment: 293033

Committed r208002: <http://trac.webkit.org/changeset/208002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.