[webkit-reviews] review denied: [Bug 126818] Add AVKit fullscreen video interface. : [Attachment 221087] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 13 20:25:56 PST 2014
Eric Carlson <eric.carlson at apple.com> has denied Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 126818: Add AVKit fullscreen video interface.
https://bugs.webkit.org/show_bug.cgi?id=126818
Attachment 221087: Patch
https://bugs.webkit.org/attachment.cgi?id=221087&action=review
------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221087&action=review
> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:131
> + _mediaElement.get()->exitFullscreen();
A Jer noted in the earlier review, RefPtr has an operator-> so no need to call
.get() here.
Here and throughout much of the rest of the patch.
>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:162
>> + return ([self rate] != 0.);
>
> Remove parentheses, please.
No need to compare against 0: "return [self rate];"
> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:190
> + NSTimeInterval anchorTimeStamp = ([self rate] == 0.f) ? NAN :
[classAVValueTiming currentTimeStamp];
No need for the ".f":
NSTimeInterval anchorTimeStamp = ![self rate] ? NAN : [classAVValueTiming
currentTimeStamp];
> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:247
> + self.loadedTimeRanges = @[ @0, @(duration) ];
This is wrong. Please file a bug about fixing it in a followup patch and
include the bug number here in a "FIXME:" comment.
> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251
> + self.playerLayer = (CALayer<AVPlayerLayer>
*)_mediaElement.get()->borrowPlatformLayer();
As Jer noted before: "ASSERT([self.playerLayer
isKindOfClass:classAVPlayerLayer])"
> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:257
> + HTMLVideoElement *ve = toHTMLVideoElement(_mediaElement.get());
Letters are cheap, don't abbreviate: "HTMLVideoElement *videoElement"
> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:279
> + self.loadedTimeRanges = @[
> + @0,
> + @(duration)
Ditto the comment above about adding a "FIXME:"
More information about the webkit-reviews
mailing list