[webkit-reviews] review granted: [Bug 63082] HTML parser should limit element depth of produced tree : [Attachment 98119] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 22 10:22:32 PDT 2011


Adam Barth <abarth at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 63082: HTML parser should limit element depth of produced tree
https://bugs.webkit.org/show_bug.cgi?id=63082

Attachment 98119: Patch
https://bugs.webkit.org/attachment.cgi?id=98119&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98119&action=review

This looks good.  I've now checked the HTMLElementStack accounting in detail. 
I think the only remaining issue is whether the foster-parenting code path
correct enforces the depth limit.  I'm marking r+/cq-, so you should feel free
to either land this patch with the nits fixed and deal with foster parenting in
a followup patch or post an updated version of the patch with the foster
parenting issues addressed/tested.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:103
> +	   parent->parserAddChild(child);
> +    } else
> +	   parent->parserAddChild(child);

Looks like you didn't move this code outside the if-block.

Do we need a similar check in HTMLConstructionSite::attachAtSite (or in the
code that chooses the site for attachAtSite?

>> Source/WebCore/html/parser/HTMLTreeBuilder.h:119
>> +	HTMLTreeBuilder(HTMLDocumentParser* parser, HTMLDocument*, bool
reportErrors, bool usePreHTML5ParserQuirks, unsigned maximumDOMTreeDepth);
> 
> The parameter name "parser" adds no information, so it should be removed. 
[readability/parameter_name] [5]

You could fix these style nits if you were so inclined.


More information about the webkit-reviews mailing list