[iOS] Fullscreen -> PiP should resume to Fullscreen, not inline
Created attachment 264883 [details] Patch
Attachment 264883 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1435: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 264901 [details] Patch
Attachment 264901 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1435: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
rdar://problem/20596271
Comment on attachment 264901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264901&action=review This looks fine to me, but a WK2 reviewer will need to give it an official r+. > Source/WebCore/ChangeLog:22 > + Add methods to for clients to request a specific fullscreen mode, and to query whether > + the page is currently visible. Nit: "to for clients"?
Comment on attachment 264901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264901&action=review > Source/WebCore/ChangeLog:8 > + When "restoring" the user interface when exiting PiP, we should return to Fullscreen if that "restoring" reads like butchers quotes. > Source/WebCore/html/HTMLVideoElement.cpp:405 > + LOG(Media, "HTMLVideoElement::webkitSetPresentationMode(%p) - setting to \"%s\"", this, mode.utf8().data()); You could LOG_STREAM(Media, stream << "HTMLVideoElement::webkitSetPresentationMode(" << this << ") - setting to " << mode); > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:657 > if ([_videoSublayer superlayer] != self) > return; > > - [_videoSublayer setPosition:CGPointMake(CGRectGetMidX(bounds), CGRectGetMidY(bounds))]; > - > if (![_avPlayerController delegate]) > return; Would be nice to wrap these checks in a function that describes what state you expect these to be true in. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:838 > + WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer]; Do we know that [pipView layer] is always a WebAVPlayerLayer? Could some other framework break this under us?
(In reply to comment #7) > Comment on attachment 264901 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264901&action=review > > > Source/WebCore/ChangeLog:8 > > + When "restoring" the user interface when exiting PiP, we should return to Fullscreen if that > > "restoring" reads like butchers quotes. Okay, I'll "remove" them. > > Source/WebCore/html/HTMLVideoElement.cpp:405 > > + LOG(Media, "HTMLVideoElement::webkitSetPresentationMode(%p) - setting to \"%s\"", this, mode.utf8().data()); > > You could LOG_STREAM(Media, stream << > "HTMLVideoElement::webkitSetPresentationMode(" << this << ") - setting to " > << mode); Nice! > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:657 > > if ([_videoSublayer superlayer] != self) > > return; > > > > - [_videoSublayer setPosition:CGPointMake(CGRectGetMidX(bounds), CGRectGetMidY(bounds))]; > > - > > if (![_avPlayerController delegate]) > > return; > > Would be nice to wrap these checks in a function that describes what state > you expect these to be true in. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:838 > > + WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer]; > > Do we know that [pipView layer] is always a WebAVPlayerLayer? Could some > other framework break this under us? Yes. The PiP view will always have a WebAVPlayerLayer because WebAVPictureInPicturePlayerLayerView has a -layerClass member which returns [WebAVPlayerLayer class].
Created attachment 266482 [details] Patch for landing
Attachment 266482 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1434: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 266482 [details] Patch for landing Clearing flags on attachment: 266482 Committed r193340: <http://trac.webkit.org/changeset/193340>