RESOLVED FIXED 43836
Delete LegacyHTMLDocumentParser and LegacyPreloadScanner
https://bugs.webkit.org/show_bug.cgi?id=43836
Summary Delete LegacyHTMLDocumentParser and LegacyPreloadScanner
Adam Barth
Reported 2010-08-11 01:16:37 PDT
Delete LegacyHTMLDocumentParser and LegacyPreloadScanner
Attachments
Patch (173.30 KB, patch)
2010-08-11 01:20 PDT, Adam Barth
no flags
Patch (177.28 KB, patch)
2010-08-11 01:40 PDT, Adam Barth
no flags
Adam Barth
Comment 1 2010-08-11 01:20:18 PDT
Adam Barth
Comment 2 2010-08-11 01:40:26 PDT
Eric Seidel (no email)
Comment 3 2010-08-11 09:27:44 PDT
Comment on attachment 64087 [details] Patch WebCore/dom/DocumentParser.h:62 + // used to implements it. used to implement My name is Eric Seidel, and I support this message.
Darin Adler
Comment 4 2010-08-11 09:30:30 PDT
At Apple we’re starting to get multiple bug reports about Mac OS X applications and websites that don’t work any more due to differences between the old and new parser. One example is AOL’s AIM client, which malfunctions in a spectacular way. But I’m sure more of these reports are coming as people have more time to test. Once you delete the old parser, we will presumably have to add hacks to the new one to make that old content work. Is that your preferred approach?
Adam Barth
Comment 5 2010-08-11 09:50:26 PDT
> One example is AOL’s AIM client, which malfunctions in a spectacular way. But I’m sure more of these reports are coming as people have more time to test. Yes. Some of these reports have filtered into bugs.webkit.org as bugs blocking Bug 41115. If you're able to share the reports (or sanitized versions of the reports), filing them as blocking Bug 41115 will make sure they don't get lost. > Once you delete the old parser, we will presumably have to add hacks to the new one to make that old content work. Is that your preferred approach? We talked this over with Alexey at some point. We think that's a better approach because maintaining branches in the new parser seems easier then maintaining an entirely separate parser. As an example, Chrome decided to ship the old parser in the upcoming M6 beta because we're not quite done with the new parser yet. When they turned it on, they found that it had already bit rotted to the point of a use-after-free on every document parse due to this change: http://trac.webkit.org/changeset/62302/trunk/WebCore/html/LegacyHTMLDocumentParser.cpp Notice the comment at the top of the diff that says "this actually causes us to be deleted." I don't mean to blame anyone by pointing out this patch. It just illustrates the general principle that "if it's not tested, it's wrong."
Adam Barth
Comment 6 2010-08-11 11:31:22 PDT
Comment on attachment 64087 [details] Patch Clearing flags on attachment: 64087 Committed r65167: <http://trac.webkit.org/changeset/65167>
Adam Barth
Comment 7 2010-08-11 11:31:28 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 8 2010-08-11 11:49:50 PDT
http://trac.webkit.org/changeset/65167 might have broken Chromium Win Release The following changes are on the blame list: http://trac.webkit.org/changeset/65166 http://trac.webkit.org/changeset/65167
Note You need to log in before you can comment on or make changes to this bug.