[webkit-reviews] review granted: [Bug 43673] The preload attribute of the video tag is not completely implemented : [Attachment 92129] Updated patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 43673: The preload attribute of the video tag is not completely implemented
https://bugs.webkit.org/show_bug.cgi?id=43673

Attachment 92129: Updated patch
https://bugs.webkit.org/attachment.cgi?id=92129&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=92129&action=review

It might have been possible to break this patch up into slightly smaller
pieces, though I'm sure you tried to break it up already.

Why does only MediaPlayerPrivateAVFoundation need all this work, and not other
MediaPlayerPrivate implementations?

> Source/WebCore/ChangeLog:9
> +2011-05-02  Eric Carlson  <eric.carlson at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   The preload attribute of the video tag is not completely implemented

> +	   https://bugs.webkit.org/show_bug.cgi?id=43673
> +	   <rdar://problem/7508322>
> +
> +	   Tested manually with manual-tests/media-elements/video-preload.html.


It would be nice if this explained a little more what parts of "preload" aren't
implemented, and what parts this patch implements.

> Source/WebCore/ChangeLog:15
> +	   (WebCore::HTMLMediaElement::seek): Call prepareToPlay when preload
is less than 'metadata'

Do you mean "less than 'auto'"?

> Source/WebCore/manual-tests/media-elements/video-preload.html:100
> -	   function setURL(url, vidID)
> +	   function setURL(url)
>	   {
> -	       var vid = document.getElementById(vidID);
> +	       var vid = document.getElementById("vid");

None of the changes in this file are mentioned in your ChangeLog.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:245
> +    LOG(Media, "MediaPlayerPrivateAVFMac::duration(%p) - caching %f", this,
m_cachedDuration);

Not sure what the "Mac" is about here.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:395
> -	   AVAssetStatus avAssetStatus = assetStatus();
> +	   AssetStatus avAssetStatus = assetStatus();

Maybe this should just be called assetStatus now? (You can use
this->assetStatus() on the right-hand side.)

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:437
>		   case MediaPlayerAVPlayerItemStatusPlaybackBufferFull:
> +		       m_networkState = MediaPlayer::Idle;
> +
> +		   case MediaPlayerAVPlayerItemStatusReadyToPlay:

Did you intentionally leave out a "break" here?

Maybe it would be clearer to put the m_networkState logic after the switch.
That way the switch can be all about m_readyState, and the code just after the
switch (which is already dealing with m_networkState) can be solely in charge
of m_networkState.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:461
> +	   if (m_readyState < MediaPlayer::HaveCurrentData)
> +	       m_readyState = MediaPlayer::HaveCurrentData;

You could use max() here, but I'm not sure if it would be clearer.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:619
> +	       // AVFoundation can't open arbitrary data pointers, so if this
ApplicationCacheResource doesn't 
> +	       // have a valid local path, just open the resource's original
URL.
> +	       if (resource->path().isEmpty())
> +		   createAVAssetForURL(resource->url());
> +	       else
> +		   createAVAssetForCacheResource(resource);

That sounds like a deficiency of AVFoundation. Maybe we should file a bug and
reference it here with a FIXME?

I think it was clearer when this logic was inside
createAVAssetForCacheResource. Why move it here?

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:269
> +    ASSERT(!resource->path().isEmpty());

This could be ASSERT_ARG.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:295
> +    const double veryLongInterval = 60*60*60*24*30;

1800 days? I think this would be a little clearer if written like this: 60 * 60
* 24 * 1800

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:315
> +    // Create the player item so we can media data. 

Typo: so we can media data

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:318
> +    [[NSNotificationCenter defaultCenter]
addObserver:m_objcObserver.get()selector:@selector(didEnd:)
name:AVPlayerItemDidPlayToEndTimeNotification object:m_avPlayerItem.get()];

Missing a space before "selector:".

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:419
> +    // Check the avitem if we have one, some assets never report duration.

Maybe "AVItem"?

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:432
> +    LOG(Media, "MediaPlayerPrivateAVFoundationObjC::duration(%p) - invalid
duration, returning -1", this);
> +    return invalidTime();

Maybe you should use invalidTime() in the LOG, too, so they can't get out of
sync if we change the definition of invalidTime().

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:722
> +	   for (AVPlayerItemTrack *track in tracks) {
> +	       if ([track isEnabled]) {
> +		   AVAssetTrack *assetTrack = [track assetTrack];
> +		   if ([[assetTrack mediaType]
isEqualToString:AVMediaTypeVideo])
> +		       hasVideo = true;
> +		   else if ([[assetTrack mediaType]
isEqualToString:AVMediaTypeAudio])
> +		       hasAudio = true;
> +		   else if ([[assetTrack mediaType]
isEqualToString:AVMediaTypeClosedCaption])
> +		       hasCaptions = true;
> +	       }
>	   }

We could break out of this loop early once hasVideo && hasAudio && hasCaptions
is true.