[webkit-reviews] review denied: [Bug 105796] Web Inspector: plumb trace events to Timeline agent : [Attachment 180791] 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 105796: Web Inspector: plumb trace events to Timeline agent
https://bugs.webkit.org/show_bug.cgi?id=105796

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

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


> Source/WebCore/inspector/InspectorInstrumentation.h:122
> +    static void didReportTraceEvent(Page*, char phase, const char*
categoryName, const char* name, unsigned long long id,

traceEvent ?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:117
> +	   : m_size(numArguments), m_argumentNames(argumentNames),
m_argumentTypes(argumentTypes), m_argumentValues(argumentValues)

stack these please.
who owns the argumentNames?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:123
> +    bool getBool(const char* name) const { return getParameter(name,
TRACE_VALUE_TYPE_BOOL).m_bool; }

Place block on its own lines. asBool(const char* name).

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:140
> +    const TraceValueUnion& getParameter(const char* name, char expectedType)
const

parameter(

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:226
> +	   m_client->enableTraceEventsReporting(true);

setReportTraceEvents

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:243
> +    m_deferredPaintLayerId = 0;

Lets split these.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:378
> +    TraceEventHandler handler = m_traceEventHandlers.get(name);

Lets make name+phase a key.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:385
> +const char kLayerId[] = "layerId";

anonymous namespace for this one?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:692
> +    m_traceEventHandlers.set("Instrumentation::DeferPaint",
&InspectorTimelineAgent::onDeferPaint);

I would whitelist the ones we want to expose + handle specifics using switch.
Or lets introduce generic fallback handler. Or rename to onTraceEventDeferPaint


> Source/WebCore/inspector/InspectorTimelineAgent.cpp:725
> +void InspectorTimelineAgent::pushSkippedRecord(const String& type)

mute? Can we also land this as a separate patch?