Bug 60184 - Introduce HTML5 track list objects
Summary: Introduce HTML5 track list objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on:
Blocks: 56459 60205 60260 61127
  Show dependency treegraph
 
Reported: 2011-05-04 10:41 PDT by Leandro Graciá Gil
Modified: 2012-09-19 07:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (62.09 KB, patch)
2011-05-04 13:21 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (67.27 KB, patch)
2011-05-04 13:36 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (67.77 KB, patch)
2011-05-17 08:41 PDT, Leandro Graciá Gil
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Patch (67.55 KB, patch)
2011-05-17 08:52 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (66.14 KB, patch)
2011-05-19 08:52 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (66.07 KB, patch)
2011-05-20 04:40 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (66.03 KB, patch)
2011-05-24 06:21 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (66.11 KB, patch)
2011-05-24 09:26 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (66.13 KB, patch)
2011-05-25 03:34 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2011-05-04 10:41:36 PDT
Introduce the TrackList, MultipleTrackList and ExclusiveTrackList objects for their use in the MediaStream API and the HTML Media Element.
Specification: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#tracklist
Comment 1 Leandro Graciá Gil 2011-05-04 13:21:24 PDT
Created attachment 92307 [details]
Patch
Comment 2 Leandro Graciá Gil 2011-05-04 13:36:09 PDT
Created attachment 92310 [details]
Patch

Adding missing changes to EventTarget and JSC/V8 files.
Comment 3 Leandro Graciá Gil 2011-05-17 08:41:23 PDT
Created attachment 93771 [details]
Patch

Adding static methods for the posted tasks (fixes a problem with the lastest code).
Comment 4 Leandro Graciá Gil 2011-05-17 08:52:19 PDT
Created attachment 93773 [details]
Patch

Minor fixes in the CodeGenerators.pri and GNUmakefile.list.am files.
Comment 5 Collabora GTK+ EWS bot 2011-05-17 09:06:13 PDT
Comment on attachment 93771 [details]
Patch

Attachment 93771 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8708226
Comment 6 Eric Carlson 2011-05-19 08:00:12 PDT
Comment on attachment 93773 [details]
Patch

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

> Source/WebCore/dom/ExclusiveTrackList.cpp:76
> +void ExclusiveTrackList::select(long index, ExceptionCode& ec)
> +{
> +    if (index != NoSelection && !checkIndex(index, ec))
> +        return;
> +
> +    // Don't assert since it can be null in degenerate cases like frames detached from their pages.
> +    if (!scriptExecutionContext())
> +        return;
> +
> +    ASSERT(scriptExecutionContext()->isContextThread());
> +    scriptExecutionContext()->postTask(DispatchTask<ExclusiveTrackList, long>::create(this, &ExclusiveTrackList::onSelect, index));
> +}
> +
> +void ExclusiveTrackList::onSelect(long index)
> +{
> +    ASSERT(index >= NoSelection && index < static_cast<long>(length()));
> +    m_selectedIndex = index;
> +
> +    dispatchEvent(Event::create(eventNames().changeEvent, false, false));
> +}

Shouldn't the track be selected synchronously (in select) and only the 'change' event posted asynchronously? The spec only says that the event should happen in the task:

  The select(index) must select the track with index index,
  if there is one, unselecting whichever track was previously
  selected. If there is no track with index index, it must 
  instead throw an INDEX_SIZE_ERR exception.

  Whenever the selected track is changed, the user agent 
  must queue a task to fire a simple event named change 
  at the MultipleTrackList object.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#dom-tracklist-select

Also, is there any reason to not use dispatchChangeEvent() in the base class?



> Source/WebCore/dom/ExclusiveTrackList.idl:32
> +        // FIXME: the spec says unsigned long, but -1 is used when nothing is selected.
> +        // A bug has been already submitted to the spec draft.

It would be good to include the bug url here.


> Source/WebCore/dom/MultipleTrackList.cpp:81
> +void MultipleTrackList::onEnable(unsigned long index)
> +{
> +    ASSERT(index < length());
> +    m_isEnabled[index] = true;
> +
> +    dispatchEvent(Event::create(eventNames().changeEvent, false, false));
> +}

