Add support for playbackControlsManager rdar://problem/23833753
Created attachment 272360 [details] Patch
Attachment 272360 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:35: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:36: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:68: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:97: Tab found; better to use spaces [whitespace/tab] [1] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:102: Tab found; better to use spaces [whitespace/tab] [1] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:112: Tab found; better to use spaces [whitespace/tab] [1] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:117: Tab found; better to use spaces [whitespace/tab] [1] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:130: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 8 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 272362 [details] Patch
Attachment 272362 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:35: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:39: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:69: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:131: *SoftLink.h header should be included after all other headers. [build/include_order] [4] Total errors found: 4 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 272362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272362&action=review > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:289 > + uint64_t contextId; > + > + auto addResult = m_videoElements.add(&videoElement, 0); > + if (addResult.isNewEntry) > + addResult.iterator->value = nextContextId(); > + contextId = addResult.iterator->value; There is new fangled way to do this using HashMap::ensure(..) uint64_t contextId = m_videoElements.add(&videoElement, [&] { return nextContextId(); }).iterator->value; or in two lines: auto addResult = m_videoElements.add(&videoElement, [&] { return nextContextId(); }); auto contextId = addResult.iterator->value;
Comment on attachment 272360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272360&action=review > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:179 > + controlsManager.hasEnabledAudio = YES; > + controlsManager.hasEnabledVideo = YES; This isn't necessarily correct, should it have a FIXME? > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:209 > + double start = timeRanges.start(i, exceptionCode); > + double end = timeRanges.end(i, exceptionCode); Can you use ASSERT_NO_EXCEPTION instead of ignoring potential exceptions? > Source/WebKit2/UIProcess/mac/PageClientImpl.h:217 > + virtual void isPlayingMediaDidChange() override; Nit: "virtual" is implied with "override".
Comment on attachment 272362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272362&action=review > Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:213 > + double start = timeRanges.start(i, exceptionCode); > + double end = timeRanges.end(i, exceptionCode); > + > + CMTimeRange range = CMTimeRangeMake(CMTimeMakeWithSeconds(start, 1000), CMTimeMakeWithSeconds(end-start, 1000)); > + [seekableRanges addObject:[NSValue valueWithCMTimeRange:range]]; You can use timeRanges.ranges() to get a PlatformTimeRanges. This object's start() and end() methods don't take an exception code, and they return MediaTime objects. So this would become: const PlatformTimeRanges& ranges = timeRanges.ranges(); CMTimeRange range = CMTimeRangeMake(toCMTime(ranges.start(i)), toCMTime(ranges.end(i))); > Source/WebCore/platform/spi/mac/AVFoundationSPI.h:35 > +#import <AVFoundation/AVTime.h> AVTime.h should be available in the SDK.
Thanks all! I believe that I addressed all comments: http://trac.webkit.org/changeset/197204
Build fix: http://trac.webkit.org/changeset/197207
More build fix: http://trac.webkit.org/changeset/197211
This caused multiple crashes, will roll out. More info in e-mail. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x00000001073e4762 WebKit::WebVideoFullscreenManagerProxy::ensureModelAndInterface(unsigned long long) + 180 1 com.apple.WebKit 0x00000001073e50a5 WebKit::WebVideoFullscreenManagerProxy::controlsManagerInterface() + 13 2 com.apple.WebKit 0x00000001073e9e5c WebKit::WebViewImpl::updateWebViewImplAdditions() + 310 3 com.apple.WebKit 0x00000001073730c5 WebKit::WebPageProxy::isPlayingMediaDidChange(unsigned int, unsigned long long) + 51 4 com.apple.WebKit 0x000000010738672d WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) + 12389
Re-opened since this is blocked by bug 154780
Created attachment 272680 [details] Patch that attempts to resolve crashes
Attachment 272680 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:34: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:35: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:38: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:40: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:78: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:83: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:84: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:85: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:86: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:87: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:88: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:89: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:148: *SoftLink.h header should be included after all other headers. [build/include_order] [4] ERROR: Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:298: More than one command on the same line [whitespace/newline] [4] Total errors found: 14 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks Jer! Giving this another go: http://trac.webkit.org/changeset/197461
This caused some new crashes, which I addressed with http://trac.webkit.org/changeset/197469