Bug 55901

Summary: [Qt] Fix the error code for media resource failures when using QtMobility
Product: WebKit Reporter: Yi Shen <max.hong.shen>
Component: MediaAssignee: Yi Shen <max.hong.shen>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, cmarcelo, hui_huang, inferno, joel.parks, menard, nancy.piedra, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 50925    
Attachments:
Description Flags
proposed fix
hausmann: review+, hausmann: commit-queue-
fix the patch according to the qtmobility change
hausmann: review+, hausmann: commit-queue-
fix typo none

Description Yi Shen 2011-03-07 13:40:01 PST
According to Html 5 video spec, when the media resource failed to load or that the given URL could not be resolved, the media element should adjust its status as following,

      1. Set the error attribute to a new MediaError object whose code attribute is set to MEDIA_ERR_SRC_NOT_SUPPORTED.
      2. Set the element's networkState attribute to the NETWORK_NO_SOURCE value.

Currently, qMediaPlayer returns QMediaPlayer::InvalidMedia for media resource failures, instead of setting m_networkState to NetworkError, we should use MediaPlayer::FormatError to indicate the resource error.
Comment 1 Yi Shen 2011-03-07 13:49:08 PST
Created attachment 84977 [details]
proposed fix
Comment 2 Alexis Menard (darktears) 2011-03-11 06:43:42 PST
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?
Comment 3 Yi Shen 2011-03-11 06:59:04 PST
(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 :)
Comment 4 Alexis Menard (darktears) 2011-03-21 12:12:54 PDT
(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
Comment 5 Alexis Menard (darktears) 2011-03-29 14:34:03 PDT
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 :).
Comment 6 Yi Shen 2011-04-01 06:36:58 PDT
(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!
Comment 7 Laszlo Gombos 2011-04-18 16:53:05 PDT
*** Bug 40155 has been marked as a duplicate of this bug. ***
Comment 8 Joel Parks 2011-04-26 11:32:10 PDT
needed for BR-7521
Comment 9 Simon Hausmann 2011-04-26 15:38:57 PDT
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 10 Simon Hausmann 2011-04-26 15:42:41 PDT
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 :)
Comment 11 Yi Shen 2011-04-26 16:47:35 PDT
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 :)
Comment 12 Yi Shen 2011-04-27 09:21:38 PDT
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 13 Alexis Menard (darktears) 2011-05-23 14:35:07 PDT
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 14 Simon Hausmann 2011-05-26 10:14:59 PDT
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 :)
Comment 15 Yi Shen 2011-05-31 14:10:26 PDT
(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.
Comment 16 Yi Shen 2011-06-07 17:15:32 PDT
Created attachment 96331 [details]
fix typo
Comment 17 WebKit Review Bot 2011-06-07 19:44:47 PDT
Comment on attachment 96331 [details]
fix typo

Clearing flags on attachment: 96331

Committed r88318: <http://trac.webkit.org/changeset/88318>
Comment 18 WebKit Review Bot 2011-06-07 19:44:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Ademar Reis 2011-06-08 08:48:08 PDT
Revision r88318 cherry-picked into qtwebkit-2.2 with commit f1e20a4 <http://gitorious.org/webkit/qtwebkit/commit/f1e20a4>