Bug 124715 - [GStreamer] Invalid command line error when visiting www.chessbase.com
Summary: [GStreamer] Invalid command line error when visiting www.chessbase.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL: http://www.chessbase.com
Keywords:
Depends on: 85994
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-21 07:08 PST by Sergio Villar Senin
Modified: 2013-11-27 02:45 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.85 KB, patch)
2013-11-24 09:46 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (1.72 KB, patch)
2013-11-25 09:25 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (5.12 KB, patch)
2013-11-26 09:29 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.