Bug 55901

Created attachment 84977 [details] proposed fix

Comment on attachment 84977 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535 > + else if (m_networkState != MediaPlayer::FormatError) Not sure to get that change...Could you explain me?

(In reply to comment #2) > (From update of attachment 84977 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review > > > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535 > > + else if (m_networkState != MediaPlayer::FormatError) > > 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 :)

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 84977 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review > > > > > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535 > > > + else if (m_networkState != MediaPlayer::FormatError) > > > > 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

Could you please update the patch with your second suggestion and a little comment on the condition on why we need this extra check, basically what you wrote to me :).

(In reply to comment #5) > Could you please update the patch with your second suggestion and a little comment on the condition on why we need this extra check, basically what you wrote to me :). Thanks for reminding me, Alexis :) It has been a while, so I may need to check/test the patch carefully with the latest qt-mobility. I will provide a new patch once I finish the task on my hand. Thanks!

*** Bug 40155 has been marked as a duplicate of this bug. ***

needed for BR-7521

Comment on attachment 84977 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review > Source/WebCore/ChangeLog:8 > + To indicate the media resource failures, set the netwrok state to netwrok -> network

Comment on attachment 84977 [details] proposed fix 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 :)

Thanks for the review, Simon. I think I need to double check the patch with the QtMobility 1.2 before landing the patch. Will do it tomorrow and put comments here. (In reply to comment #10) > (From update of attachment 84977 [details]) > 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 :)

Created attachment 91298 [details] fix the patch according to the qtmobility change With the QtMobility 1.2.0, it can send out QMediaPlayer::ResourceError after QMediaPlayer::InvalidMedia for resource issue. So I made a little bit change for it in this new patch. Also, I noticed that all the media tests are disabled in https://bugs.webkit.org/show_bug.cgi?id=57983, so I just tested the related layouttest(including tests under LayoutTest/media and LayoutTest/fast/media) on my environment.

Comment on attachment 91298 [details] fix the patch according to the qtmobility change I'm fine with the change. The only hic is that we will enforce the usage of QtMM 1.2, but I'm also fine with that.

Comment on attachment 91298 [details] fix the patch according to the qtmobility change View in context: https://bugs.webkit.org/attachment.cgi?id=91298&action=review r=me > Source/WebCore/ChangeLog:8 > + To indicate the media resource failures, set the netwrok state to Please fix the netwrok -> network typo before landing :)

(In reply to comment #14) > (From update of attachment 91298 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91298&action=review > > r=me > > > Source/WebCore/ChangeLog:8 > > + To indicate the media resource failures, set the netwrok state to > > Please fix the netwrok -> network typo before landing :) Thanks for your review, Simon :) I will update/test it ASAP.

Created attachment 96331 [details] fix typo

Comment on attachment 96331 [details] fix typo Clearing flags on attachment: 96331 Committed r88318: <http://trac.webkit.org/changeset/88318>

All reviewed patches have been landed. Closing bug.

Revision r88318 cherry-picked into qtwebkit-2.2 with commit f1e20a4 <http://gitorious.org/webkit/qtwebkit/commit/f1e20a4>