Summary: | [GStreamer] Invalid command line error when visiting www.chessbase.com | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||||
Component: | WebKitGTK | Assignee: | 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
Sergio Villar Senin
2013-11-21 07:08:12 PST
Created attachment 217763 [details]
Patch
HTTP errors were not being handled at the GStreamer WebKit source
Comment on attachment 217763 [details]
Patch
Hum, please talk to Andres, he has a patch for this code too.
Comment on attachment 217763 [details]
Patch
There's a problem with the comment.
Created attachment 217814 [details]
Patch
Rebased
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? (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. Created attachment 217882 [details]
Patch
Added test. It doesn't fail either because it doesn't play either though for a different reason.
(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 on attachment 217882 [details] Patch Clearing flags on attachment: 217882 Committed r159810: <http://trac.webkit.org/changeset/159810> All reviewed patches have been landed. Closing bug. |