When analyzing a document loading trace, it's useful to know what lines of HTML have been parsed. Let's add some trace points for this.
<rdar://problem/59824845>
Created attachment 391793 [details] Patch
Comment on attachment 391793 [details] 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.
Comment on attachment 391793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391793&action=review > Source/WTF/ChangeLog:5 > + Ok as-is. NO change necessary. Another "in the know" convention is to put radar URL here and in every ChangeLog because: 1. Makes it convenient for Apple engineers to open radar bug
Created attachment 392006 [details] Patch
Comment on attachment 392006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392006&action=review > Source/WebCore/html/parser/HTMLDocumentParser.cpp:70 > + , m_shouldEmitTracePoints(isMainDocumentLoadingFromHTTP(document)) Why only when isMainDocumentLoadingFromHTTP?
(In reply to Sam Weinig from comment #6) > Comment on attachment 392006 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392006&action=review > > > Source/WebCore/html/parser/HTMLDocumentParser.cpp:70 > > + , m_shouldEmitTracePoints(isMainDocumentLoadingFromHTTP(document)) > > Why only when isMainDocumentLoadingFromHTTP? For one thing, when you only log intervals related to the main document, it's unambiguous which URL is being parsed when you see the interval in the trace. Once you start emitting parsing intervals related to iframes in the trace, the next question is probably what iframe URL is being parsed. Passing in string metadata like this isn't easy to do for a ktrace-based probe. For another, if trace points can nest (like iframe parsing interval under a main frame parsing interval), then you have to pass something like a pointer to allow the instrumentation tool to properly nest the intervals. We don't do this properly anywhere with WTF::tracePoint right now. I am thinking of passing HTML parsing events through os_signpost, which has a richer API for annotation which allows us to pass things like URLs, but they show up in a bit uglier way in our instrumentation tool, and it's blocked by https://bugs.webkit.org/show_bug.cgi?id=208395.
Created attachment 392245 [details] Patch for landing
Comment on attachment 392245 [details] Patch for landing Clearing flags on attachment: 392245 Committed r257763: <https://trac.webkit.org/changeset/257763>
All reviewed patches have been landed. Closing bug.