[webkit-reviews] review denied: [Bug 105851] Web Inspector: show deferred paints on Timeline : [Attachment 180938] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 105851: Web Inspector: show deferred paints on Timeline
https://bugs.webkit.org/show_bug.cgi?id=105851

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180938&action=review


r- for introducing page pointer and not using constants. Otherwise looks
cryptic and good.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:508
> +    TRACE_EVENT_INSTANT1("webkit", "Instrumentation::Paint", "pageId", frame
? reinterpret_cast<unsigned long long>(frame->page()) : 0);

These need constants. Is there anything we could reuse?

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:179
>
+TimelineTraceEventProcessor::TimelineTraceEventProcessor(InspectorTimelineAgen
t* timelineAgent, InspectorClient *client, Page* page)

timeline agent has page agent already.

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:182
> +    , m_pageId(reinterpret_cast<unsigned long long>(page))

page can be recreated.

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:191
> +    m_handlersByType.set("Instrumentation::Paint",
EventTypeEntry(&TimelineTraceEventProcessor::onPaint));

Lets have constants for these in InspectorInstrumentation.