Bug 80127 – Web Inspector: add timeline instrumentation for frame events

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Kosyakov]

This adds an instrumentation interfaces and some visualization for frame-related events:
- beginning and end of frame rendering (end is currently ignored)
- layer compositing events

When frame marks are available, these are displayed in "start at zero" overview mode as vertical lines on main timeline grid, provided it is zoomed to 1 second per screen or closer. Also, start at zero aggregates events by frames.

[Andrey Kosyakov]

Created attachment 129844 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 129844 [details]
Patch

Attachment 129844 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11776932

New failing tests:
inspector/timeline/timeline-enum-stability.html

[Timothy Hatcher]

Comment on attachment 129844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129844&action=review

> Source/WebKit/chromium/ChangeLog:20
> +        * public/WebWidget.h:
> +        (WebWidget):
> +        (WebKit::WebWidget::willBeginUpdate):
> +        (WebKit::WebWidget::didEndUpdate):
> +        * src/WebDevToolsAgentImpl.cpp:
> +        (WebKit::WebDevToolsAgentImpl::willComposeLayers):
> +        (WebKit):
> +        (WebKit::WebDevToolsAgentImpl::didComposeLayers):
> +        (WebKit::WebDevToolsAgentImpl::willBeginFrameUpdate):
> +        (WebKit::WebDevToolsAgentImpl::didEndFrameUpdate):

Please file bugs for the other WebKit ports to hook this up.

[Andrey Kosyakov]

Created attachment 129892 [details]
Patch

[Pavel Feldman]

Comment on attachment 129892 [details]
Patch

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.

[Andrey Kosyakov]

Created attachment 129932 [details]
Patch

[Pavel Feldman]

Comment on attachment 129932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129932&action=review

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:283
> +        return this._windowEndTime || this._presentationModel.maximumRecordTime();

!!this._windowEndTime ?

> Source/WebCore/inspector/front-end/TimelineOverviewPane.js:809
> +        var scale = (this.element.clientHeight - 4) / boundarySpan;

Please use const paddingTop = 4;

[Pavel Feldman]

I don't think we should wait for Darin's approval for the WebView::instrumentBeginFrame() and it can be landed safely.

Darin, if you have any concerns about the changes to the API, please let us know and we will fix it.

[Andrey Kosyakov]

Committed r109724: <http://trac.webkit.org/changeset/109724>