Bug 53275

Created attachment 80395 [details] first try

Comment on attachment 80395 [details] first try View in context: https://bugs.webkit.org/attachment.cgi?id=80395&action=review > Source/WebCore/ChangeLog:10 > + Refactor only, no new tests. This is not a refactor, you're fixing a bug. We need a regression test. :)

(In reply to comment #2) > (From update of attachment 80395 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80395&action=review > > > Source/WebCore/ChangeLog:10 > > + Refactor only, no new tests. > > This is not a refactor, you're fixing a bug. We need a regression test. :) Thanks Kling, I will work on making a test for it :)

Created attachment 80481 [details] add logs to help testing

Kling, LayoutTests/media/video-can-play-type.html is supposed to be the test for this change, however, it may not help much because no one really uses the media type codecs. For example,MediaPlayerPrivateQTKit::supportsType() only checks whether the codecs is empty or not (this can explain why this bug exists so long without causing any trouble.). I tried to fix it since MediaPlayerPrivateQt::supportsType() needs to pass correct codecs to QMediaPlayer::hasSupport(mime, QStringList(codec)). However, the hasSupport() may have other unsolved issues since it always returns "QtMultimediaKit::MaybeSupported", event the mime type is "video/blahblah". So all the layouttest is not really reliable. Instead, I added some logs in the patch to help testing. If you have better idea to make a test for this change, please let me know. thx

Comment on attachment 80481 [details] add logs to help testing There is nothing in RFC 2616 that talks about SGML-style named entities, so """ is not magic in HTTP. If there is an actual compatibility issue that affects existing Web sites, please provide more information about it.

(In reply to comment #6) > (From update of attachment 80481 [details]) > There is nothing in RFC 2616 that talks about SGML-style named entities, so """ is not magic in HTTP. > > If there is an actual compatibility issue that affects existing Web sites, please provide more information about it. Thanks Alexey for review :). One example for """ issue is LayoutTests/media/video-can-play-type.html. testExpected("video.canPlayType('video/mpeg; Codecs="avc1.4D400C"')", "probably"); Also,if you check the ContentType::parameter() in ContentType.cpp and the 50th line seems not right, size_t end = strippedType.find(';', start + 6); Note, the 'start' is the index of semi-colon, then why it adds 6 here?

(In reply to comment #7) > One example for """ issue is LayoutTests/media/video-can-play-type.html. I don't know why this subtest was originally added. This could be an intentional test for an incorrect parameter, or a simple mistake (perhaps \" should have been used instead of "). Nothing in the test's history indicates that " should be parsed as a quotation mark. > Also,if you check the ContentType::parameter() in ContentType.cpp and the 50th line seems not right, > > size_t end = strippedType.find(';', start + 6); > > Note, the 'start' is the index of semi-colon, then why it adds 6 here? Yes, this part of your patch seems fine. I didn't verify it in detail for off by one errors and such. Please add tests for the issues you fixed.

