Bug 39985

Created attachment 57525 [details] Patch

Comment on attachment 57525 [details] Patch LayoutTests/ChangeLog:12 + for this test to depend on WebKit's old parser behavior. Can you cite a specific test here? I want to be sure we don't lose test coverage. LayoutTests/ChangeLog:18 + no need to depend on the old title behavior here. This is also tested in fast/tokenizer. I worry that some of these changes will cause web compat problems. We need to keep close tabs on this sort of thing.

Comment on attachment 57525 [details] Patch Besides Web compatibility, there is compatibility with Mac applications using WebKit (such as Dashboard). I think that there are enough different changes to regression tests here to discuss them as part of review, not post-review.

@ap: I don't understand your comment. We're not changing any behavior. We're just improving the quality of our test suite by testing parsing issues in parser tests and editing issues in editing tests.

Adam, I think I'm saying the same as you did in your review. The ChangeLog should mention which tests cover the aspects of behavior that are no longer covered by the changed tests, and a reviewer needs to verify those claims. I never said anything about changing behavior.

Ah, thanks. I misunderstood your comment.

Created attachment 57625 [details] Patch

I'm concerned that after this change, current behavior will mostly be tested as failure cases in html5lib test suite. It may be too easy to overlook that we'll be changing behavior. After all, HTML5 is nothing but a draft yet. Maybe such tests should be copied to fast/invalid?

> Maybe such tests should be copied to fast/invalid? The tests aren't invalid, as far as I can tell. They appear to be perfectly good tests with typos. > I'm concerned that after this change, current behavior will mostly be tested as failure cases in html5lib test suite. It may be too easy to overlook that we'll be changing behavior. Every time we change the results of a test in html5lib we're changing behavior. It's unclear to me why you think these particular parser behaviors ought to be tested in editing tests. The <title> changes are also tested in fast/tokenizer by two tests. The <foo<bar case is already tested in fast/invalid, which is the remedy you propose. As far as I can tell, there's nothing we can do to address your concerns. > After all, HTML5 is nothing but a draft yet. Well, perhaps we should implement the HTML4 parser then. :P

> > After all, HTML5 is nothing but a draft yet. > > Well, perhaps we should implement the HTML4 parser then. :P Ok. I thought of a more witty retort... Let's try that again. :) > After all, HTML5 is nothing but a draft yet. That's true, and, after all, evolution nothing but a theory.

Comment on attachment 57625 [details] Patch Clearing flags on attachment: 57625 Committed r60556: <http://trac.webkit.org/changeset/60556>

All reviewed patches have been landed. Closing bug.

Mitz points out that we shouldn't modify tests in /css1. Can you roll out this patch? > Ok. I thought of a more witty retort... So suppose HTML5 changes in some way, and html5lib tests change consequently. Practically speaking, how will we ensure that upgrading our copy won't degrade coverage in areas that this patch delegated to html5lib tests? Delegating test coverage to a highly volatile external suite doesn't sound like a good idea to me, because upgrading responsibly will be a pain. Do we know if any policy governs maintenance of the suite? I'd expect that its goal is to test HTM5 requirements as well as possible, which is close to our goals, but doesn't align with them 100%.

(In reply to comment #13) > Mitz points out that we shouldn't modify tests in /css1. Can you roll out this patch? I think Eric agreed to undo the changes to /css1 via email. I'm not sure who was CCed on the thread, but I'd expect those changes shortly. > > Ok. I thought of a more witty retort... > > So suppose HTML5 changes in some way, and html5lib tests change consequently. Practically speaking, how will we ensure that upgrading our copy won't degrade coverage in areas that this patch delegated to html5lib tests? > > Delegating test coverage to a highly volatile external suite doesn't sound like a good idea to me, because upgrading responsibly will be a pain. Do we know if any policy governs maintenance of the suite? I'd expect that its goal is to test HTM5 requirements as well as possible, which is close to our goals, but doesn't align with them 100%. Alexey, I'm not looking to argue with you, but I'm not sure you have all the facts right. My understanding is that there are two parser behaviors at issue here: 1) Unterminated <title> tags. This is covered both by the html5lib test suite and by: http://trac.webkit.org/browser/trunk/LayoutTests/fast/tokenizer/missing-title-end-tag-1.html http://trac.webkit.org/browser/trunk/LayoutTests/fast/tokenizer/missing-title-end-tag-2.html as mentioned in Comment #9. 2) Nested start tags (e.g., "<a<b"). This is covered both by the html5lib test suite and by http://trac.webkit.org/browser/trunk/LayoutTests/fast/invalid/016.html as mentioned in the ChangeLog. Is there something else specific you're worried about? I read, understood, and acted on your feedback. You've already said it was the same as my review in comment #2 of the original patch. I'm not sure why we're still arguing about this.

Committed r60576: <http://trac.webkit.org/changeset/60576>

Looks like I did have my facts wrong. My feeling was that each unclosed tag's handling was (or could potentially be) different, and only some cases were covered. Looking at the ChangeLog again, I see that I misunderstood its structure - there are two lists in it, not one.