[webkit-reviews] review denied: [Bug 235174] Setting undefined to video.playbackRate causes Safari to crash : [Attachment 449567] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Takeshi Sone
<takeshi.sone at gmail.com>'s request for review:
Bug 235174: Setting undefined to video.playbackRate causes Safari to crash
https://bugs.webkit.org/show_bug.cgi?id=235174

Attachment 449567: Patch

https://bugs.webkit.org/attachment.cgi?id=449567&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 449567
  --> https://bugs.webkit.org/attachment.cgi?id=449567
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449567&action=review

>>> Source/WebCore/html/HTMLMediaElement.cpp:3498
>>> +
>> 
>> The spec defines `defaultPlaybackRate` and `playbackRate` as `double`:
>> 
>>   attribute double defaultPlaybackRate;
>>   attribute double playbackRate;
>> 
>> but HTMLMediaElement.idl uses `unrestricted double`:
>> 
>>     attribute unrestricted double defaultPlaybackRate;
>>     attribute unrestricted double playbackRate;
>> 
>> I wonder if the changes to setDefaultPlaybackRate and setPlaybackRate would
be unnecessary if this was fixed?
> 
> Eric is completely right. if our IDL matched the spec, it wouldn't be
possible for the implementation to be called with infinite.

Another way to put this is: The correct fix would be to change the .idl, not to
change the .cpp file. I suspect we won’t need any changes to
WebAVPlayerController.mm either. Please pursue that solution.

> Source/WebCore/platform/ios/WebAVPlayerController.mm:201
> +    /* NaN == NaN is false.
> +	* Catch that case to prevent infinite mutual recursion. */

Incorrect coding style. Please follow WebKit coding style, using "//" comments.