Bug 175888

Summary: HTMLTrackElement behavior violates the standard
Product: WebKit Reporter: Kirill Ovchinnikov <kirill.ovchinn>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: buildbot, commit-queue, eric.carlson, jer.noble, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=175031
Attachments:
Description Flags
Updated `layout test` to check described issues
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch none

Description Kirill Ovchinnikov 2017-08-23 09:50:24 PDT
Created attachment 318878 [details]
Updated `layout test` to check described issues

- on every TextTrack.cues() and TextTrack.active_cues call the same  the same TextTrackCueList object must be returned each time.
(like the standard requires: https://html.spec.whatwg.org/multipage/media.html#text-track-api:texttrack-22) - now WebKit creates a new object after setting track's src to the empty value

- 'error' event should be fired if the new URL is empty - now WebKit just ignores this.
(https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:event-track-error)

- WebKit now clears TextTrack's cues after 'src' attribute change (see https://bugs.webkit.org/show_bug.cgi?id=175031 for details), but the cues list is also cleared if the new URL is equal to the previous URL (in this case the URL is not really 'changed'), and because of that, if the same URL is set twice, the cue list would be cleared, which doesn't seem like the behavior one would expect.
Comment 1 Kirill Ovchinnikov 2017-08-23 10:01:21 PDT
Created attachment 318881 [details]
Patch
Comment 2 Eric Carlson 2017-08-23 10:35:41 PDT
Comment on attachment 318881 [details]
Patch

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

r=me with a few minor suggestions.

Thank you!

> Source/WebCore/html/track/TextTrackCueList.cpp:109
> +void TextTrackCueList::removeAll()

Nit: I think "clear" would be a slightly better name than "removeAll"

> LayoutTests/media/track/text-track-src-change.html:33
>                          cues = testTrack.track.cues;
> +                        testExpected("cues === testTrack.track.cues", true);

This test can't fail. Did you mean to delete "cues = testTrack.track.cues;"?

> LayoutTests/media/track/text-track-src-change.html:51
> +                        errorEventTimer = setTimeout(loadedTrackTest(), 100);

This timeout is too short, it will fire before the error event on heavily loaded machines (e.g. on the bots). I would set it to a much larger value, say 10 seconds.
Comment 3 Build Bot 2017-08-23 11:07:45 PDT
Comment on attachment 318881 [details]
Patch

Attachment 318881 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4369302

New failing tests:
http/tests/security/text-track-crossorigin.html
Comment 4 Build Bot 2017-08-23 11:07:47 PDT
Created attachment 318890 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Build Bot 2017-08-23 11:14:13 PDT
Comment on attachment 318881 [details]
Patch

Attachment 318881 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4369314

New failing tests:
http/tests/security/text-track-crossorigin.html
Comment 6 Build Bot 2017-08-23 11:14:15 PDT
Created attachment 318891 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-08-23 11:21:28 PDT
Comment on attachment 318881 [details]
Patch

Attachment 318881 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4369313

New failing tests:
http/tests/security/text-track-crossorigin.html
Comment 8 Build Bot 2017-08-23 11:21:29 PDT
Created attachment 318892 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-08-23 12:46:41 PDT
Comment on attachment 318881 [details]
Patch

Attachment 318881 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4369666

New failing tests:
http/tests/security/text-track-crossorigin.html
Comment 10 Build Bot 2017-08-23 12:46:43 PDT
Created attachment 318903 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 11 Kirill Ovchinnikov 2017-08-24 06:16:19 PDT
Created attachment 318983 [details]
Patch
Comment 12 Kirill Ovchinnikov 2017-08-24 06:17:10 PDT
Prepared a new patch with some fixes mentioned in comments and tests' results.
Comment 13 WebKit Commit Bot 2017-08-24 12:44:24 PDT
Comment on attachment 318983 [details]
Patch

Clearing flags on attachment: 318983

Committed r221155: <http://trac.webkit.org/changeset/221155>
Comment 14 WebKit Commit Bot 2017-08-24 12:44:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-08-24 12:45:39 PDT
<rdar://problem/34064737>