Implement scan backward and forward in video fullscreen.
Created attachment 230741 [details] Patch
<rdar://16562168>
Comment on attachment 230741 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230741&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:280 > +- (void)scanForward:(id)sender > +{ > + UNUSED_PARAM(sender); > + ASSERT(self.delegate); > + self.delegate->beginScanningForward(); > +} > + > +- (void)beginScanningForward:(id)sender > +{ > + UNUSED_PARAM(sender); > + ASSERT(self.delegate); > + self.delegate->beginScanningForward(); > +} What is the difference between these? Will only one be called? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:311 > +- (void)scanBackward:(id)sender > +{ > + UNUSED_PARAM(sender); > + ASSERT(self.delegate); > + self.delegate->beginScanningBackward(); > +} > + > +- (void)beginScanningBackward:(id)sender > +{ > + UNUSED_PARAM(sender); > + ASSERT(self.delegate); > + self.delegate->beginScanningBackward(); > +} Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:322 > + CMTime minimumTime = getkCMTimeIndefinite(); Nit: this would be easier to understand if you defined a macro above: #define kCMTimeIndefinite getkCMTimeIndefinite > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:327 > + return CMTIME_IS_NUMERIC(minimumTime); I am not sure this makes sense unless "canSeekToBeginning" mean "can seek somewhere", because this will return true if it is possible to seek anywhere. What is the answer returned from this selector used for? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:351 > +- (BOOL)canSeekToEnd > +{ > + CMTime maximumTime = getkCMTimeIndefinite(); > + > + for (NSValue *value in [self seekableTimeRanges]) > + maximumTime = CMTimeMaximum(CMTimeRangeGetEnd([value CMTimeRangeValue]), maximumTime); > + > + return CMTIME_IS_NUMERIC(maximumTime); > +} Ditto.
(In reply to comment #3) > (From update of attachment 230741 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230741&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:280 > > +- (void)scanForward:(id)sender > > +{ > > + UNUSED_PARAM(sender); > > + ASSERT(self.delegate); > > + self.delegate->beginScanningForward(); > > +} > > + > > +- (void)beginScanningForward:(id)sender > > +{ > > + UNUSED_PARAM(sender); > > + ASSERT(self.delegate); > > + self.delegate->beginScanningForward(); > > +} > > What is the difference between these? Will only one be called? I re-read the code. These are not used in conjunction with each-other, but for different ways of scanning. I'll remove -scanForward: > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:311 > > +- (void)scanBackward:(id)sender > > +{ > > + UNUSED_PARAM(sender); > > + ASSERT(self.delegate); > > + self.delegate->beginScanningBackward(); > > +} > > + > > +- (void)beginScanningBackward:(id)sender > > +{ > > + UNUSED_PARAM(sender); > > + ASSERT(self.delegate); > > + self.delegate->beginScanningBackward(); > > +} > > Ditto. Ditto. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:322 > > + CMTime minimumTime = getkCMTimeIndefinite(); > > Nit: this would be easier to understand if you defined a macro above: #define kCMTimeIndefinite getkCMTimeIndefinite Changed in two places. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:327 > > + return CMTIME_IS_NUMERIC(minimumTime); > > I am not sure this makes sense unless "canSeekToBeginning" mean "can seek somewhere", because this will return true if it is possible to seek anywhere. What is the answer returned from this selector used for? It is looking for an answer that works for byte-range, HLS and VOD. "beginning" isn't necessarily defined to mean time zero for HLS and VOD. And file based can always seek to time zero. It is used to enable the seek to beginning button. It will be false when there is no movie loaded, or seekable ranges are not yet known. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:351 > > +- (BOOL)canSeekToEnd > > +{ > > + CMTime maximumTime = getkCMTimeIndefinite(); > > + > > + for (NSValue *value in [self seekableTimeRanges]) > > + maximumTime = CMTimeMaximum(CMTimeRangeGetEnd([value CMTimeRangeValue]), maximumTime); > > + > > + return CMTIME_IS_NUMERIC(maximumTime); > > +} > > Ditto. Ditto.
Created attachment 230848 [details] Patch
Created attachment 230854 [details] Patch
Comment on attachment 230854 [details] Patch These changes look fine to me, but I am not a WK2 reviewer so someone else will have to r+.
Created attachment 230867 [details] Patch
Comment on attachment 230867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230867&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:95 > +@property (readonly) BOOL canScanForward; > +@property (readonly) BOOL canScanBackward; > +@property (readonly) BOOL canSeekToBeginning; > +@property (readonly) BOOL canSeekToEnd; Should these be nonatomic?
(In reply to comment #9) > (From update of attachment 230867 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230867&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:95 > > +@property (readonly) BOOL canScanForward; > > +@property (readonly) BOOL canScanBackward; > > +@property (readonly) BOOL canSeekToBeginning; > > +@property (readonly) BOOL canSeekToEnd; > > Should these be nonatomic? Possibly. But that is a bigger question than this bug. I've filed https://bugs.webkit.org/show_bug.cgi?id=132595 to cover this question.
Comment on attachment 230867 [details] Patch Clearing flags on attachment: 230867 Committed r168341: <http://trac.webkit.org/changeset/168341>
All reviewed patches have been landed. Closing bug.