Ditto the question about select() above.


> Source/WebCore/dom/MultipleTrackList.cpp:102
> +void MultipleTrackList::onDisable(unsigned long index)
> +{
> +    ASSERT(index < length());
> +    m_isEnabled[index] = false;
> +
> +    dispatchEvent(Event::create(eventNames().changeEvent, false, false));
> +}

And here.


> Source/WebCore/dom/TrackList.cpp:136
> +ScriptExecutionContext* TrackList::scriptExecutionContext() const
> +{
> +    // FIXME: provide an script execution context for HTML Media Element and MediaStream API use cases.
> +    return 0;
> +}

Bug number?
Comment 7 Leandro Graciá Gil 2011-05-19 08:52:50 PDT
Created attachment 94076 [details]
Patch
Comment 8 Leandro Graciá Gil 2011-05-19 08:53:19 PDT
Comment on attachment 93773 [details]
Patch

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

>> Source/WebCore/dom/ExclusiveTrackList.cpp:76
>> +}
> 
> Shouldn't the track be selected synchronously (in select) and only the 'change' event posted asynchronously? The spec only says that the event should happen in the task:
> 
>   The select(index) must select the track with index index,
>   if there is one, unselecting whichever track was previously
>   selected. If there is no track with index index, it must 
>   instead throw an INDEX_SIZE_ERR exception.
> 
>   Whenever the selected track is changed, the user agent 
>   must queue a task to fire a simple event named change 
>   at the MultipleTrackList object.
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#dom-tracklist-select
> 
> Also, is there any reason to not use dispatchChangeEvent() in the base class?

Fixed the problem with the asynchronous selection. The index is properly checked and raising the appropriate exceptions in the checkIndex method. Now using the postChangeEvent method from the parent class as it should have done from the start.

>> Source/WebCore/dom/ExclusiveTrackList.idl:32
>> +        // A bug has been already submitted to the spec draft.
> 
> It would be good to include the bug url here.

Fixed.

>> Source/WebCore/dom/MultipleTrackList.cpp:81
>> +}
> 
> Ditto the question about select() above.

Fixed.

>> Source/WebCore/dom/MultipleTrackList.cpp:102
>> +}
> 
> And here.

Fixed.

>> Source/WebCore/dom/TrackList.cpp:136
>> +}
> 
> Bug number?

Fixed.
Comment 9 Eric Carlson 2011-05-19 11:27:24 PDT
Comment on attachment 94076 [details]
Patch

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

> Source/WebCore/dom/ExclusiveTrackList.cpp:62
> +    if (index != NoSelection && !checkIndex(index, ec))
> +        return;
> +
> +    ASSERT(index >= NoSelection && index < static_cast<long>(length()));

Doesn't the checkIndex() test above mean that this ASSERT can never fire?


> Source/WebCore/dom/MultipleTrackList.cpp:67
> +    ASSERT(index < length());

Ditto.


> Source/WebCore/dom/MultipleTrackList.cpp:78
> +    ASSERT(index < length());

Ditto.


> Source/WebCore/dom/TrackList.cpp:78
> +bool TrackList::checkIndex(unsigned long index, ExceptionCode& ec) const
> +{
> +    if (index >= length()) {
> +        ec = INDEX_SIZE_ERR;
> +        return false;
> +    }
> +
> +    ec = 0;
> +    return true;
> +}

The spec says "If there is no such track, then the method must instead throw an INDEX_SIZE_ERR exception", so shouldn't this also fail if index is negative?
Comment 10 Leandro Graciá Gil 2011-05-20 04:40:08 PDT
Created attachment 94198 [details]
Patch
Comment 11 Leandro Graciá Gil 2011-05-20 04:40:33 PDT
Comment on attachment 94076 [details]
Patch

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

>> Source/WebCore/dom/ExclusiveTrackList.cpp:62
>> +    ASSERT(index >= NoSelection && index < static_cast<long>(length()));
> 
> Doesn't the checkIndex() test above mean that this ASSERT can never fire?

Fixed. Assert removed.

>> Source/WebCore/dom/MultipleTrackList.cpp:67
>> +    ASSERT(index < length());
> 
> Ditto.

