Bug 135525 - Support both window and view based video fullscreen.
Summary: Support both window and view based video fullscreen.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-08-01 15:45 PDT by Jeremy Jones
Modified: 2014-12-03 11:21 PST (History)
8 users (show)

See Also:

Attachments
Patch (18.22 KB, patch)
2014-08-01 16:16 PDT, Jeremy Jones 		simon.fraser: review+
Details | Formatted Diff | Diff
Patch for landing. (19.92 KB, patch)
2014-08-02 16:21 PDT, Jeremy Jones 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2014-08-01 15:45:29 PDT 
Support both window and view based video fullscreen.
Comment 1 Jeremy Jones 2014-08-01 15:47:39 PDT 
<rdar://problem/17834694>
Comment 2 Jeremy Jones 2014-08-01 16:16:40 PDT 
Created attachment 235921 [details]
Patch
Comment 3 WebKit Commit Bot 2014-08-01 16:18:03 PDT 
Attachment 235921 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:913:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Simon Fraser (smfr) 2014-08-02 09:12:24 PDT 
Comment on attachment 235921 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:67
>      RetainPtr<UIViewController> m_viewController;
> +    RetainPtr<UIWindow> m_window;
> +    RetainPtr<UIView> m_parentView;

Please add a comment or two here explaining which of these are used in which cases.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:750
> +        if (![bundleIdentifier isEqualToString:@"com.apple.AdSheetPhone"]) {

You should not bury this application check here. Please add it to RuntimeApplicationCheckIOS.mm and use !applicationIsAdSheet() here. I think it's worth referencing a radar in a comment too.
Comment 5 Eric Carlson 2014-08-02 09:37:06 PDT 
Comment on attachment 235921 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:857
> +        [m_window setHidden:YES];
> +        [m_window setRootViewController:nil];

Nit: you nil-check m_window below so you might as well put these in that block.
Comment 6 Jeremy Jones 2014-08-02 16:21:52 PDT 
Created attachment 235941 [details]
Patch for landing.
Comment 7 Jeremy Jones 2014-08-02 16:22:14 PDT 
(In reply to comment #5)
> (From update of attachment 235921 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235921&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:857
> > +        [m_window setHidden:YES];
> > +        [m_window setRootViewController:nil];
> 
> Nit: you nil-check m_window below so you might as well put these in that block.

Done.
Comment 8 Jeremy Jones 2014-08-02 16:22:49 PDT 
(In reply to comment #4)
> (From update of attachment 235921 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235921&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:67
> >      RetainPtr<UIViewController> m_viewController;
> > +    RetainPtr<UIWindow> m_window;
> > +    RetainPtr<UIView> m_parentView;
> 
> Please add a comment or two here explaining which of these are used in which cases.

Grouped them and added a comment.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:750
> > +        if (![bundleIdentifier isEqualToString:@"com.apple.AdSheetPhone"]) {
> 
> You should not bury this application check here. Please add it to RuntimeApplicationCheckIOS.mm and use !applicationIsAdSheet() here. I think it's worth referencing a radar in a comment too.

Done.
Comment 9 WebKit Commit Bot 2014-08-02 16:24:52 PDT 
Attachment 235941 [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]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 WebKit Commit Bot 2014-08-02 17:03:41 PDT 
Comment on attachment 235941 [details]
Patch for landing.

Clearing flags on attachment: 235941

Committed r171973: <http://trac.webkit.org/changeset/171973>