[webkit-reviews] review granted: [Bug 55901] [Qt] Fix the error code for media resource failures when using QtMobility : [Attachment 84977] proposed fix

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted yi shen
<yi.4.shen at nokia.com>'s request for review:
Bug 55901: [Qt] Fix the error code for media resource failures when using
QtMobility
https://bugs.webkit.org/show_bug.cgi?id=55901

Attachment 84977: proposed fix
https://bugs.webkit.org/attachment.cgi?id=84977&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review

Please fix these issues before landing.

>>>> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535

>>> 
>>> Not sure to get that change...Could you explain me?
>> 
>> That's because Qt Mobility sends out a NetworkError after InvalidMedia
(Note, InvalidMedia is not a kind of error for Qt Mobility). I think it makes
sense for Qt Mobility, but for MediaPlayerPrivateQt, it should treat this
special NetworkError (the one follow the InvalidMedia) as a FormatError.
>> 
>> Maybe following code is more clear about this idea, although it causes an
unnecessary assignment.
>> 
>>  -	     if (currentError == QMediaPlayer::FormatError)
>>  +	     if (currentError == QMediaPlayer::FormatError || (m_networkState
== QMediaPlayer::FormatError && currentError == QMediaPlayer::NetworkError))
>>		m_networkState = MediaPlayer::FormatError;
>>	    else
>>		m_networkState = MediaPlayer::NetworkError;
>> 
>> What do you think? Alexis :)
> 
> Well the assignment is no big deal as soon as it does not send an extra
notification. But at least the code is clearer to read

I also prefer the more readable code. Let's go for that. The rest looks okay to
me :)