(In reply to comment #8) > (In reply to comment #7) > > One example for """ issue is LayoutTests/media/video-can-play-type.html. > > I don't know why this subtest was originally added. This could be an intentional test for an incorrect parameter, or a simple mistake (perhaps \" should have been used instead of "). > > Nothing in the test's history indicates that " should be parsed as a quotation mark. > > > Also,if you check the ContentType::parameter() in ContentType.cpp and the 50th line seems not right, > > > > size_t end = strippedType.find(';', start + 6); > > > > Note, the 'start' is the index of semi-colon, then why it adds 6 here? > > Yes, this part of your patch seems fine. I didn't verify it in detail for off by one errors and such. Please add tests for the issues you fixed. I have reported a qtmobility bug, http://bugreports.qt.nokia.com/browse/QTMOBILITY-1091, once it gets fixed, then we can run LayoutTests/media/video-can-play-type.html to test this change.

I spoke to Yi and he says he is not working on this error any longer. I started working on a related error (https://bugs.webkit.org/show_bug.cgi?id=42094) so I told Yi I'd take over this error. I looked into the test case video-can-play-type.html and I think there is an issue with the content. I don't think " is a valid way to specify quotes in javascript. I think you should use \" instead. However, when I fix this it still doesn't solve the problem with the test case failing because MediaPlayPrivateQt::supportsType does not return the right values. I will create a separate error for fixing the test content. If that gets approved then I think maybe this error can be closed as not valid since the problem is in the test content and not the code. I don't think the ContentType class should have to parse out the entity references (ie, "). The issue with video-can-play-type.html failing for Qt can be fixed under 42094.

Actually we may still need part of this fix to parse out the quotation marks.

Created the following bug to change """ to '\"' in the layout test. https://bugs.webkit.org/show_bug.cgi?id=57728

After looking at the specification for the codecs paramter, http://tools.ietf.org/html/rfc4281, I think the intention of the ContentType class was to return everything after the '=' since single quotes and double quotes have special meaning to that paramter. So, I'm not sure that the quotes should be parsed out. However, I'm not sure about the '+ 6' that Yi mentions above. I will look intot that.

Quote handling in Content-Type is governed by rfc2616, not by rfc4281. Please see sections 3.7 and 3.6 in <http://www.ietf.org/rfc/rfc2616.txt>: parameter = attribute "=" value attribute = token value = token | quoted-string

I was looking at the following section of the HTML5 spec which references RFC4281. I will look at your references more carefully also. 4.8.10.3 MIME types A media resource can be described in terms of its type, specifically a MIME type, in some cases with a codecs parameter. (Whether the codecs parameter is allowed or not depends on the MIME type.) [RFC4281]

After looking at RFC2616, I conclude that double quotes should be parsed out but not single quotes. Here are excerpts from RFC2616 for reference: Parameters are in the form of attribute/value pairs. parameter = attribute "=" value attribute = token value = token | quoted-string A string of text is parsed as a single word if it is quoted using double-quote marks. quoted-string = ( <"> *(qdtext | quoted-pair ) <"> ) qdtext = <any TEXT except <">> Alexey - Could you also look at https://bugs.webkit.org/show_bug.cgi?id=57728 and see if you agree?

> double quotes should be parsed out but not single quotes Yes, that's correct.

Created attachment 88340 [details] Patch to fix codecs parsing Attached patch to parse out quotes from 'codecs' parameter according to RFC2616 mentioned above. Also, added some tests to the existing video-can-play-type.html layout test to test the codecs parsing.

Comment on attachment 88340 [details] Patch to fix codecs parsing View in context: https://bugs.webkit.org/attachment.cgi?id=88340&action=review I only had a brief look yet, and didn't verify that the parsing code is correct. > Source/WebCore/platform/ContentType.cpp:50 > + size_t quote = notFound, end = notFound; Please don't declare two variables on one line.

Created attachment 88397 [details] New patch based on Alexey's comments Put the two variable declarations on separate lines.

Comment on attachment 88397 [details] New patch based on Alexey's comments View in context: https://bugs.webkit.org/attachment.cgi?id=88397&action=review > Source/WebCore/ChangeLog:5 > + Parse quotes from content type codecs parameter This comment is incorrect, the change is for any parameter not just "codecs". > Source/WebCore/platform/ContentType.cpp:48 > - start = strippedType.find('=', start + 6); > + start = strippedType.find('=', start + 1); This isn't right, it starts the search at the beginning of the parameter name instead of at the end. > Source/WebCore/platform/ContentType.cpp:52 > + size_t quote = notFound; > + size_t end = notFound; > + if ((quote = strippedType.find('\"', start + 1)) != notFound && (end = strippedType.find('\"', start + 2)) != notFound) // find dobule quote The initializations to notFound are not helpful because you always set quote and end to strippedType.find(). I think it would be easier to read if you made the assignments outside of the test, but that may just be me.

Thanks! I will rework the patch.

Created attachment 88616 [details] Modified patch based on Eric's comments

Comment on attachment 88616 [details] Modified patch based on Eric's comments View in context: https://bugs.webkit.org/attachment.cgi?id=88616&action=review r+ with the minor changes suggested. Thanks! > Source/WebCore/ChangeLog:7 > + > + Parse quotes from content type parameters > + https://bugs.webkit.org/show_bug.cgi?id=53275 > + It would be helpful to have a comment about how this change is tested, eg. something like the comment you are adding to the LayoutTests ChangeLog. > Source/WebCore/platform/ContentType.cpp:52 > + if (quote != notFound && end != notFound) // find dobule quote Typo: dobule -> double. However, the comment doesn't really add anything for someone reading the code later so I would just remove it.

Created attachment 88621 [details] Modified patch based on Eric's latest comments

Comment on attachment 88621 [details] Modified patch based on Eric's latest comments Thanks!

Comment on attachment 88621 [details] Modified patch based on Eric's latest comments Clearing flags on attachment: 88621 Committed r83191: <http://trac.webkit.org/changeset/83191>

All reviewed patches have been landed. Closing bug.