Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specifying QueuingStrategies in WebIDL #1005

Closed
ricea opened this issue Jul 9, 2019 · 14 comments · Fixed by #1035
Closed

Specifying QueuingStrategies in WebIDL #1005

ricea opened this issue Jul 9, 2019 · 14 comments · Fixed by #1035

Comments

@ricea
Copy link
Collaborator

ricea commented Jul 9, 2019

As part of #963, we will have to specify CountQueuingStrategy and ByteLengthQueuingStrategy in WebIDL. There are a couple of things about these classes that are "strange" from a WebIDL point-of-view:

  1. size() is a function (as opposed to a method), but it is inherited
  2. highWaterMark is added by the constructor as a property of the object, rather than coming from the prototype.

I think problem 2. can be fixed quite easily by making highWaterMark a normal interface attribute.

Problem 1. is harder. WebIDL doesn't seem to have functions as such at all. The closest thing is "Callback functions", but it doesn't appear that they can exist as attributes on an interface. There are also "static operations", which are a bit like functions, but appear as properties of the constructor, not the prototype.

@domenic
Copy link
Member

domenic commented Jul 9, 2019

I think it's OK to make size() a normal method, although there is some risk of back-compat problems if people are using it in strange ways.

If we think that is not likely to work, we have a few workarounds:

  • Use any and return a function object.
  • Introduce a new Web IDL extended attribute for methods that removes the brand check.

As always, I apologize for this mess.

@MattiasBuelens
Copy link
Collaborator

Would this mean that CountQueuingStrategy.prototype.size() must always be called with a proper CountQueuingStrategy object as this context? Correct me if I'm wrong, but it seems like the WebIDL spec requires a brand check on all operations:

  1. If target is an interface, and op is not a static operation:
    2.1. Let esValue be the this value, if it is not null or undefined, or realm’s global object otherwise. (This will subsequently cause a TypeError in a few steps, if the global object does not implement target and [LenientThis] is not specified.)
    2.2. If esValue is a platform object, then perform a security check, passing esValue, id, and "method".
    2.3. If esValue does not implement the interface target, throw a TypeError.

This would be a problem in the current streams specification, since we always call the size algorithm as a plain function with no this context.

@ricea
Copy link
Collaborator Author

ricea commented Jul 10, 2019

I don't want to give up on calling size as a plain function. The principle that it is a pure function is important to the mental model of streams. Making it a method gives the implication that it can be stateful.

The brand check is necessary for the correct functioning of WebIDL-generated code, at least in Blink. Before it can call a C++ method, it needs to make sure it's calling it on an object of the correct type.

The way I'm currently doing it in my in-progress implementation in Blink is this:

[
    Exposed=(Window,Worker,Worklet),
    Constructor(QueuingStrategyInit init)
] interface CountQueuingStrategy {
    readonly attribute any highWaterMark;

    // size is an accessor that returns a function.
    readonly attribute any size;
};

This still has slightly odd semantics:

const size = new CountQueuingStrategy({}).size;
console.assert(size() == 1);

works as expected, but

const size = CountQueuingStrategy.prototype.size;

throws an exception.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 16, 2019
Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679897}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679897}
@domenic
Copy link
Member

domenic commented Jul 23, 2019

For what it's worth, I would prefer to introduce a new Web IDL extended attribute for this, as it's pretty weird to have a getter that returns a function, instead of just having a function directly.

@ricea
Copy link
Collaborator Author

ricea commented Jul 24, 2019

For what it's worth, I would prefer to introduce a new Web IDL extended attribute for this, as it's pretty weird to have a getter that returns a function, instead of just having a function directly.

Yes, an extended attribute would make sense. In C++, the behaviour corresponds to a static method, but in the WebIDL world it's more like a normal method that doesn't take a this value. So perhaps if you annotate an ordinary method like [NoThis] then it would mostly appear like a normal method from JavaScript, but the bindings code would translate it to a call to a static method in C++.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 31, 2019
…stonly

Automatic update from web-platform-tests
Implement QueuingStrategies in C++

Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679897}

--

wpt-commits: 4dbc86fa9b79da17a037140fc2eaba05c80f90ee
wpt-pr: 17804
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jul 31, 2019
…stonly

Automatic update from web-platform-tests
Implement QueuingStrategies in C++

Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679897}

--

