Summary: | Enable activating optimized fullscreen mode from standard fullscreen mode. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||||||||
Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | iPhone / iPad | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jeremy Jones
2014-12-01 11:09:43 PST
Created attachment 242326 [details]
Patch
Attachment 242326 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:970: 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 242326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242326&action=review r=me, with nits. This will need a 👍 from a WK2 owner before landing. > Source/WebCore/html/HTMLMediaElement.h:395 > - enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized }; > + enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized, VideoFullscreenModeStandardAndOptimized }; Seems like this could be turned into a bitfield: enum { VideoFullscreenModeNone = 0, VideoFullscreenModeStandard = 1 << 0, VideoFullscreenModeOptimized = 1 << 1, }; typedef VideoFullscreenMode uint32_t; > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1044 > + if (m_mode == HTMLMediaElement::VideoFullscreenModeOptimized || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized) > [m_playerViewController stopOptimizedFullscreen]; > - else > + if (m_mode == HTMLMediaElement::VideoFullscreenModeStandard || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized) These if() statements argue for the bitfield approach. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:238 > - return _page->videoFullscreenManager() && _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized; > + return _page->videoFullscreenManager() && (_page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized || _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeStandardAndOptimized); This return statement has become sufficiently complicated so as to justify breaking it up into multiple statements. > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:89 > + void setFullscreenModeChanged(uint32_t videoFullscreenMode); Since this file includes HTMLMediaElement.h, you can just use HTMLMediaElement::VideoFullscreenMode here, rather than uint32_t, with the bitfield change above. Comment on attachment 242326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242326&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:92 > + void setOptimizedActive(bool); This is a confusing name. Is it "set the current optimized state to active" or "set optimized and active" or something else? Maybe setIsOptimized()? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:525 > + NSString* propertyName = NSStringFromSelector(@selector(optimizedFullscreenActive)); Will the property name ever be different from @"optimizedFullscreenActive"? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:531 > + if (_avPlayerViewController) > + [_avPlayerViewController removeObserver:self forKeyPath:propertyName]; > _avPlayerViewController = playerViewController; > + if (_avPlayerViewController) > + [_avPlayerViewController addObserver:self forKeyPath:propertyName options:(NSKeyValueObservingOptionNew | NSKeyValueObservingOptionOld) context:nil]; Messaging nil is safe. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1092 > + if (!active && m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized) > + m_mode = HTMLMediaElement::VideoFullscreenModeStandard; > + else if (active && m_mode == HTMLMediaElement::VideoFullscreenModeStandard) > + m_mode = HTMLMediaElement::VideoFullscreenModeStandardAndOptimized; Bitfields would help here too. (In reply to comment #4) > Comment on attachment 242326 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242326&action=review > > r=me, with nits. This will need a 👍 from a WK2 owner before landing. > > > Source/WebCore/html/HTMLMediaElement.h:395 > > - enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized }; > > + enum VideoFullscreenMode { VideoFullscreenModeNone, VideoFullscreenModeStandard, VideoFullscreenModeOptimized, VideoFullscreenModeStandardAndOptimized }; > > Seems like this could be turned into a bitfield: > > enum { > VideoFullscreenModeNone = 0, > VideoFullscreenModeStandard = 1 << 0, > VideoFullscreenModeOptimized = 1 << 1, > }; > > typedef VideoFullscreenMode uint32_t; done. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1044 > > + if (m_mode == HTMLMediaElement::VideoFullscreenModeOptimized || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized) > > [m_playerViewController stopOptimizedFullscreen]; > > - else > > + if (m_mode == HTMLMediaElement::VideoFullscreenModeStandard || m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized) > > These if() statements argue for the bitfield approach. done. > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:238 > > - return _page->videoFullscreenManager() && _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized; > > + return _page->videoFullscreenManager() && (_page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeOptimized || _page->videoFullscreenManager()->mode() == WebCore::HTMLMediaElement::HTMLMediaElement::VideoFullscreenModeStandardAndOptimized); > > This return statement has become sufficiently complicated so as to justify > breaking it up into multiple statements. done. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.h:89 > > + void setFullscreenModeChanged(uint32_t videoFullscreenMode); > > Since this file includes HTMLMediaElement.h, you can just use > HTMLMediaElement::VideoFullscreenMode here, rather than uint32_t, with the > bitfield change above. done. Also updated WebVideoFullscreenManager.messages.in, and the other use of this enum in WebVideoFullscreenManagerProxy.messages.in (In reply to comment #5) > Comment on attachment 242326 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242326&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.h:92 > > + void setOptimizedActive(bool); > > This is a confusing name. Is it "set the current optimized state to active" > or "set optimized and active" or something else? > > Maybe setIsOptimized()? Changed. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:525 > > + NSString* propertyName = NSStringFromSelector(@selector(optimizedFullscreenActive)); > > Will the property name ever be different from @"optimizedFullscreenActive"? No. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:531 > > + if (_avPlayerViewController) > > + [_avPlayerViewController removeObserver:self forKeyPath:propertyName]; > > _avPlayerViewController = playerViewController; > > + if (_avPlayerViewController) > > + [_avPlayerViewController addObserver:self forKeyPath:propertyName options:(NSKeyValueObservingOptionNew | NSKeyValueObservingOptionOld) context:nil]; > > Messaging nil is safe. Removed conditions. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1092 > > + if (!active && m_mode == HTMLMediaElement::VideoFullscreenModeStandardAndOptimized) > > + m_mode = HTMLMediaElement::VideoFullscreenModeStandard; > > + else if (active && m_mode == HTMLMediaElement::VideoFullscreenModeStandard) > > + m_mode = HTMLMediaElement::VideoFullscreenModeStandardAndOptimized; > > Bitfields would help here too. Done. Created attachment 242880 [details]
Patch for landing.
Created attachment 242884 [details]
Patch for landing.
Created attachment 242885 [details]
Patch for landing.
Attachment 242885 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:971: 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 242885 [details] Patch for landing. Clearing flags on attachment: 242885 Committed r177012: <http://trac.webkit.org/changeset/177012> |