Bug 132517

Summary: Implement scan backward and forward in video fullscreen.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: WebKit2Assignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, glenn, jer.noble, philipj, sergio, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Jeremy Jones 2014-05-02 21:54:40 PDT
Implement scan backward and forward in video fullscreen.
Comment 1 Jeremy Jones 2014-05-02 22:09:43 PDT
Created attachment 230741 [details]
Patch
Comment 2 Eric Carlson 2014-05-05 09:16:48 PDT
<rdar://16562168>
Comment 3 Eric Carlson 2014-05-05 10:49:29 PDT
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.
Comment 4 Jeremy Jones 2014-05-05 12:50:34 PDT
(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.
Comment 5 Jeremy Jones 2014-05-05 12:52:30 PDT
Created attachment 230848 [details]
Patch
Comment 6 Jeremy Jones 2014-05-05 13:37:22 PDT
Created attachment 230854 [details]
Patch
Comment 7 Eric Carlson 2014-05-05 14:05:24 PDT
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+.
Comment 8 Jeremy Jones 2014-05-05 16:55:03 PDT
Created attachment 230867 [details]
Patch
Comment 9 Simon Fraser (smfr) 2014-05-05 18:09:30 PDT
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?
Comment 10 Jeremy Jones 2014-05-05 18:18:04 PDT
(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 11 WebKit Commit Bot 2014-05-05 18:50:01 PDT
Comment on attachment 230867 [details]
Patch

Clearing flags on attachment: 230867

Committed r168341: <http://trac.webkit.org/changeset/168341>
Comment 12 WebKit Commit Bot 2014-05-05 18:50:05 PDT
All reviewed patches have been landed.  Closing bug.