Implement ByteLengthQueuingStrategy

Implement ByteLengthQueuingStrategy

This implements the ByteLengthQueuingStrategy class, using experimental V8
extras (so that it will only show up with experimental web platform features
enabled). It is a very simple initial step toward implementing ReadableStream
using the same mechanisms.

BUG=503491
R=yhirano@chromium.org,tyoshino@chromium.org,jochen@chromium.org

Committed: https://crrev.com/f630fa0f19dec2f48f4cdd1fcc94488ebe6d3d9e
Cr-Commit-Position: refs/heads/master@{#352951}

[domenic, 2015-09-30 22:49:07]

See
https://github.com/whatwg/streams/commit/bbb51cb064358e99263545cc51d4c7de3c15...
for a start at trying to move the tests over to web-platform-tests format in
whatwg/streams. This should make it easier to sync upstream over time.

[domenic, 2015-09-30 23:57:15]

domenic@chromium.org changed reviewers:
+ tyoshino@chromium.org

[yhirano, 2015-10-01 02:15:51]

I'd love to have a style rule guideline for JS code using V8 Extra. The
guideline would be Blink-wide because V8 Extra will also be used by other
modules. We don't have to wait for the conclusion, but we can start a discussion
in blink-dev. What do you think?

[domenic, 2015-10-01 16:16:33]

On 2015/10/01 at 02:15:51, yhirano wrote:
> I'd love to have a style rule guideline for JS code using V8 Extra. The
guideline would be Blink-wide because V8 Extra will also be used by other
modules. We don't have to wait for the conclusion, but we can start a discussion
in blink-dev. What do you think?

Yes, that makes sense to me. I will start a blink-dev thread.

[yhirano, 2015-10-02 07:27:16]

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js
(right):

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js:44:

It would be good to have a test checking that "highwaterMark" value is settable
after construction.

[tyoshino (SeeGerritForStatus), 2015-10-02 11:33:28]

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.html
(right):

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.html:12:
service_worker_test('byte-length-queuing-strategy.js');
So, this test is run with HTTP server (placed in http/tests/...) since we want
to check that it works under ServiceWorker environment?

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js
(right):

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js:43:
}, 'ByteLengthQueuingStrategy constructor behaves as expected with wrong
arguments');
nitpick: is the first test case considered to be using a wrong argument?

[domenic, 2015-10-02 22:32:15]

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.html
(right):

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.html:12:
service_worker_test('byte-length-queuing-strategy.js');
On 2015/10/02 at 11:33:28, tyoshino wrote:
> So, this test is run with HTTP server (placed in http/tests/...) since we want
to check that it works under ServiceWorker environment?

Yes. I followed the pattern from the promise rejection event tests which are
another cross-environment feature.

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js
(right):

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js:43:
}, 'ByteLengthQueuingStrategy constructor behaves as expected with wrong
arguments');
On 2015/10/02 at 11:33:28, tyoshino wrote:
> nitpick: is the first test case considered to be using a wrong argument?

Yeah. (The wrong argument is undefined.)

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js:44:

On 2015/10/02 at 07:27:16, yhirano wrote:
> It would be good to have a test checking that "highwaterMark" value is
settable after construction.

Added. Kind of redundant with the property descriptor check but might as well
have it.

[yhirano, 2015-10-05 01:22:40]

lgtm

[tyoshino (SeeGerritForStatus), 2015-10-05 07:55:46]

lgtm

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js
(right):

https://codereview.chromium.org/1378213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/streams/byte-length-queuing-strategy.js:43:
}, 'ByteLengthQueuingStrategy constructor behaves as expected with wrong
arguments');
On 2015/10/02 22:32:14, domenic wrote:
> On 2015/10/02 at 11:33:28, tyoshino wrote:
> > nitpick: is the first test case considered to be using a wrong argument?
> 
> Yeah. (The wrong argument is undefined.)

Ahh, sorry. By "first case", I meant highWaterMarkObjectGetter. I was looking at
L24-L27 and said "first". It was confusing.

So, my question was "new ByteLengthQueuingStrategy(highWaterMarkObjectGetter);"
is considered to be using a wrong argument?"

Any way, it's not a big deal.

https://codereview.chromium.org/1378213002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ByteLengthQueuingStrategy.js
(right):

