[webkit-reviews] review denied: [Bug 80127] Web Inspector: add timeline instrumentation for frame events : [Attachment 129892] 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 80127: Web Inspector: add timeline instrumentation for frame events
https://bugs.webkit.org/show_bug.cgi?id=80127

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

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


> Source/WebCore/inspector/InspectorController.cpp:214
> +void InspectorController::willBeginFrameUpdate()

We use will*/did* notation for non-atomic actions, we use did* notation for
atomic actions. Should either be
didBeginFrame() / didEndFrame()
or willProcessFrame / didProcessFrame.

> Source/WebCore/inspector/InspectorController.h:83
> +    void willBeginFrameUpdate();

I would move it into the instrumentation.

> Source/WebCore/inspector/InspectorController.h:85
> +    void willBeginCompositing();

What does Begin / End compositing mean? Can it contain nested paints? If yes,
when and why? Also, seems unrelated to the change.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:64
> +static const char LayersCompositing[] = "LayersCompositing";

This will make test fail.

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:792
> +	   var lastFrameTime = records[0] && records[0].startTime;

records.length && ... ?

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:828
> +		   for (; currentFrame < lastIndex && currentRecord <
records.length; ++currentRecord) {

Could you extract method?

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:-856
> -	   var rightIndex = rightOffset + barWidth >= this.element.clientWidth
? null : Math.ceil((rightOffset - offset0 - 2) / barWidth *
this._recordsPerBar);

What is 2 ?

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:909
> +	       endTime: rightOffset + 3 > windowSpan ? null :
this._barTimes[lastBar].endTime

What is 3?

> Source/WebKit/chromium/public/WebWidget.h:83
> +    virtual void willBeginUpdate() { }

1) "update" looks to generic, WebWidget may be updated in various ways
2) We should emphasize the instrumenting nature of these methods.
debugWillBeginUpdate

instrumentBeginFrame()

> Source/WebKit/chromium/public/WebWidget.h:84
> +    virtual void didEndUpdate() { }

nuke this?

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:234
> +void WebDevToolsAgentImpl::willBeginFrameUpdate()

revert?

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:72
> +    virtual void willBeginFrameUpdate();

revert?

> Source/WebKit/chromium/src/WebDevToolsAgentPrivate.h:46
> +    virtual void willBeginFrameUpdate() = 0;

revert?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1271
> +    if (m_devToolsAgent)

InspectorInstrumentation::didBeginFrame(m_page.get())

> Source/WebKit/chromium/src/WebViewImpl.cpp:1421
> +	   if (m_devToolsAgent)

Separate change please.