Bug 124715

Summary: [GStreamer] Invalid command line error when visiting www.chessbase.com
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebKitGTKAssignee: Xabier Rodríguez Calvar <calvaris>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, eric.carlson, glenn, gustavo, jer.noble, menard, mrobinson, pnormand, svillar, vjaquez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.chessbase.com
Bug Depends on: 85994    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Sergio Villar Senin 2013-11-21 07:08:12 PST
Whenever I visit that page I get a popup window with this error:

"gstreamer|1.0|WebKitWebProcess|decodificador text/html|decoder-text/html"

because apparently there is a wrong parameter.
Comment 1 Xabier Rodríguez Calvar 2013-11-24 09:46:29 PST
Created attachment 217763 [details]
Patch

HTTP errors were not being handled at the GStreamer WebKit source
Comment 2 Philippe Normand 2013-11-24 23:07:58 PST
Comment on attachment 217763 [details]
Patch

Hum, please talk to Andres, he has a patch for this code too.
Comment 3 Xabier Rodríguez Calvar 2013-11-25 00:51:03 PST
Comment on attachment 217763 [details]
Patch

There's a problem with the comment.
Comment 4 Xabier Rodríguez Calvar 2013-11-25 09:25:32 PST
Created attachment 217814 [details]
Patch

Rebased
Comment 5 Sergio Villar Senin 2013-11-25 09:37:17 PST
Comment on attachment 217814 [details]
Patch

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

I guess this won't be accepted without a test case.

> Source/WebCore/ChangeLog:5
> +

You used an incorrect bug number to create this patch

> Source/WebCore/ChangeLog:7
> +

Short explanation missing here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:847
> +

Don't know a lot about GStreamer but shouldn't this be protected by the locker?

Also looks like the error handling code is exactly the same as the one bellow that handles invald partial respones (!= 206). Could it be possible to refactor it in some function or macro?
Comment 6 Xabier Rodríguez Calvar 2013-11-25 09:59:52 PST
(In reply to comment #5)
> (From update of attachment 217814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217814&action=review
> 
> I guess this won't be accepted without a test case.

It is very port specific, writing a test for this and flagging it everywhere that is not gstreamer port feels wrong, specially because this is not a new feature.

> > Source/WebCore/ChangeLog:5
> > +
> 
> You used an incorrect bug number to create this patch

Roger.

> > Source/WebCore/ChangeLog:7
> > +
> 
> Short explanation missing here.

Roger.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:847
> > +
> 
> Don't know a lot about GStreamer but shouldn't this be protected by the locker?

Well, I considered that, but we would lock, check the response code and then unlock. I don't think it is necessary, though maybe Phil thinks otherwise.

> Also looks like the error handling code is exactly the same as the one bellow that handles invald partial respones (!= 206). Could it be possible to refactor it in some function or macro?

I though about it but as we have that locker, that I didn't have the intention to use, that is a stack variable, I would have to pass it and I though it wasn't worth, or writing a macro.
Comment 7 Xabier Rodríguez Calvar 2013-11-26 09:29:10 PST
Created attachment 217882 [details]
Patch

Added test. It doesn't fail either because it doesn't play either though for a different reason.
Comment 8 Xabier Rodríguez Calvar 2013-11-26 09:31:03 PST
(In reply to comment #6)
> > I guess this won't be accepted without a test case.
> 
> It is very port specific, writing a test for this and flagging it everywhere that is not gstreamer port feels wrong, specially because this is not a new feature.

Now that I think about it, I could use the internals to know the reason of the failure and check if it is the right one, but it seems overkilling to me.
Comment 9 WebKit Commit Bot 2013-11-27 02:45:18 PST
Comment on attachment 217882 [details]
Patch

Clearing flags on attachment: 217882

Committed r159810: <http://trac.webkit.org/changeset/159810>
Comment 10 WebKit Commit Bot 2013-11-27 02:45:21 PST
All reviewed patches have been landed.  Closing bug.