https://codereview.chromium.org/1378213002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ByteLengthQueuingStrategy.js:24:
writable: true
Could you teach me which part of the ECMA spec I should read to learn how these
data property attributes are determined?

[domenic, 2015-10-05 17:31:07]

On 2015/10/05 at 07:55:46, tyoshino wrote:

> Ahh, sorry. By "first case", I meant highWaterMarkObjectGetter. I was looking
at L24-L27 and said "first". It was confusing.

Oh, I see now. I will change it from "wrong" to "strange".

>
https://codereview.chromium.org/1378213002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/streams/ByteLengthQueuingStrategy.js:24:
writable: true
> Could you teach me which part of the ECMA spec I should read to learn how
these data property attributes are determined?

It is indeed somewhat hard to find.
http://tc39.github.io/ecma262/#sec-ecmascript-standard-built-in-objects says
"Every other data property described in clauses 18 through 26 and in Annex B.2
has the attributes { [[Writable]]: true, [[Enumerable]]: false,
[[Configurable]]: true } unless otherwise specified."

[domenic, 2015-10-05 17:48:55]

The CQ bit was checked by domenic@chromium.org

[domenic, 2015-10-05 17:48:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1378213002/#ps40001
(title: "Tweak test description")

[commit-bot: I haz the power, 2015-10-05 17:50:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1378213002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1378213002/40001

[commit-bot: I haz the power, 2015-10-05 18:30:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 18:30:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[domenic, 2015-10-05 18:32:20]

The CQ bit was unchecked by domenic@chromium.org

[domenic, 2015-10-05 18:32:43]

domenic@chromium.org changed reviewers:
+ jochen@chromium.org

[domenic, 2015-10-05 18:33:55]

On 2015/10/05 at 18:30:36, commit-bot wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Need owner-LGTM for build/common.gypi so I added jochen@. Maybe there is a
better way to do this, e.g. core/streams should have its own .gypi that gets
included, and then we don't need a build owner LGTM for all V8 extras? What do
you think, Jochen?

[tyoshino (SeeGerritForStatus), 2015-10-06 04:16:26]

On 2015/10/05 17:31:07, domenic wrote:
> On 2015/10/05 at 07:55:46, tyoshino wrote:
> 
> > Ahh, sorry. By "first case", I meant highWaterMarkObjectGetter. I was
looking
> at L24-L27 and said "first". It was confusing.
> 
> Oh, I see now. I will change it from "wrong" to "strange".
> 

Thanks

> >
>
https://codereview.chromium.org/1378213002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/streams/ByteLengthQueuingStrategy.js:24:
> writable: true
> > Could you teach me which part of the ECMA spec I should read to learn how
> these data property attributes are determined?
> 
> It is indeed somewhat hard to find.
> http://tc39.github.io/ecma262/#sec-ecmascript-standard-built-in-objects says
> "Every other data property described in clauses 18 through 26 and in Annex B.2
> has the attributes { [[Writable]]: true, [[Enumerable]]: false,
> [[Configurable]]: true } unless otherwise specified."

Got it. Thanks!

still lgtm

[jochen (gone - plz use gerrit), 2015-10-07 09:33:25]

lgtm

the definition needs to be in common.gypi because that's the only file that is
included by all gyp files (including v8's gyp files).

we could move it to a separate gypi file that is included from common.gypi.
ideally, that file would be restrict to api owners, so nobody accidentally
launches some v8 extra without an approved intent to ship.

for that, maybe add a comment to the variable declaration in common.gypi that
asks ppl to get an review from an blink api owner for changes here?

[domenic, 2015-10-07 16:31:48]

On 2015/10/07 at 09:33:25, jochen wrote:
> lgtm
> 
> the definition needs to be in common.gypi because that's the only file that is
included by all gyp files (including v8's gyp files).
> 
> we could move it to a separate gypi file that is included from common.gypi.
ideally, that file would be restrict to api owners, so nobody accidentally
launches some v8 extra without an approved intent to ship.
> 
> for that, maybe add a comment to the variable declaration in common.gypi that
asks ppl to get an review from an blink api owner for changes here?

So, I just uploaded a new patch set that adds a v8extras.gypi inside
core/streams that we can edit, and includes that from build/common.gypi. That
way, we can continue adding experimental extras files as we work on them without
needing review from a common.gypi owner. I also added a comment in
core/streams/v8extras.gypi saying effectively "moving stuff from experimental to
non-experimental requires API owners review".

Does this seem good?

Also ... do I need to duplicate all of this for gn too?

[jochen (gone - plz use gerrit), 2015-10-07 16:53:30]

On 2015/10/07 at 16:31:48, domenic wrote:
> On 2015/10/07 at 09:33:25, jochen wrote:
> > lgtm
> > 
> > the definition needs to be in common.gypi because that's the only file that
is included by all gyp files (including v8's gyp files).
> > 
> > we could move it to a separate gypi file that is included from common.gypi.
ideally, that file would be restrict to api owners, so nobody accidentally
launches some v8 extra without an approved intent to ship.
> > 
> > for that, maybe add a comment to the variable declaration in common.gypi
that asks ppl to get an review from an blink api owner for changes here?
> 
> So, I just uploaded a new patch set that adds a v8extras.gypi inside
core/streams that we can edit, and includes that from build/common.gypi. That
way, we can continue adding experimental extras files as we work on them without
needing review from a common.gypi owner. I also added a comment in
core/streams/v8extras.gypi saying effectively "moving stuff from experimental to
non-experimental requires API owners review".
> 
> Does this seem good?


Can you restrict the file to api owners? We also require an api owner review for
experimental runtime features

> 
> Also ... do I need to duplicate all of this for gn too?

Yes

[domenic, 2015-10-07 16:55:41]

On 2015/10/07 at 16:53:30, jochen wrote:
> On 2015/10/07 at 16:31:48, domenic wrote:
> > On 2015/10/07 at 09:33:25, jochen wrote:
> > > lgtm
> > > 
> > > the definition needs to be in common.gypi because that's the only file
that is included by all gyp files (including v8's gyp files).
> > > 
> > > we could move it to a separate gypi file that is included from
common.gypi. ideally, that file would be restrict to api owners, so nobody
accidentally launches some v8 extra without an approved intent to ship.
> > > 
> > > for that, maybe add a comment to the variable declaration in common.gypi
that asks ppl to get an review from an blink api owner for changes here?
> > 
> > So, I just uploaded a new patch set that adds a v8extras.gypi inside
core/streams that we can edit, and includes that from build/common.gypi. That
way, we can continue adding experimental extras files as we work on them without
needing review from a common.gypi owner. I also added a comment in
core/streams/v8extras.gypi saying effectively "moving stuff from experimental to
non-experimental requires API owners review".
> > 
> > Does this seem good?
> 
> 
> Can you restrict the file to api owners? We also require an api owner review
for experimental runtime features
>
> 
> > 
> > Also ... do I need to duplicate all of this for gn too?
> 
> Yes

Oh, I see. In that case it seems better to go back to inlining everything into
build/common.gypi and build/module_args/v8.gni since we haven't lightened
anyone's review burden by doing this. Otherwise I'll have to create and maintain
parallel .gypi and .gni files anyway.

[domenic, 2015-10-07 17:05:50]

The CQ bit was checked by domenic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-07 17:10:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1378213002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1378213002/80001

[commit-bot: I haz the power, 2015-10-07 17:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-07 17:31:01]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[domenic, 2015-10-07 17:34:01]

The CQ bit was checked by domenic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-07 17:36:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1378213002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1378213002/100001

[domenic, 2015-10-07 19:05:08]

The CQ bit was unchecked by domenic@chromium.org

[domenic, 2015-10-07 19:09:10]

The CQ bit was checked by domenic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-07 19:11:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2015-10-07 19:11:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1378213002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1378213002/120001

[commit-bot: I haz the power, 2015-10-07 20:32:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-07 20:32:49]

Dry run: This issue passed the CQ dry run.

[domenic, 2015-10-07 21:35:41]

The CQ bit was checked by domenic@chromium.org

[domenic, 2015-10-07 21:35:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, jochen@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1378213002/#ps120001
(title: "Fix webexposed tests")

[commit-bot: I haz the power, 2015-10-07 21:37:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1378213002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1378213002/120001

[commit-bot: I haz the power, 2015-10-07 21:46:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-07 21:46:51]

Patchset 7 (id:??) landed as
https://crrev.com/f630fa0f19dec2f48f4cdd1fcc94488ebe6d3d9e
Cr-Commit-Position: refs/heads/master@{#352951}