Bug 60184

Created attachment 92307 [details] Patch

Created attachment 92310 [details] Patch Adding missing changes to EventTarget and JSC/V8 files.

Created attachment 93771 [details] Patch Adding static methods for the posted tasks (fixes a problem with the lastest code).

Created attachment 93773 [details] Patch Minor fixes in the CodeGenerators.pri and GNUmakefile.list.am files.

Comment on attachment 93771 [details] Patch Attachment 93771 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8708226

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?

Created attachment 94076 [details] Patch

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 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?

Created attachment 94198 [details] Patch

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.

Eric's r+ got lost when uploading a new patch. Just flipping the bit again so this can move forward.

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

Created attachment 94601 [details] Patch Rebase only.

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

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.

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 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

Created attachment 94766 [details] Patch Rebasing again to solve the problems in the commit queue.

Comment on attachment 94766 [details] Patch Clearing flags on attachment: 94766 Committed r87293: <http://trac.webkit.org/changeset/87293>

All reviewed patches have been landed. Closing bug.

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.