Bug 139465

Summary: Restore interface state when stopping optimized fullscreen.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: MediaAssignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ddkilzer, eric.carlson, glenn, jer.noble, jonlee, mitz, philipj, sam, sergio, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+, jeremyj-wk: commit-queue-

Description Jeremy Jones 2014-12-09 16:38:08 PST
Restore interface state when stopping optimized fullscreen.
Comment 1 Jeremy Jones 2014-12-09 16:41:10 PST
rdar://problem/18878722
Comment 2 Jeremy Jones 2014-12-09 16:56:06 PST
Created attachment 242976 [details]
Patch
Comment 3 WebKit Commit Bot 2014-12-09 16:59:03 PST
Attachment 242976 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:910:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:90:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Sam Weinig 2014-12-09 21:41:11 PST
WebKit doesn't really have any concept of tab, so it is weird to be adding it here. Can you explain why this is necessary?
Comment 5 mitz 2014-12-09 22:25:35 PST
Comment on attachment 242976 [details]
Patch

Tabs aren’t a WebKit concept so at least the terminology in most of this patch is wrong. The new method seems similar to -webViewFocus: in the legacy WebUIDelegate, but the boolean parameter suggests that it’s different. But it seems strange to have an “animate” parameter to this delegate method. The delegate can implement a UI where animation doesn’t make sense, or everything is always animated. So perhaps the right delegate method for this should be specifically about the optimized full screen video scenario and just tell the delegate what happened (some playback mode was exited in some way).
Comment 6 Jeremy Jones 2014-12-10 16:00:57 PST
(In reply to comment #4)
> WebKit doesn't really have any concept of tab, so it is weird to be adding
> it here. Can you explain why this is necessary?

I guess "tab" is too concrete. It is about making sure the page that optimized fullscreen animates back into is visible.
Comment 7 Jeremy Jones 2014-12-10 16:02:06 PST
(In reply to comment #5)
> Comment on attachment 242976 [details]
> Patch
> 
> Tabs aren’t a WebKit concept so at least the terminology in most of this
> patch is wrong. The new method seems similar to -webViewFocus: in the legacy
> WebUIDelegate, but the boolean parameter suggests that it’s different. But
> it seems strange to have an “animate” parameter to this delegate method. The
> delegate can implement a UI where animation doesn’t make sense, or
> everything is always animated. So perhaps the right delegate method for this
> should be specifically about the optimized full screen video scenario and
> just tell the delegate what happened (some playback mode was exited in some
> way).

How about -fullscreenMayReturnToInline which describes the event that is happening. Which is basically saying that fullscreen may return to inline at any time, so get the interface ready for that.
Comment 8 Jeremy Jones 2014-12-10 16:02:59 PST
Created attachment 243076 [details]
Patch
Comment 9 Simon Fraser (smfr) 2014-12-10 17:24:18 PST
Comment on attachment 243076 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Implement fullscreenMayReturnToInline() to request the client application show the corresponding tab.

I think the "may" is a a bit vague and confusing for clients of the API, but we can fix that later.
Comment 10 Jon Lee 2014-12-11 00:40:49 PST
The commit queue is reflecting no patches left to merge in-- it looks stuck. Can we get this checked in directly?
Comment 11 Jeremy Jones 2014-12-11 09:53:34 PST
(In reply to comment #10)
> The commit queue is reflecting no patches left to merge in-- it looks stuck.
> Can we get this checked in directly?

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm
	M	Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h
	M	Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/UIProcess/API/APIUIClient.h
	M	Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h
	M	Source/WebKit2/UIProcess/Cocoa/UIDelegate.h
	M	Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm
	M	Source/WebKit2/UIProcess/WebPageProxy.cpp
	M	Source/WebKit2/UIProcess/WebPageProxy.h
	M	Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h
	M	Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm
Committed r177154