Bug 137472 - [Media] Expose AudioTracks in the "captions" menu.
Summary: [Media] Expose AudioTracks in the "captions" menu.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks: 137505 137474 137525
  Show dependency treegraph
 
Reported: 2014-10-06 17:28 PDT by Jer Noble
Modified: 2014-10-16 14:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.78 MB, patch)
2014-10-06 17:36 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (3.79 MB, patch)
2014-10-06 17:58 PDT, Jer Noble
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (1.42 MB, application/zip)
2014-10-06 19:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (967.64 KB, application/zip)
2014-10-06 19:35 PDT, Build Bot
no flags Details
Patch (3.79 MB, patch)
2014-10-06 20:37 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-10-06 17:28:24 PDT
[Media] Expose AudioTracks in the "captions" menu.
Comment 1 Jer Noble 2014-10-06 17:36:27 PDT
Created attachment 239370 [details]
Patch
Comment 2 Jer Noble 2014-10-06 17:58:53 PDT
Created attachment 239373 [details]
Patch
Comment 3 Build Bot 2014-10-06 19:11:23 PDT
Comment on attachment 239373 [details]
Patch

Attachment 239373 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5349634390097920

New failing tests:
media/video-controls-rendering.html
media/controls-strict.html
media/video-volume-slider.html
media/video-display-toggle.html
media/audio-controls-rendering.html
editing/unsupported-content/list-delete-003.html
editing/unsupported-content/table-type-before.html
editing/unsupported-content/list-delete-001.html
editing/unsupported-content/list-type-after.html
accessibility/media-element.html
media/controls-without-preload.html
editing/unsupported-content/table-delete-002.html
editing/unsupported-content/list-type-before.html
editing/unsupported-content/table-type-after.html
Comment 4 Build Bot 2014-10-06 19:11:25 PDT
Created attachment 239377 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-10-06 19:35:50 PDT
Comment on attachment 239373 [details]
Patch

Attachment 239373 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6299740211773440

New failing tests:
media/controls-strict.html
media/video-volume-slider.html
media/video-display-toggle.html
media/audio-controls-rendering.html
media/video-controls-rendering.html
accessibility/media-element.html
media/controls-without-preload.html
media/controls-after-reload.html
Comment 6 Build Bot 2014-10-06 19:35:52 PDT
Created attachment 239378 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 7 Jer Noble 2014-10-06 20:37:08 PDT
Created attachment 239380 [details]
Patch
Comment 8 Brent Fulgham 2014-10-07 10:25:26 PDT
Comment on attachment 239380 [details]
Patch

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

I'm worried that we need to flesh out the AudioTrack stuff on Windows; there isn't any such thing as an AudioTrackPrivateAVCF/VideoTrackPrivateAVCF. Otherwise, this looks great! Let's get it landed and if anything breaks on Windows I'll clean it up during my ongoing media work.

> Source/WebCore/html/track/AudioTrack.cpp:-150
> -    setEnabled(enabled);

Just Curious: Why don't we want to recurse into the private 'setEnabled' here? What problem does that fix?

> Source/WebCore/page/CaptionUserPreferences.cpp:228
> +    });

Oooh!  A lambda! :-)
Comment 9 Jer Noble 2014-10-07 10:37:29 PDT
(In reply to comment #8)
> (From update of attachment 239380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239380&action=review
> 
> I'm worried that we need to flesh out the AudioTrack stuff on Windows; there isn't any such thing as an AudioTrackPrivateAVCF/VideoTrackPrivateAVCF. Otherwise, this looks great! Let's get it landed and if anything breaks on Windows I'll clean it up during my ongoing media work.

Thanks!

> > Source/WebCore/html/track/AudioTrack.cpp:-150
> > -    setEnabled(enabled);
> 
> Just Curious: Why don't we want to recurse into the private 'setEnabled' here? What problem does that fix?

Previously, calling AudioTrack::setEnabled() calls
  AudioTrackPrivate::setEnabled() which calls
    AudioTrack::enabledChanged() which calls
      AudioTrackPrivate::setEnabled() which ...

The chain only breaks when the AudioTrackPrivate notices that the enabled param == the stored value of enabled.  What if some future AudioTrackPrivate doesn't cache the value of enabled? Or doesn't check?

> > Source/WebCore/page/CaptionUserPreferences.cpp:228
> > +    });
> 
> Oooh!  A lambda! :-)
Comment 10 WebKit Commit Bot 2014-10-07 12:15:36 PDT
Comment on attachment 239380 [details]
Patch

Clearing flags on attachment: 239380

Committed r174402: <http://trac.webkit.org/changeset/174402>
Comment 11 WebKit Commit Bot 2014-10-07 12:15:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Alexey Proskuryakov 2014-10-08 10:07:14 PDT
https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Ftrack%2Ftrack-forced-subtitles-in-band.html

-EXPECTED (trackMenuItems.length == '6') OK
+EXPECTED (trackMenuItems.length == '6'), OBSERVED '3' FAIL
Comment 14 Jer Noble 2014-10-08 10:20:34 PDT
(In reply to comment #13)
> https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Ftrack%2Ftrack-forced-subtitles-in-band.html
> 
> -EXPECTED (trackMenuItems.length == '6') OK
> +EXPECTED (trackMenuItems.length == '6'), OBSERVED '3' FAIL

https://bugs.webkit.org/show_bug.cgi?id=137505
Comment 15 Jon Lee 2014-10-16 14:50:41 PDT
rdar://problem/13466748