[webkit-reviews] review granted: [Bug 208271] Add performance probes for HTML parsing : [Attachment 391793] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Ben Nham <nham at apple.com>'s
request for review:
Bug 208271: Add performance probes for HTML parsing
https://bugs.webkit.org/show_bug.cgi?id=208271

Attachment 391793: Patch

https://bugs.webkit.org/attachment.cgi?id=391793&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 391793
  --> https://bugs.webkit.org/attachment.cgi?id=391793
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391793&action=review

This patch is good.

> Source/WTF/ChangeLog:8
> +	   This adds some instrumentation so we can know which lines of HTML
have been parsed when

This is ok as-is. No change is necessary. FYI it is an "in the know" convention
that change log lines are wrapped at ~100 characters.

> Source/WebCore/ChangeLog:8
> +	   This adds some instrumentation so we can know which lines of HTML
have been parsed when

Ditto. Again, NO change necessary.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:30
> +#include <wtf/SystemTracing.h>

This is ok as-is. According to WebKit code style this include should be after
all other includes.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:307
> +    bool isMainDocument = document() && document()->frame() &&
document()->frame()->isMainFrame();

This is ok as-is. The optimal solution removes the null check of document()
because 

1. It can never be nullptr when THIS function called.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:309
> +	   TextPosition startPosition = textPosition();

This is ok as-is. The optimal solution is to use auto because

1. The right hand side (rhs)  is as descriptive as the left.
2. If the data type of the rhs were to change in the future then using auto
will prevent implicit conversion, which could be a ��.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:316
> +	   TextPosition endPosition = textPosition();

Ditto.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:317
> +	   tracePoint(ParseHTMLEnd, endPosition.m_line.oneBasedInt(),
endPosition.m_column.oneBasedInt());

This is ok as-is. A better solution would be to define a lambda or INLINED
private member function that takes a TracePointCode and use this function twice
because:

1. Do not need to duplicate code
2. If there is a bug in this code or future enhancement then it can be made in
one place instead of 2.

One suggestion for a name, tracePointIfMainDocument()

> Tools/ChangeLog:8
> +	   This adds some instrumentation so we can know which lines of HTML
have been parsed when

Ok as is. NO change necessary. Line is long.