Fixed.

>> Source/WebCore/dom/MultipleTrackList.cpp:78
>> +    ASSERT(index < length());
> 
> Ditto.

Fixed.

>> Source/WebCore/dom/TrackList.cpp:78
>> +}
> 
> The spec says "If there is no such track, then the method must instead throw an INDEX_SIZE_ERR exception", so shouldn't this also fail if index is negative?

The index in this case is unsigned, but I'm fixing this since the ExclusiveTrackList objects will provide signed indexes.
Comment 12 Tony Gentilcore 2011-05-24 05:56:23 PDT
Eric's r+ got lost when uploading a new patch. Just flipping the bit again so this can move forward.
Comment 13 WebKit Commit Bot 2011-05-24 05:58:21 PDT
Comment on attachment 94198 [details]
Patch

Rejecting attachment 94198 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
rackList.h
patching file Source/WebCore/dom/ExclusiveTrackList.idl
patching file Source/WebCore/dom/MultipleTrackList.cpp
patching file Source/WebCore/dom/MultipleTrackList.h
patching file Source/WebCore/dom/MultipleTrackList.idl
patching file Source/WebCore/dom/TrackList.cpp
patching file Source/WebCore/dom/TrackList.h
patching file Source/WebCore/dom/TrackList.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Tony Gentilcore', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8727564
Comment 14 Leandro Graciá Gil 2011-05-24 06:21:13 PDT
Created attachment 94601 [details]
Patch

Rebase only.
Comment 15 WebKit Commit Bot 2011-05-24 08:47:26 PDT
Comment on attachment 94601 [details]
Patch

Rejecting attachment 94601 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
rackList.h
patching file Source/WebCore/dom/ExclusiveTrackList.idl
patching file Source/WebCore/dom/MultipleTrackList.cpp
patching file Source/WebCore/dom/MultipleTrackList.h
patching file Source/WebCore/dom/MultipleTrackList.idl
patching file Source/WebCore/dom/TrackList.cpp
patching file Source/WebCore/dom/TrackList.h
patching file Source/WebCore/dom/TrackList.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Tony Gentilcore', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/8726709
Comment 16 Leandro Graciá Gil 2011-05-24 09:26:53 PDT
Created attachment 94625 [details]
Patch

Rebase only. Bug 56666 introduced merge problems in some project files since GeneratedStream and ExclusiveTrackList were competing for the same line number.
Comment 17 WebKit Commit Bot 2011-05-24 12:33:23 PDT
The commit-queue encountered the following flaky tests while processing attachment 94625 [details]:

http/tests/websocket/tests/workers/worker-handshake-challenge-randomness.html bug 61386 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2011-05-24 12:35:17 PDT
Comment on attachment 94625 [details]
Patch

Rejecting attachment 94625 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 2

Last 500 characters of output:
rce/WebCore/loader/archive/ArchiveResourceCollection.h
	M	Source/WebCore/loader/FrameLoader.h
	M	Source/WebCore/features.pri
	M	Tools/Scripts/build-webkit
	M	Tools/Scripts/webkitperl/features.pm
	M	Tools/Scripts/webkitpy/layout_tests/port/test_files.py
	M	Tools/Scripts/old-run-webkit-tests
	M	Tools/ChangeLog
	M	configure.ac
r87189 = c2019dd7c9ae136ae4719a0956d21c68de17fd2e (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8731443
Comment 19 Leandro Graciá Gil 2011-05-25 03:34:21 PDT
Created attachment 94766 [details]
Patch

Rebasing again to solve the problems in the commit queue.
Comment 20 WebKit Commit Bot 2011-05-25 05:34:16 PDT
Comment on attachment 94766 [details]
Patch

Clearing flags on attachment: 94766

Committed r87293: <http://trac.webkit.org/changeset/87293>
Comment 21 WebKit Commit Bot 2011-05-25 05:34:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Commit Bot 2011-05-25 06:45:48 PDT
The commit-queue encountered the following flaky tests while processing attachment 94766 [details]:

http/tests/websocket/tests/sub-protocol-with-space.html bug 61434 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.