Bug 124715 – [GStreamer] Invalid command line error when visiting www.chessbase.com

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

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.

[Xabier Rodríguez Calvar]

Created attachment 217763 [details]
Patch

HTTP errors were not being handled at the GStreamer WebKit source

[Philippe Normand]

Comment on attachment 217763 [details]
Patch

Hum, please talk to Andres, he has a patch for this code too.

[Xabier Rodríguez Calvar]

Comment on attachment 217763 [details]
Patch

There's a problem with the comment.

[Xabier Rodríguez Calvar]

Created attachment 217814 [details]
Patch

Rebased

[Sergio Villar Senin]

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?

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

Created attachment 217882 [details]
Patch

Added test. It doesn't fail either because it doesn't play either though for a different reason.

[Xabier Rodríguez Calvar]

(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.

[WebKit Commit Bot]

Comment on attachment 217882 [details]
Patch

Clearing flags on attachment: 217882

Committed r159810: <http://trac.webkit.org/changeset/159810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.