Bug 132517 – Implement scan backward and forward in video fullscreen.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Implement scan backward and forward in video fullscreen.

[Jeremy Jones]

Created attachment 230741 [details]
Patch

[Eric Carlson]

<rdar://16562168>

[Eric Carlson]

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.

[Jeremy Jones]

(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.

[Jeremy Jones]

Created attachment 230848 [details]
Patch

[Jeremy Jones]

Created attachment 230854 [details]
Patch

[Eric Carlson]

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+.

[Jeremy Jones]

Created attachment 230867 [details]
Patch

[Simon Fraser (smfr)]

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?

[Jeremy Jones]

(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.

[WebKit Commit Bot]

Comment on attachment 230867 [details]
Patch

Clearing flags on attachment: 230867

Committed r168341: <http://trac.webkit.org/changeset/168341>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.