Add AVKit fullscreen video interface.
Created attachment 220941 [details] Patch
Comment on attachment 220941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220941&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42 > +@property (assign) WebView *webView; WebCore is not allowed to know about WebView's existence. If you need the host view, you should use some abstraction.
Comment on attachment 220941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220941&action=review r=me, with some nits. Go ahead and upload a new patch with a cq? flag and someone will cq+ it. > Source/WebCore/html/HTMLMediaElement.cpp:4916 > + if (document().settings() && document().settings()->mediaPlaybackFullscreenAVKit()) { > + if (m_platformLayerBorrowed) I think the if (m_platformLayerBorrowed) check here is sufficient. >> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42 >> +@property (assign) WebView *webView; > > WebCore is not allowed to know about WebView's existence. If you need the host view, you should use some abstraction. It looks like this is dead code. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:131 > + _mediaElement.get()->exitFullscreen(); RefPtr has an operator->, so no need to call .get() here. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:140 > + _mediaElement.get()->play(); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:148 > + _mediaElement.get()->pause(); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:156 > + _mediaElement.get()->togglePlayState(); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:171 > + _mediaElement.get()->play(); > + else > + _mediaElement.get()->pause(); Double ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:183 > + ExceptionCode exceptionCode {}; ExceptionCode is an integer, so it can be initialized with `= 0`; > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:184 > + _mediaElement.get()->setCurrentTime(time, exceptionCode); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:190 > + NSTimeInterval anchorTimeStamp = ([self rate] == 0.f) ? NAN : [classAVValueTiming currentTimeStamp]; No need for the `.f`. In fact, that conditional should be `![self rate]`. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:192 > + anchorTimeStamp:anchorTimeStamp rate:0]; Weird indenting here. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251 > + self.playerLayer = (CALayer<AVPlayerLayer> *)_mediaElement.get()->borrowPlatformLayer(); Again, no need for .get(). This should have something like: ASSERT([self.playerLayer isKindOfClass:classAVPlayerLayer]); To ensure that we catch if anyone starts returning an unexpected layer type here. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:254 > + [self updateTimingWithCurrentTime:_mediaElement.get()->currentTime()]; No need for .get(). > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:257 > + HTMLVideoElement *ve = toHTMLVideoElement(_mediaElement.get()); Please use a more descriptive variable name here, like "videoElement". > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:286 > + [self updateTimingWithCurrentTime:_mediaElement.get()->currentTime()]; No need for .get(). > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:979 > +#if PLATFORM(IOS) > + if (Settings::mediaPlaybackFullscreenAVKit()) > + return isHTMLVideoElement(node); > + else > + return false; > +#else Just to clean things up a little, this could be: #if PLATFORM(IOS) if (!Settings::mediaPlaybackFullscreenAVKit()) return false; #endif return isHTMLVideoElement(node);
(In reply to comment #2) > (From update of attachment 220941 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220941&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42 > > +@property (assign) WebView *webView; > > WebCore is not allowed to know about WebView's existence. If you need the host view, you should use some abstraction. Please don't land with this layering violation in place.
Comment on attachment 220941 [details] Patch Attachment 220941 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/6052748294356992
Comment on attachment 220941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220941&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:247 > + self.loadedTimeRanges = @[ @0, @(duration) ]; We don't always have the entire file loaded. minTimeSeekable() and maxTimeSeekable() are probably what you want. This is probably fine for the initial patch, but please file a bug to fix it and include a "FIXME:" here that references the bug. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:275 > + self.maxTime = duration; Is maxTime the maximum time loaded or the maximum time seekable? In either case, this isn't correct. Please file a bug and reference it here if you aren't going to fix it in this patch. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:280 > + self.loadedTimeRanges = @[ > + @0, > + @(duration) > + ]; Ditto the comment about loadedTimeRanges above. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:305 > + if ((self = [super init])) { if (!(self = [super init])) return nil;
Created attachment 221086 [details] Patch
Created attachment 221087 [details] Patch
Comment on attachment 221087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221087&action=review > Source/WebCore/page/Settings.cpp:101 > +bool Settings::gMediaPlaybackFullscreenAVKit = false; This name could be improved. "gMediaPlayerFullscreenAVKitEnabled"? or just "gAVKitEnabled"? Also, this should propagate across all the settings-related names in this patch. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY This is the wrong license to use. (What's Apple Computer, Inc?) > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:25 > + Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:162 > + return ([self rate] != 0.); Remove parentheses, please. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:183 > + ExceptionCode exceptionCode {}; ExceptionCode ec = 0; > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:184 > + _mediaElement.get()->setCurrentTime(time, exceptionCode); setCurrentTime() takes one parameter here... > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:265 > + Event *event = core(evnt); Event* event. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:280 > + ]; Small enough to combine into one line without losing legibility. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:283 > + || event->type() == eventNames().ratechangeEvent) I believe we indent 4 spaces, and not align the conditionals. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:305 > + if ((self = [super init])) { Would be better to do an early return to get rid of a scope.
Comment on attachment 221087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221087&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:131 > + _mediaElement.get()->exitFullscreen(); A Jer noted in the earlier review, RefPtr has an operator-> so no need to call .get() here. Here and throughout much of the rest of the patch. >> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:162 >> + return ([self rate] != 0.); > > Remove parentheses, please. No need to compare against 0: "return [self rate];" > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:190 > + NSTimeInterval anchorTimeStamp = ([self rate] == 0.f) ? NAN : [classAVValueTiming currentTimeStamp]; No need for the ".f": NSTimeInterval anchorTimeStamp = ![self rate] ? NAN : [classAVValueTiming currentTimeStamp]; > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:247 > + self.loadedTimeRanges = @[ @0, @(duration) ]; This is wrong. Please file a bug about fixing it in a followup patch and include the bug number here in a "FIXME:" comment. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251 > + self.playerLayer = (CALayer<AVPlayerLayer> *)_mediaElement.get()->borrowPlatformLayer(); As Jer noted before: "ASSERT([self.playerLayer isKindOfClass:classAVPlayerLayer])" > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:257 > + HTMLVideoElement *ve = toHTMLVideoElement(_mediaElement.get()); Letters are cheap, don't abbreviate: "HTMLVideoElement *videoElement" > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:279 > + self.loadedTimeRanges = @[ > + @0, > + @(duration) Ditto the comment above about adding a "FIXME:"
Created attachment 221290 [details] Patch
Comment on attachment 221290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221290&action=review > Source/WebCore/html/HTMLMediaElement.cpp:4883 > + if (document().settings() && document().settings()->avKitEnabled()) { This could be replaced with `document().page()->chrome().client().supportsFullscreenForNode(this)`, but this can be done in a follow-up patch. In fact, you could remove the entire #if !PLATFORM(IOS) check, since the default chromeClient will answer true for HTMLMediaElements.
Comment on attachment 221290 [details] Patch Attachment 221290 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4701501385605120
Comment on attachment 221290 [details] Patch Attachment 221290 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4890110579441664
Created attachment 221319 [details] Patch
Comment on attachment 221319 [details] Patch Attachment 221319 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/4749109487468544
Comment on attachment 221319 [details] Patch Attachment 221319 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/5122859152179200
Comment on attachment 221319 [details] Patch Attachment 221319 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5671146624122880
Comment on attachment 221319 [details] Patch Attachment 221319 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5073698956509184
Comment on attachment 221319 [details] Patch Attachment 221319 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/5837646936932352
Created attachment 221437 [details] Patch
Comment on attachment 221437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221437&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:250 > + if (_mediaElement == mediaElement) > + return; You need to stash "mediaElement" in a local RefPtr since you use it twice, dereferencing a PassRefPtr nulls the internal ptr (see http://www.webkit.org/coding/RefPtr.html). > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:258 > + _mediaElement->removeEventListener(eventNames().durationchangeEvent, _listener.get(), false); > + _mediaElement->removeEventListener(eventNames().pauseEvent, _listener.get(), false); > + _mediaElement->removeEventListener(eventNames().playEvent, _listener.get(), false); > + _mediaElement->removeEventListener(eventNames().ratechangeEvent, _listener.get(), false); > + _mediaElement->removeEventListener(eventNames().timeupdateEvent, _listener.get(), false); Do you need _listener.get(), it isn't necessary in the addEventListener() calls below?
(In reply to comment #22) > (From update of attachment 221437 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221437&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:250 > > + if (_mediaElement == mediaElement) > > + return; > > You need to stash "mediaElement" in a local RefPtr since you use it twice, dereferencing a PassRefPtr nulls the internal ptr (see http://www.webkit.org/coding/RefPtr.html). Good catch. I'll fix this. > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:258 > > + _mediaElement->removeEventListener(eventNames().durationchangeEvent, _listener.get(), false); > > + _mediaElement->removeEventListener(eventNames().pauseEvent, _listener.get(), false); > > + _mediaElement->removeEventListener(eventNames().playEvent, _listener.get(), false); > > + _mediaElement->removeEventListener(eventNames().ratechangeEvent, _listener.get(), false); > > + _mediaElement->removeEventListener(eventNames().timeupdateEvent, _listener.get(), false); > > Do you need _listener.get(), it isn't necessary in the addEventListener() calls below? addEventListener takes a PassRefPtr, removeEventListener takes a naked pointer. Maybe so removes can happen during destruction?
Comment on attachment 221437 [details] Patch r=me. Looks like you've got all the bots passing, so whenever you're ready, just mark it as cq? and I'll cq+ it.
(In reply to comment #24) > (From update of attachment 221437 [details]) > r=me. Looks like you've got all the bots passing, so whenever you're ready, just mark it as cq? and I'll cq+ it. Or you can put up another patch to address Eric's comments, but add the "Reviewed by Jer Noble." string to the ChangeLog, and pass the '-no-obselete' flag to webkit-patch.
Created attachment 221504 [details] Patch
Comment on attachment 221504 [details] Patch Clearing flags on attachment: 221504 Committed r162379: <http://trac.webkit.org/changeset/162379>
All reviewed patches have been landed. Closing bug.