wpt-commits: 4dbc86fa9b79da17a037140fc2eaba05c80f90ee
wpt-pr: 17804
natechapin pushed a commit to natechapin/wpt that referenced this issue Aug 23, 2019
Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679897}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…stonly

Automatic update from web-platform-tests
Implement QueuingStrategies in C++

Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhiranochromium.org>
Commit-Queue: Adam Rice <riceachromium.org>
Cr-Commit-Position: refs/heads/master{#679897}

--

wpt-commits: 4dbc86fa9b79da17a037140fc2eaba05c80f90ee
wpt-pr: 17804

UltraBlame original commit: b4794d593bcd7a512c4ffd29566d54ae268df898
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…stonly

Automatic update from web-platform-tests
Implement QueuingStrategies in C++

Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhiranochromium.org>
Commit-Queue: Adam Rice <riceachromium.org>
Cr-Commit-Position: refs/heads/master{#679897}

--

wpt-commits: 4dbc86fa9b79da17a037140fc2eaba05c80f90ee
wpt-pr: 17804

UltraBlame original commit: b4794d593bcd7a512c4ffd29566d54ae268df898
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…stonly

Automatic update from web-platform-tests
Implement QueuingStrategies in C++

Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhiranochromium.org>
Commit-Queue: Adam Rice <riceachromium.org>
Cr-Commit-Position: refs/heads/master{#679897}

--

wpt-commits: 4dbc86fa9b79da17a037140fc2eaba05c80f90ee
wpt-pr: 17804

UltraBlame original commit: b4794d593bcd7a512c4ffd29566d54ae268df898
@domenic
Copy link
Member

domenic commented Nov 3, 2019

Can we get use counters on CountQueuingStrategy and ByteQueuingStrategy? If they are not used almost at all, we could consider more drastic design changes or removals. I doubt that's the case, but it might be, and I think we should do due dilligence.

@domenic
Copy link
Member

domenic commented Nov 3, 2019

I've posted whatwg/webidl#819 to ask the Web IDL community if they have any particular guidance here. /cc @jswalden who seems to be poking around streams stuff a bit, and especially as a JS person, might have helpful opinions.

@ricea
Copy link
Collaborator Author

ricea commented Nov 5, 2019

Can we get use counters on CountQueuingStrategy and ByteQueuingStrategy? If they are not used almost at all, we could consider more drastic design changes or removals. I doubt that's the case, but it might be, and I think we should do due dilligence.

I'm adding some in https://chromium-review.googlesource.com/c/chromium/src/+/1899236 but it will be weeks before we have an answer.

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 5, 2019
Add use counters for the constructors of CountQueuingStrategy and
ByteLengthQueuingStrategy. This will help us evaluate their usage, with a view
to possibly deprecating them. See
whatwg/streams#1005 (comment).

Change-Id: I633ab863fd93da8e74ecdd7001815d6a1728c82b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899236
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712498}
@ricea
Copy link
Collaborator Author

ricea commented Mar 16, 2020

Usage is certainly low. CountQueuingStrategy is used on roughly 0.000006% of page loads: https://chromestatus.com/metrics/feature/timeline/popularity/3082.

ByteLengthQueuingStrategy is used on about 0.00004% of page loads: https://chromestatus.com/metrics/feature/timeline/popularity/3083.

Given how easily they can be polyfilled, removal might be an option.

domenic added a commit that referenced this issue Apr 13, 2020
Normative changes, all stemming from the Web IDL adaptation:

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes #586.

* All classes now have [Symbol.toStringTag] properties. (At least, pending whatwg/webidl#357 resolution.) Closes #952.

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes #1005. Note that the size function is not settable anymore, but highWaterMark has a setter.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes #885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes #907.

* Abstract operations are now consistently alphabetized within their section. Closes #684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes #687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes #965.
domenic added a commit that referenced this issue Apr 13, 2020
Normative changes, all stemming from the Web IDL adaptation:

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes #586.

* All classes now have [Symbol.toStringTag] properties. (At least, pending whatwg/webidl#357 resolution.) Closes #952.

* All methods and accesors are now enumerable, per Web IDL defaults, instead of non-enumerable, per ECMAScript defaults.

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes #1005. Note that the size function is not settable anymore, but highWaterMark has a setter.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes #885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes #907.

* Abstract operations are now consistently alphabetized within their section. Closes #684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes #687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes #965.
domenic added a commit that referenced this issue Apr 13, 2020
Normative changes, all stemming from the Web IDL adaptation:

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes #586.

* All classes now have [Symbol.toStringTag] properties. (At least, pending whatwg/webidl#357 resolution.) Closes #952.

* All methods and accesors are now enumerable, per Web IDL defaults, instead of non-enumerable, per ECMAScript defaults.

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes #1005. Note that the size function is not settable anymore, but highWaterMark has a setter.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes #885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes #907.

* Abstract operations are now consistently alphabetized within their section. Closes #684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes #687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes #965.
@domenic
Copy link
Member

domenic commented Apr 15, 2020

I'm updating the tests with the "Specify size as a readonly attribute Function size;" strategy, and noticing that a lot of them do `things like

  assert_equals(CountQueuingStrategy.prototype.size.call(thisValue, chunk), 1);

i.e. they assume you can access CountQueuingStrategy.prototype.size. That is not possible, even with this strategy, because the getter will do a thisArg check. You can do

const strategy = new CountQueuingStrategy({ highWaterMark: 0 });
const size = strategy.size; // passes the getter thisArg check

size(5); // no thisArg while calling
size.call(undefined, chunk); // no thisArg while calling

but I'm really unsure if this is worth it. I think maybe we should just specify size() as a normal Web IDL method.

@ricea
Copy link
Collaborator Author

ricea commented Apr 16, 2020

In Blink we simply fail those tests:

https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/streams/count-queuing-strategy.any-expected.txt

Those tests will have to be changed whatever we do, so I don't think it should be a deciding factor.

@ExE-Boss
Copy link

ExE-Boss commented Apr 16, 2020

Alternatively, we could create size as a data property in a custom binding, similar to how DOMException has a custom binding.


Optionally, we could make it possible to specify data properties in WebIDL.

@domenic
Copy link
Member

domenic commented Apr 16, 2020

My point was this is all not really worth it. The benefits are so small (smaller than I'd realized) and the spec/implementation complexity relatively large. I think we should just make them normal methods.

@domenic
Copy link
Member

domenic commented Apr 16, 2020

Hmm, but if we make them normal methods, then we have to do similar tricks to what we're doing for UnderlyingSource/UnderlyingSink dictionaries. I.e. convert to a QueuingStrategy dictionary in prose and save the original object. So the simplicity gains are not as nice as I thought. OK, maybe I'll just continue with the current path...

Alternately we could add infrastructure to Web IDL to allow invoking methods of a dictionary on the original object, somehow, to handle both cases. But I'm not sure that's a good idea.

domenic added a commit that referenced this issue Jun 11, 2020
Closes #963.

Normative changes to widely-implemented features, roughly in order of most disruptive to least-disruptive:

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes #1005. In particular this means that attempts to set either of them post-creation will throw a TypeError. Chromium already ships these semantics.

* Functions which take a dictionary no longer accept non-objects.

* For the queuing strategy classes, their highWaterMark property will no longer return a non-number from their highWaterMark properties, if one was passed to the constructor. Instead, NaN will be returned.

* All methods and accessors are now enumerable, per Web IDL defaults, instead of non-enumerable, per ECMAScript defaults.

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes #586.

* All classes now have [Symbol.toStringTag] properties. Closes #952.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

* Property lookup in options arguments now happens earlier, at argument-conversion time, and in alphabetical order, per dictionary rules.

Normative changes to unimplemented features:

* ReadableStream's getIterator() method has been renamed to values() as part of adopting Web IDL's infrastructure for async iterators.

* The byobRequest property on ReadableByteStreamController now returns null when there is no BYOB request, instead of returning undefined.

* The view property on ReadableStreamBYOBRequest now returns null when the view cannot be written into, instead of returning undefined.

* Various byte-stream-related APIs that used to specifically prohibit detached buffers now check for zero-length views or buffers, which is a more general category.

* The async iterator's next() and return() methods now behave more like async generators, e.g. returning promises fulfilled with { value: undefined, done: true } after return()ing the iterator, instead of returning a rejected promise.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification. Closes #963. Closes #1017. See #1036 for further followup on the iteration result objects.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes #885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes #907.

* Abstract operations are now consistently alphabetized within their section. Closes #684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes #687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes #965.
yutakahirano pushed a commit to yutakahirano/streams that referenced this issue Jun 3, 2021
Closes whatwg#963.

Normative changes to widely-implemented features, roughly in order of most disruptive to least-disruptive:

* For the queuing strategy classes, their size and highWaterMark properties are now getters on the prototype, instead of data properties on the prototype and instance (respectively). Closes whatwg#1005. In particular this means that attempts to set either of them post-creation will throw a TypeError. Chromium already ships these semantics.

* Functions which take a dictionary no longer accept non-objects.

* For the queuing strategy classes, their highWaterMark property will no longer return a non-number from their highWaterMark properties, if one was passed to the constructor. Instead, NaN will be returned.

* All methods and accessors are now enumerable, per Web IDL defaults, instead of non-enumerable, per ECMAScript defaults.

* All classes are now exposed globally. Formerly, ReadableStreamDefaultReader, ReadableStreamBYOBReader, ReadableStreamDefaultController, ReadableByteStreamController, WritableStreamDefaultWriter, WritableStreamDefaultController, and TransformStreamDefaultController were not exposed. Closes whatwg#586.

* All classes now have [Symbol.toStringTag] properties. Closes whatwg#952.

* Some functions have changed their length property value.

* Some exceptions are thrown earlier, at argument-conversion time.

* Property lookup in options arguments now happens earlier, at argument-conversion time, and in alphabetical order, per dictionary rules.

Normative changes to unimplemented features:

* ReadableStream's getIterator() method has been renamed to values() as part of adopting Web IDL's infrastructure for async iterators.

* The byobRequest property on ReadableByteStreamController now returns null when there is no BYOB request, instead of returning undefined.

* The view property on ReadableStreamBYOBRequest now returns null when the view cannot be written into, instead of returning undefined.

* Various byte-stream-related APIs that used to specifically prohibit detached buffers now check for zero-length views or buffers, which is a more general category.

* The async iterator's next() and return() methods now behave more like async generators, e.g. returning promises fulfilled with { value: undefined, done: true } after return()ing the iterator, instead of returning a rejected promise.

Editorial changes:

* All APIs are specified to using Web IDL now, instead of using a modified version of the ECMAScript specification conventions. We continue using abstract operations and completion records for now, and we have to drop down to the ECMAScript level in a couple places (notably for dealing with %ObjectPrototype% vs. null-prototype iteration result objects, and transferring array buffers). But overall this removes a lot of type-checking and conversion boilerplate from the specification. Closes whatwg#963. Closes whatwg#1017. See whatwg#1036 for further followup on the iteration result objects.

* Individual abstract operations, constructors, methods, and properties no longer have their own heading. They are instead lumped together in sections. Closes whatwg#885.

* The constructors, methods, and properties are now documented in a per-class block, using the usual WHATWG "domintro" style. Closes whatwg#907.

* Abstract operations are now consistently alphabetized within their section. Closes whatwg#684.

* By using Bikeshed's <div algorithm> feature, we now get automatic identifier highlighting. Closes whatwg#687.

* Switched to 100-character line limits, 1-space indents, and omitting end tags, per WHATWG conventions.

* Removed usage of emu-algify in favor of using some more of Bikeshed's built-in features, plus manually annotating a few things.

* Switched to concise Bikeshed linking syntax, e.g. [=term=] and [$AbstractOp$].

* Eliminated a number of utility abstract operations, especially around calling functions, by better using Web IDL.

Other bug fixes:

* Web IDL makes constructor behavior clear, so this closes whatwg#965.
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jun 1, 2024
…stonly

Automatic update from web-platform-tests
Implement QueuingStrategies in C++

Implement CountQueuingStrategy and ByteLengthQueuingStrategy in C++.

Implementing in WebIDL leads to some changes in semantics:

1. highWaterMark is now a getter on the prototype rather than an
property on each object. This is because adding properties directly on
objects is not usual practice in WebIDL, and is problematic to implement
in Blink.

2. size() is now a function returned by a getter. This is because a
methods in WebIDL must be called with |this| set to a valid object of
the appropriate type, but size() is called with |this| set to undefined.
See whatwg/streams#1005 for more discussion.

These aren't expected to change behaviour of code in the wild, but the
count-queuing-strategy.any.js and byte-length-queuing-strategy.js
tests detect it.

Also add tests that subclassing of CountQueuingStrategy and
ByteLengthQueuingStrategyworks works properly. A previous iteration
of this change broke it.

Bug: 981333
Change-Id: Ifc18a469a58f73d54563ca549a1c8db0e001303b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692108
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679897}

--

wpt-commits: 4dbc86fa9b79da17a037140fc2eaba05c80f90ee
wpt-pr: 17804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants