Convert HTMLTrackElement timer to per-frame scheduler as Networking.

Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
Networking.

https://html.spec.whatwg.org/#the-track-element

BUG=624694
Review-Url: https://codereview.chromium.org/2637833003
Cr-Commit-Position: refs/heads/master@{#444696}
Committed: https://chromium.googlesource.com/chromium/src/+/45ea2ba70b8a87d266cceb0dc7f824a21da74da0

[joelhockey, 2017-01-17 08:08:36]

Description was changed from

==========
Convert HTMLTrackElement timer to per-frame scheduler as MediaElementEvent.


work


work

BUG=
==========

to

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
MediaElementEvent.


BUG=624694
==========

[joelhockey, 2017-01-17 08:08:36]

joelhockey@chromium.org changed reviewers:
+ nverne@chromium.org, sashab@chromium.org, slangley@chromium.org

[joelhockey, 2017-01-17 08:09:24]

Description was changed from

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
MediaElementEvent.


BUG=624694
==========

to

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
MediaElementEvent.

https://html.spec.whatwg.org/#the-track-element

BUG=624694
==========

[joelhockey, 2017-01-17 08:09:42]

Please review HTMLTrackElement timer conversion.

[joelhockey, 2017-01-17 08:09:53]

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

[commit-bot: I haz the power, 2017-01-17 08:10:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sashab, 2017-01-17 08:10:36]

LGTM, but above spec link in CL desc put "Link to spec:" or something :)

Also not sure if this is a media element event, will let haraken take a look :)

[joelhockey, 2017-01-17 08:12:33]

joelhockey@chromium.org changed reviewers:
+ haraken@chromium.org
- nverne@chromium.org, slangley@chromium.org

[joelhockey, 2017-01-17 08:13:12]

Haraken, I'm not sure if MediaElementEvent is the correct TaskType for
HTMLTrackElement.

[haraken, 2017-01-17 09:02:03]

haraken@chromium.org changed reviewers:
+ foolip@chromium.org

[haraken, 2017-01-17 09:02:04]

+foolip

Hmm. It's not clear from the spec what the task source we should use :/

I guess we should use the networking task source because the timer is handling
loading operations but I want to have foolip@ confirm it.

[fs, 2017-01-17 09:10:28]

On 2017/01/17 at 08:13:12, joelhockey wrote:
> Haraken, I'm not sure if MediaElementEvent is the correct TaskType for
HTMLTrackElement.

The thing this timer achieves is the emulation of a "stable state" (see end of
HTMLTrackElement::scheduleLoad and [1]), so the "right thing (TM)" here is
likely to queue a microtask (a mechanism I believe was unavailable when this
code was written.)

[1]
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...

[haraken, 2017-01-17 09:17:36]

On 2017/01/17 09:10:28, fs wrote:
> On 2017/01/17 at 08:13:12, joelhockey wrote:
> > Haraken, I'm not sure if MediaElementEvent is the correct TaskType for
> HTMLTrackElement.
> 
> The thing this timer achieves is the emulation of a "stable state" (see end of
> HTMLTrackElement::scheduleLoad and [1]), so the "right thing (TM)" here is
> likely to queue a microtask (a mechanism I believe was unavailable when this
> code was written.)
> 
> [1]
>
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...

Thanks fs!

https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...

"The tasks queued by the fetching algorithm on the networking task source to
process the data as it is being fetched must determine the type of the
resource."

Maybe should we use the networking task source?

[commit-bot: I haz the power, 2017-01-17 09:25:36]

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

[commit-bot: I haz the power, 2017-01-17 09:25:36]

Dry run: This issue passed the CQ dry run.

[joelhockey, 2017-01-19 04:49:45]

On 2017/01/17 at 09:17:36, haraken wrote:
> On 2017/01/17 09:10:28, fs wrote:
> > On 2017/01/17 at 08:13:12, joelhockey wrote:
> > > Haraken, I'm not sure if MediaElementEvent is the correct TaskType for
> > HTMLTrackElement.
> > 
> > The thing this timer achieves is the emulation of a "stable state" (see end
of
> > HTMLTrackElement::scheduleLoad and [1]), so the "right thing (TM)" here is
> > likely to queue a microtask (a mechanism I believe was unavailable when this
> > code was written.)
> > 
> > [1]
> >
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...
> 
> Thanks fs!
> 
>
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...
> 
> "The tasks queued by the fetching algorithm on the networking task source to
process the data as it is being fetched must determine the type of the
resource."
> 
> Maybe should we use the networking task source?

I will update TaskType to Networking.
haraken and fs, let me know if you have come to a consensus or if I should
change again.

[haraken, 2017-01-19 06:38:07]

LGTM

[joelhockey, 2017-01-19 09:01:35]

The CQ bit was checked by joelhockey@chromium.org

[joelhockey, 2017-01-19 09:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/2637833003/#ps40001
(title: "Convert HTMLTrackElement timer to per-frame scheduler as
MediaElementEvent.")

[commit-bot: I haz the power, 2017-01-19 09:02:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 10:16:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484816495085140,
"parent_rev": "299f619e031f43824d555ed8d7777caa6c835eeb", "commit_rev":
"45ea2ba70b8a87d266cceb0dc7f824a21da74da0"}

[commit-bot: I haz the power, 2017-01-19 10:17:02]

Description was changed from

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
MediaElementEvent.

https://html.spec.whatwg.org/#the-track-element

BUG=624694
==========

to

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
MediaElementEvent.

https://html.spec.whatwg.org/#the-track-element

BUG=624694

Review-Url: https://codereview.chromium.org/2637833003
Cr-Commit-Position: refs/heads/master@{#444696}
Committed:
https://chromium.googlesource.com/chromium/src/+/45ea2ba70b8a87d266cceb0dc7f8...
==========

[commit-bot: I haz the power, 2017-01-19 10:17:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/45ea2ba70b8a87d266cceb0dc7f8...

[fs, 2017-01-19 11:56:11]

(Looks like I fell out of the CC-list here, don't know how that happened... so
didn't see this before the CL landed =( )

On 2017/01/17 at 09:17:36, haraken wrote:
> On 2017/01/17 09:10:28, fs wrote:
> > On 2017/01/17 at 08:13:12, joelhockey wrote:
> > > Haraken, I'm not sure if MediaElementEvent is the correct TaskType for
> > HTMLTrackElement.
> > 
> > The thing this timer achieves is the emulation of a "stable state" (see end
of
> > HTMLTrackElement::scheduleLoad and [1]), so the "right thing (TM)" here is
> > likely to queue a microtask (a mechanism I believe was unavailable when this
> > code was written.)
> > 
> > [1]
> >
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...
> 
> Thanks fs!
> 
>
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...
> 
> "The tasks queued by the fetching algorithm on the networking task source to
process the data as it is being fetched must determine the type of the
resource."
> 
> Maybe should we use the networking task source?

No, that would be conflating the 'fetch' part of the algorithm with the 'await a
stable state' bit. It would probably make sense to make this separation more
obvious by pushing some of the code in scheduleLoad into TextTrackLoader, and
then make sure to always call didCompleteLoad() on the networking task source or
so.

That being said though, using the networking task source here is equally good
(bad) to what was before (modulo throttling decisions.)

[joelhockey, 2017-01-19 12:28:40]

Description was changed from

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
MediaElementEvent.

https://html.spec.whatwg.org/#the-track-element

BUG=624694

Review-Url: https://codereview.chromium.org/2637833003
Cr-Commit-Position: refs/heads/master@{#444696}
Committed:
https://chromium.googlesource.com/chromium/src/+/45ea2ba70b8a87d266cceb0dc7f8...
==========

to

==========
Convert HTMLTrackElement timer to per-frame scheduler.

Change m_loadTimer to TaskRunnerTimer with TaskType
Networking.

https://html.spec.whatwg.org/#the-track-element

BUG=624694

Review-Url: https://codereview.chromium.org/2637833003
Cr-Commit-Position: refs/heads/master@{#444696}
Committed:
https://chromium.googlesource.com/chromium/src/+/45ea2ba70b8a87d266cceb0dc7f8...
==========

[joelhockey, 2017-01-19 12:35:56]

On 2017/01/19 at 11:56:40, fs wrote:
> 

Sorry fs and haraken, I wouldn't have submitted if I thought that there was
still discussion to complete.
Do let me know if I should either revert this, or if there is a specific further
change I should make.

[fs, 2017-01-19 13:17:07]

On 2017/01/19 at 12:35:56, joelhockey wrote:
> On 2017/01/19 at 11:56:40, fs wrote:
> > 
> 
> Sorry fs and haraken, I wouldn't have submitted if I thought that there was
still discussion to complete.
> Do let me know if I should either revert this, or if there is a specific
further change I should make.

As I mentioned, this is fine, but it is also roughly as "wrong" as before. I
guess someone (TM) will have to fix it (make it approximate the spec better) at
some point...

[haraken, 2017-01-19 13:47:14]

On 2017/01/19 13:17:07, fs wrote:
> On 2017/01/19 at 12:35:56, joelhockey wrote:
> > On 2017/01/19 at 11:56:40, fs wrote:
> > > 
> > 
> > Sorry fs and haraken, I wouldn't have submitted if I thought that there was
> still discussion to complete.
> > Do let me know if I should either revert this, or if there is a specific
> further change I should make.
> 
> As I mentioned, this is fine, but it is also roughly as "wrong" as before. I
> guess someone (TM) will have to fix it (make it approximate the spec better)
at
> some point...

Shall we add a comment and mention that that should use a microtask at the
moment?

[foolip, 2017-01-24 10:28:21]

On 2017/01/19 13:47:14, haraken wrote:
> On 2017/01/19 13:17:07, fs wrote:
> > On 2017/01/19 at 12:35:56, joelhockey wrote:
> > > On 2017/01/19 at 11:56:40, fs wrote:
> > > > 
> > > 
> > > Sorry fs and haraken, I wouldn't have submitted if I thought that there
was
> > still discussion to complete.
> > > Do let me know if I should either revert this, or if there is a specific
> > further change I should make.
> > 
> > As I mentioned, this is fine, but it is also roughly as "wrong" as before. I
> > guess someone (TM) will have to fix it (make it approximate the spec better)
> at
> > some point...
> 
> Shall we add a comment and mention that that should use a microtask at the
> moment?

Sorry I didn't respond earlier, but that would be good. In the spec this is
https://html.spec.whatwg.org/multipage/embedded-content.html#start-the-track-...
so a microtask would be correct. joelhockey@, I think the change itself should
be quite trivial, if you want to give it a try. I think it should be possible to
test like this:

<!DOCTYPE html>
<video></video>
<script>
async_test(t => {
  var video = document.querySelector('video');
  var track = document.createElement('track');
  track.src = 'foo.vtt'; // doesn't need to actually exist
  video.appendChild(track);
  assert_equals(track.readyState, HTMLTrackElement.NONE);

  // The "start the track processing model" algorithm should run at next stable
state
  Promise.resolve().then(t.step_func_done(() => {
    assert_equals(track.readyState, HTMLTrackElement.LOADING);
  }));
});
</script>

If it doesn't seem to work and you have better things to do, maybe you can file
a bug and link to it?