| Summary: | video-timeupdate-duringplayback.html missing closing angle | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||||||
| Component: | New Bugs | Assignee: | 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
Tony Gentilcore
2010-06-08 10:46:16 PDT
Created attachment 58160 [details]
Patch
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. 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. :) Created attachment 58173 [details]
Patch
Comment on attachment 58173 [details]
Patch
Your added test does not test anything interesting. I think you meant </p
(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" Did you check the HTML5 spec/ Mozilla minefield to see which behavior is correct? Comment on attachment 58173 [details]
Patch
ok
(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? (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. 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. 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"
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.
Created attachment 58258 [details]
Patch for landing
Committed r60900: <http://trac.webkit.org/changeset/60900> |