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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 48719: HTML5 TreeBuilder regressed a Peacekeeper DOM test by 40%
https://bugs.webkit.org/show_bug.cgi?id=48719

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72566&action=review

Idea seems fine, but I think you could tighten up the execution of the idea a
bit. I’ll say review+ but I think you should take another crack at it.

> WebCore/html/parser/HTMLTreeBuilder.cpp:397
> +// innerHTML is a common call).  So we use a shared dummy document.
> +// This sharing works because there can only ever be one fragment
> +// parser at any time.  Fragment parsing is synchronous and done
> +// only from the main thread.  It should be impossible for javascript

One space after periods, please.

> WebCore/html/parser/HTMLTreeBuilder.cpp:408
> +    static int s_sharedDummyDocumentMutex;

I think this should be debug-only.

> WebCore/html/parser/HTMLTreeBuilder.cpp:423
> +void DummyDocumentFactory::releaseDocument(HTMLDocument* dummyDocument)

I think this should be inline.

> WebCore/html/parser/HTMLTreeBuilder.cpp:426
> +    s_sharedDummyDocumentMutex--;

I think this should be debug-only.

> WebCore/html/parser/HTMLTreeBuilder.cpp:450
> +    // Setting the baseURL should work the same as it would have had we
passed
> +    // it during HTMLDocument() construction, since the new document is
empty.
> +   
m_dummyDocumentForFragmentParsing->setURL(fragment->document()->baseURI());

I suggest having the createDummyDocument function take a URL, instead.

> WebCore/html/parser/HTMLTreeBuilder.h:221
> -	   RefPtr<Document> m_dummyDocumentForFragmentParsing;
> +	   // Use a shared dummy document to avoid expensive Document creation.

> +	   // Hold a raw pointer to the document since there is no need to ref
it.
> +	   HTMLDocument* m_dummyDocumentForFragmentParsing;

Since this is a single shared dummy document, why do we need to store a pointer
to it at all?