Bug 40311

Summary: video-timeupdate-duringplayback.html missing closing angle
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 39259    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Tony Gentilcore
Reported 2010-06-08 10:46:16 PDT
video-timeupdate-duringplayback.html missing closing angle
Attachments
Patch (1.26 KB, patch)
2010-06-08 11:04 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch (1.95 KB, patch)
2010-06-08 13:00 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.59 KB, patch)
2010-06-09 10:47 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Tony Gentilcore
Comment 1 2010-06-08 11:04:20 PDT
Created attachment 58160 [details] Patch
Tony Gentilcore
Comment 2 2010-06-08 11:08:27 PDT
We shouldn't land this until verifying there is an appropriate parser test. I figured I'd check with you first to see if you know where it is off-hand. If not, I'll dig around and/or add one.
Eric Seidel (no email)
Comment 3 2010-06-08 11:14:05 PDT
You can always add another test. fast/invalid doesn't seem to cover this exact case, but I would expect html5lib/resources/* does. I would just add another test to html5lib/resources/webkit01.dat to be sure. :)
Tony Gentilcore
Comment 4 2010-06-08 13:00:15 PDT
Created attachment 58173 [details] Patch
Eric Seidel (no email)
Comment 5 2010-06-08 13:13:59 PDT
Comment on attachment 58173 [details] Patch Your added test does not test anything interesting. I think you meant </p
Tony Gentilcore
Comment 6 2010-06-08 13:19:27 PDT
(In reply to comment #5) > (From update of attachment 58173 [details]) > Your added test does not test anything interesting. I think you meant </p Look at it again; it is the first "/p" that is missing the ">". The bug with the other test was that in "<p>foo</p<script ...>", we were ignoring the <script>. The point of this test is to ensure the second <p> shows up. The HTML5 parser currently produces: <html> <head> <body> <p> "Test" "Test2" I'm checking for: <html> <head> <body> <p> "Test" <p> "Test2"
Eric Seidel (no email)
Comment 7 2010-06-08 13:40:59 PDT
Did you check the HTML5 spec/ Mozilla minefield to see which behavior is correct?
Adam Barth
Comment 8 2010-06-08 13:51:42 PDT
Comment on attachment 58173 [details] Patch ok
Tony Gentilcore
Comment 9 2010-06-08 17:10:15 PDT
(In reply to comment #8) > (From update of attachment 58173 [details]) > ok (In reply to comment #7) > Did you check the HTML5 spec/ Mozilla minefield to see which behavior is correct? Both minefield and the spec don't allow for </foo<bar> to be parsed as the closing of foo and the opening of bar. However, the legacy webkit parser does. So I'd like to fix the video-timeupdate-duringplayback.html layout test so that it doesn't have this bad markup because that isn't at all the point of the test. But I think it is important to track this difference somehow. Should I update the expectation to pass with the HTML5 parser and fail with the legacy parser?
Tony Gentilcore
Comment 10 2010-06-09 10:05:59 PDT
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 58173 [details] [details]) > > ok > > (In reply to comment #7) > > Did you check the HTML5 spec/ Mozilla minefield to see which behavior is correct? > > Both minefield and the spec don't allow for </foo<bar> to be parsed as the closing of foo and the opening of bar. However, the legacy webkit parser does. > > So I'd like to fix the video-timeupdate-duringplayback.html layout test so that it doesn't have this bad markup because that isn't at all the point of the test. But I think it is important to track this difference somehow. Should I update the expectation to pass with the HTML5 parser and fail with the legacy parser? I see you r+'d this. But I'm not sure the test expectation is set correctly. Should it be set based on [1] the spec (less compatible, old parser fails, new one passes) or based on [2] the old behavior (more compatible, old parser passes, new one fails)? Currently this patch does #2.
Adam Barth
Comment 11 2010-06-09 10:19:01 PDT
The #document field of the HTML5lib tests should be set to what the spec says to do. The -expected.txt should be set to the current behavior of the old parser (so the bots stay green). the -html5-expected.txt should be set to the current behavior of the HTML5 parser so we can track regressions. I'll fix the expectations before landing.
Adam Barth
Comment 12 2010-06-09 10:31:57 PDT
Comment on attachment 58173 [details] Patch The new tests fails in the HTML5 parser. Interestingly, the test passes in Minefield. That means one of us is not following the spec. :) Test 15 of 15 in resources/webkit01.dat failed. Input: <p>Test</p<p>Test2</p> Got: | <html> | <head> | <body> | <p> | "Test" | "Test2" Expected: | <html> | <head> | <body> | <p> | "Test" | <p> | "Test2"
Adam Barth
Comment 13 2010-06-09 10:40:38 PDT
Comment on attachment 58173 [details] Patch Sorry for the confusion. I was running Minefield on the wrong instance of the tests. We match Minefield, as expected. I'll land with the corrected expectations.
Adam Barth
Comment 14 2010-06-09 10:47:01 PDT
Created attachment 58258 [details] Patch for landing
Adam Barth
Comment 15 2010-06-09 10:50:05 PDT
Committed r60900: <http://trac.webkit.org/changeset/60900>
Note You need to log in before you can comment on or make changes to this bug.