Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(173)

Issue 640433002: Update HTML parser's foster-parenting algorithm to match the current spec (Closed)

Created:
6 years, 2 months ago by adamk
Modified:
6 years, 2 months ago
CC:
abarth-chromium, blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, eseidel
Project:
blink
Visibility:
Public.

Description

Update HTML parser's foster-parenting algorithm to match the current spec The spec used to disallow DocumentFragments as foster parents, but this restriction was removed when <template> was added. This patch rewrites Blink's parser to mirror the current spec language, in the process fixing the attached bug (the added html5lib test is a regression test for that bug). Note, however, that this change causes us to fail the test for a different DocumentFragment case, which was added in http://trac.webkit.org/changeset/124465 back when the spec used to require the foster parent to be an element. I've updated the test expectations to match the new behavior. The new behavior, while it matches the spec, is no longer compatible with Firefox (the only other browser I tested). However, it seems unlikely that this case comes up much in practice, since it requires reparenting elements into a fragment while the parser is running. BUG=419762 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183370

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -39 lines) Patch
M LayoutTests/fast/parser/foster-parent.html View 2 chunks +5 lines, -14 lines 0 comments Download
M LayoutTests/fast/parser/foster-parent-expected.txt View 1 chunk +10 lines, -2 lines 0 comments Download
M LayoutTests/html5lib/resources/template.dat View 1 chunk +13 lines, -0 lines 0 comments Download
M Source/core/html/parser/HTMLConstructionSite.cpp View 2 chunks +27 lines, -23 lines 0 comments Download

Messages

Total messages: 10 (4 generated)
adamk
6 years, 2 months ago (2014-10-07 21:29:20 UTC) #2
adamk
abarth is the best reviewer for this, really, since he reviewed the original WebKit change ...
6 years, 2 months ago (2014-10-07 21:31:59 UTC) #4
eseidel
lgtm Please let FF know.
6 years, 2 months ago (2014-10-07 22:04:23 UTC) #6
adamk
On 2014/10/07 at 22:04:23, eseidel wrote: > lgtm > > Please let FF know. Filed ...
6 years, 2 months ago (2014-10-07 22:20:22 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/640433002/1
6 years, 2 months ago (2014-10-07 22:21:24 UTC) #9
commit-bot: I haz the power
6 years, 2 months ago (2014-10-08 00:32:53 UTC) #10
Message was sent while issue was closed.
Committed patchset #1 (id:1) as 183370

Powered by Google App Engine
This is Rietveld 408576698