[webkit-reviews] review denied: [Bug 90264] Web Inspector: added low-level instrumentation support for TimelineAgent : [Attachment 150641] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Sergey Rogulenko
<rogulenko at google.com>'s request for review:
Bug 90264: Web Inspector: added low-level instrumentation support for
TimelineAgent
https://bugs.webkit.org/show_bug.cgi?id=90264

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

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


> Source/WebCore/inspector/InspectorTimelineAgent.cpp:488
> +    if (m_lowlevelEventsEnabledStackMark && m_lowlevelEventsEnabledStackMark
== static_cast<int>(m_recordStack.size())) {

The idea behind making default = 0 was so that we could make new member
unsigned and get rid of such casts.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:543
> +	       InspectorInstrumentation::setTimelineAgent(this);

You should call setTimelineAgent from within InspectorTimelineAgent::start.
Otherwise call sites get confused.

> Source/WebCore/inspector/InspectorTimelineAgent.h:202
> +    int m_lowlevelEventsEnabledStackMark;

As I mentioned earlier, this should be m_lowLevelEventsEnabledStackMark.
Furthermore, I don't think you need it. Instead of special-casing paint
instrumentation, you should check whether there is a paint record in the stack
upon the resize / decompress processing.