[Mac] Represent AVMediaSelectionOptions as AudioTracks
Created attachment 239389 [details] Patch
Created attachment 239413 [details] Patch
Comment on attachment 239413 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239413&action=review I think this needs some cleanup before landing, but otherwise looks great! > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.h:78 > + RefPtr<MediaSelectionOptionAVFObjC> m_mediaSelectionOption; It almost seems like these should be two distinct classes; AVTrackPrivates that are backed by AVPlayerItemTracks, and AVTrackPrivates that are backed by media selection options. But that would probably add needless complexity... The upside would be that it would do away with the weird "dual" nature of all the methods below. > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:122 > + Extra space... > Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.h:58 > + static RefPtr<AudioTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option) Should this be a const reference? > Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:42 > + : m_impl(std::make_unique<AVTrackPrivateAVFObjCImpl>(option)) Const reference for option? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1860 > + for (auto &oldItem : oldItems) { What!?! Ugh! Is this some horrible ObjC coding standard we use? Hmm. Elsewhere in the file we use "for (auto& oldItem..." like human beings, so I think this is a typo. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1876 > + for (auto & addedItem : addedItems) Zounds! Another variation on weird reference spacing. No! > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1916 > + for (auto &oldItem : oldItems) { Nit: Bad reference spacing. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1932 > + for (auto & addedItem : addedItems) Nit: Extra space here. On the LEFT side of the '&'. > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:47 > +VideoTrackPrivateAVFObjC::VideoTrackPrivateAVFObjC(MediaSelectionOptionAVFObjC& option) Should this be a const reference, since we're using it as an argument to a constructor? > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:88 > +void VideoTrackPrivateAVFObjC::setMediaSelectonOption(MediaSelectionOptionAVFObjC& option) Ditto: Const reference perhaps? > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:57 > + static RefPtr<VideoTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option) Nit: Maybe this should be a const reference since we're only using it to construct an object?
(In reply to comment #3) > (From update of attachment 239413 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239413&action=review > > I think this needs some cleanup before landing, but otherwise looks great! > > > Source/WebCore/platform/graphics/avfoundation/AVTrackPrivateAVFObjCImpl.h:78 > > + RefPtr<MediaSelectionOptionAVFObjC> m_mediaSelectionOption; > > It almost seems like these should be two distinct classes; AVTrackPrivates that are backed by AVPlayerItemTracks, and AVTrackPrivates that are backed by media selection options. But that would probably add needless complexity... > > The upside would be that it would do away with the weird "dual" nature of all the methods below. Yes, that's true, but there are a couple of places where they share part of an implementation. Language selection, for instance. And bifurcating them into two classes would require making this class virtual, and I wanted to avoid that if at all possible. > > Source/WebCore/platform/graphics/avfoundation/MediaSelectionGroupAVFObjC.mm:122 > > + > > Extra space... > > > Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.h:58 > > + static RefPtr<AudioTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option) > > Should this be a const reference? No, because the AVTrackPrivateAVFObjCImpl needs to be able to call non-const methods on it. > > Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:42 > > + : m_impl(std::make_unique<AVTrackPrivateAVFObjCImpl>(option)) > > Const reference for option? Ditto. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1860 > > + for (auto &oldItem : oldItems) { > > What!?! Ugh! Is this some horrible ObjC coding standard we use? Hmm. Elsewhere in the file we use "for (auto& oldItem..." like human beings, so I think this is a typo. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1876 > > + for (auto & addedItem : addedItems) > > Zounds! Another variation on weird reference spacing. No! > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1916 > > + for (auto &oldItem : oldItems) { > > Nit: Bad reference spacing. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1932 > > + for (auto & addedItem : addedItems) > > Nit: Extra space here. On the LEFT side of the '&'. Ick. Fixed all these spacing issues. > > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:47 > > +VideoTrackPrivateAVFObjC::VideoTrackPrivateAVFObjC(MediaSelectionOptionAVFObjC& option) > > Should this be a const reference, since we're using it as an argument to a constructor? Same as above. No, because we need to be able to call non-const methods on it. > > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.cpp:88 > > +void VideoTrackPrivateAVFObjC::setMediaSelectonOption(MediaSelectionOptionAVFObjC& option) > > Ditto: Const reference perhaps? Ditto. > > Source/WebCore/platform/graphics/avfoundation/objc/VideoTrackPrivateAVFObjC.h:57 > > + static RefPtr<VideoTrackPrivateAVFObjC> create(MediaSelectionOptionAVFObjC& option) > > Nit: Maybe this should be a const reference since we're only using it to construct an object? Ditto.
Created attachment 239585 [details] Patch for landing
Attachment 239585 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2736: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239588 [details] Patch for landing
Attachment 239588 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2736: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239600 [details] Patch for landing
Attachment 239600 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2735: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239631 [details] Patch for landing
Attachment 239631 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2736: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239641 [details] Patch for landing
Created attachment 239642 [details] Patch for landing
Attachment 239642 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2740: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 239642 [details] Patch for landing Rejecting attachment 239642 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 239642, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/4992267878137856
rdar://problem/17290111
Committed r174823: <http://trac.webkit.org/changeset/174823>
Two tests are still failing after this change: https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r174866%20(7482)/results.html
(In reply to comment #19) > Two tests are still failing after this change: > https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/ > r174866%20(7482)/results.html It looks like, for both these tests, 'canplaythrough' events are firing earlier than they were previously. I.e., before the <video> in question has audio or video tracks. I'm still looking into this, but for the meantime, I'll check in these as expected failures.
(In reply to comment #20) > (In reply to comment #19) > > Two tests are still failing after this change: > > https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/ > > r174866%20(7482)/results.html > > It looks like, for both these tests, 'canplaythrough' events are firing > earlier than they were previously. I.e., before the <video> in question has > audio or video tracks. I'm still looking into this, but for the meantime, > I'll check in these as expected failures. Filed <https://bugs.webkit.org/show_bug.cgi?id=137882>