[webkit-reviews] review denied: [Bug 103770] [Win] support in-band text tracks : [Attachment 206830] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 103770: [Win] support in-band text tracks
https://bugs.webkit.org/show_bug.cgi?id=103770

Attachment 206830: Patch
https://bugs.webkit.org/attachment.cgi?id=206830&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=206830&action=review


>>
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp
:148
>> +	    // If possible, return a title in one of the user's preferred
languages.
> 
> I see that you are just following the pattern (that I created) in
InbandTextTrackPrivateAVFObjC::label(), but you can use an early return here.

Done!

>>
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundation
CF.cpp:208
>> +	    // AVCFAssetPropertyPreferredVolume,    // FIXME: Should be
exported by AVCF. We can use AVCFAssetTrackPropertyPreferredVolume for now.
> 
> This can be removed because neither is currently used.

Done!

>>
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundation
CF.cpp:535
>> +	    return max(narrowPrecisionToFloat(CMTimeGetSeconds(itemTime)),
0.0f);
> 
> Nit: is the "f" necessary, does "0" cause a compiler warning/error?

Yes -- the compiler can't (won't?) assume a 0.0 (or 0) argument is a float to
match the return from narrowPrecisionToFloat.  I could write this as
max<float>, or just use the 0.0f notation.

>>
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundation
CF.cpp:797
>> +	ASSERT(CFArrayGetTypeID() == CFGetTypeID(foo)); 
> 
> Won't this cause an unused variable error in a release build? If it is
necessary, I'll bet you can come up with a more descriptive name than "foo" ;-)


Doh!  Stupid debugging code.  It was worse than that -- it was an unreleased
variable each time this was called.

>>
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundation
CF.cpp:1549
>> +
> 
> It might be worth adding something like "ASSERT(legibleOutput == 
self->legibleOutput())"

Good idea.