[webkit-reviews] review denied: [Bug 48719] HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40% : [Attachment 81694] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied  review:
Bug 48719: HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
https://bugs.webkit.org/show_bug.cgi?id=48719

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81694&action=review

I'm surprised this works.

I would think there are cases which could cause us to modify nodes outside of
the container (which shouldn't happen), like how we move <style> nodes to the
head during parsing.  I'd have to check what states the TreeBuilder is set in
when we enter fragment parsing.

Obviously this bug is important to Apple, given you're the 3rd engineer to try
this.
I'm happy to be of help.

I am blind to some of the internal pressures which may have pointed you at this
since 1.  I believe the benchmark is crap (and the 40% regression mostly
meaningless).  2.  This is a really scary change.  But I'm happy to look at
this with you again tomorrow and help you get this landed.

> Source/WebCore/ChangeLog:8
> +	   No new tests. No change in behavior.

We have a fragment parsing performance test now, which I added earlier.  How
does this change affect that benchmark (which I wrote to model Peacekeeper and
make thsi change easier to write).

> Source/WebCore/html/parser/HTMLConstructionSite.h:46
> +    HTMLConstructionSite(Document*, ContainerNode*,
FragmentScriptingPermission, bool isParsingFragment);

The ContainerNode parameter should be named.

> Source/WebCore/html/parser/HTMLConstructionSite.h:133
> +    ContainerNode* m_root;

We need to change this name to avoid confusion with "root" which is explicitly
covered in the spec:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-end.html#fragme
nt-case

Maybe m_attachmentRoot? with some sort of comment to explain what it's for.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:344
> +    , m_tree(document, document, FragmentScriptingAllowed, false)

Seems we should start thinking about having a separate constructor now that we
have 3 implied parameters in the non-fragment case.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:418
> +    while (firstElement && !firstElement->isElementNode())
> +	   firstElement = firstElement->nextSibling();

Why is this necessary?	What about if nothing was added to the fragment?  (like
innerHTML="") won't we hit the later assert?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:420
> +    m_fragment->removeAllChildren();

removeAllChildren is differnet from removeChildern (doesn't send certain
callbacks) and may be wrong here.  We recently had some use-after-free bug
related to these being mis-matched.

Why do we even need to call this?  takeAllChildrenFrom should remove the
children for us.