Bug 33995 – Web Inspector: Additional instrumentation for Timeline

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

[Ilya Tikhonovsky]

Created attachment 47182 [details]
caller information screenshot

While the current timeline view in DevTools provides a great overview of
things happening, we should make it easier to locate the cause of an event,
e.g., link to JS where relevant.

[Ilya Tikhonovsky]

Created attachment 47183 [details]
Function call information screenshot

[Pavel Feldman]

With the screenshots attached, I have hard time understanding what record this bubble belongs to. Maybe it is due to the fact that it is displayed 'above' the record.  

Is it hard to implement it as a drawer animating below the record?

[Ilya Tikhonovsky]

Created attachment 47195 [details]
Additional Timeline instrumentation.

This is the first patch from the set.

There are 3 patches should be applied:
The first one is a patch for WebInspector with popup dialog and the changes for
Caller info support for all kind of Timeline events and new Function Call event.
The second one will be the patch in V8 code for Function Call info.
The third one will be in V8 bindings for pushing Caller info to Timeline.

[Pavel Feldman]

Comment on attachment 47195 [details]
Additional Timeline instrumentation.


> +        There are 3 patches should be applied: 
> +        The first one is a patch for WebInspector with popup dialog and the changes for 
> +        Caller info support for all kind of Timeline events and new Function Call event.
> +        The second one will be the patch in V8 code for Function Call info.
> +        The third one will be in V8 bindings for pushing Caller info to Timeline.
> +

What about JSC?


>  
> +int InspectorTimelineAgent::m_count = 0;
> +

s_instanceCount?

> +    copyCallerInfoToRecord(record);

populateCallerInfo?

> +        static ScriptObject createCallFunctionData(InspectorFrontend*, const String& name, int lineNumber);
> +

createFunctionCallData?

> -button.status-bar-item.toggled-1 .glyph {
> -    background-color: rgb(66, 129, 235);
> -}
> -
> -button.status-bar-item.toggled-2 .glyph {
> -    background-color: purple;   
> -}
> -

Please rebase.

>  
> +function _linkToFile(file, line, panel)
> +{

Either put this into utilities.js or declare on TimelinePanel's constructor.

> +                this._lastRecord.callFromName == formattedRecord.callFromName &&
> +                this._lastRecord.callFromLine == formattedRecord.callFromLine &&

callerURL, callerLine?


> +            this._recordStyles[recordTypes.FunctionCall] = { title: WebInspector.UIString("Function Call"), category: this.categories.scripting };

Ad this to WebCore/English.lproj/localizedStrings.js



> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerInstall) {
> +            this._timerRecords[record.data.timerId] = formattedRecord;
> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerFire) {

No need for {}

> +        case WebInspector.TimelineAgent.RecordType.FunctionCall:
> +            return record.data.scriptName + ":" + (1 + record.data.scriptLine);

Mind JSC vs V8 difference, add in bindings instead.

> +    this._callInfo = document.createElement("span");
> +    this._callInfo.className = "data dimmed";
> +    this._callInfo.textContent = "";
> +

I don't think we need this in the list.

> +    _createCell: function(content)
> +    {
> +        var text = document.createElement("label");
> +        text.className = "popup-message";
> +        text.appendChild(document.createTextNode(content));
> +        var cell = document.createElement("td");
> +        cell.appendChild(text);
> +        return cell;
> +    },
> +

Do not use table / td if possible.

> +        typedef std::pair<String, int> PositionInScript;
> +

private?

> +        void pushCallerInfo(const String& scriptName, int scriptLine) {
> +            m_callInfoStack.append(InspectorTimelineAgent::PositionInScript(scriptName, scriptLine));
> +        }
> +
> +        void popCallerInfo() { 
> +            ASSERT(m_callInfoStack.size() != 1);
> +            m_callInfoStack.shrink(m_callInfoStack.size() - 1); 
> +        }
> +

We do not generally put code into headers.

>      private:
> +        void copyCallerInfoToRecord(ScriptObject& record) {
> +            PositionInScript& callInfo = m_callInfoStack.last();
> +            record.set("callFromName", callInfo.first);
> +            record.set("callFromLine", callInfo.second);
> +        }
> +

Move to cpp

[Ilya Tikhonovsky]

Created attachment 47208 [details]
Second iteration for the first patch to WebCore

[Pavel Feldman]

Comment on attachment 47208 [details]
Second iteration for the first patch to WebCore

> +
> +void InspectorTimelineAgent::willCallFunction(const String& name, int line)
> +{

I'd use sourceName and sourceLine as parameter names. Otherwise it sounds like you are passing a function name.

> +    pushCurrentRecord(TimelineRecordFactory::createFunctionCallData(m_frontend, name, line),
> +        FunctionCallTimelineRecordType);

Long lines are Ok in WebKit.

> +        static int count() { return s_instanceCount; }

instanceCount()

> +        typedef std::pair<String, int> PositionInScript;

PositionInScript might be not the best name for it (given that it contains the name of the script as well). SourceLocation / SourceLine / ScriptNameAndLine ?

> +
> +        Vector< PositionInScript > m_callInfoStack;
> +
>          Vector< TimelineRecordEntry > m_recordStack;
> +
> +        static int s_instanceCount;
>      };
>  

Nit: why so many blank lines?

> -        element.positionAt(newElementPosition.x, newElementPosition.y);
> +        element.positionAt(newElementPosition.x - this._offsetX, newElementPosition.y - this._offsetY);

This does not break conditional breakpoints, right?

> +WebInspector.TimelinePanel.linkToFile = function(file, line, panel)
> +{
> +    var link = document.createElement("a");
> +    link.href = file;
> +    link.lineNumber = line;
> +    link.innerText = file + ":" +line;
> +    if (panel)
> +        link.preferredPanel = panel;
> +    return link;
> +}
> +

There is a _formaterror function in ConsoleView.js that uses specific style for the link and WebInspector.displayNameForURL to render the source name nicely. I think it is worth extracting that code into a WebInspector.createLinkToSourceLine function and using it here. (WebInspector.createLineToSourceLine should be defined in inspector.js in that case).

> -                sendRequestRecord.details = this._getRecordDetails(record);
> +                record.details = this._getRecordDetails(sendRequestRecord);
> +                record.callerScriptName = sendRequestRecord.callerScriptName;
> +                record.callerScriptLine = sendRequestRecord.callerScriptLine;

This is wrong. We receive 'record' from the agent. But that is not something we render. We create a formattedRecrod object that is basically a visual representation of the record we'd like to render. formattedRecord references record via its property. record.details has no meaning, no need to assign anything to it.

I am aslo confused with the "sendRequestRecord.details = this._getRecordDetails(record)." Not sure why it is needed. My only guess is that corresponding record entry did not url info before and I needed to steal if from receivedResponse one.

Another thing is that you should not provide response received / resource finished records with the callers. Resource events are asynchronous, not belonging to the js execution flow. Send request record is probably synchronous and should have it. So everything is all right without the "record.callerScriptName = changes" parts. Caller info is missing here for a reason!

>              }
>          } else if (record.type === WebInspector.TimelineAgent.RecordType.ResourceFinish) {
>              var sendRequestRecord = this._sendRequestRecords[record.data.identifier];
> -            if (sendRequestRecord) // False for main resource.
> +            if (sendRequestRecord) {// False for main resource.
>                  formattedRecord.startTime = sendRequestRecord._responseReceivedFormattedTime;
> +                record.callerScriptName = sendRequestRecord.callerScriptName;
> +                record.callerScriptLine = sendRequestRecord.callerScriptLine;
> +            }

Ditto.

> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerInstall)
> +            this._timerRecords[record.data.timerId] = formattedRecord;
> +        else if (record.type === WebInspector.TimelineAgent.RecordType.TimerFire) {
> +            var timerInstalledRecord = this._timerRecords[record.data.timerId];
> +            if (timerInstalledRecord) {
> +                record.callerScriptName = timerInstalledRecord.callerScriptName;
> +                record.callerScriptLine = timerInstalledRecord.callerScriptLine;

Also wrong - timer fire is initiated by the host timer. So caller should be missing here.

> +            }
> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerRemove) {
> +            var timerInstalledRecord = this._timerRecords[record.data.timerId];
> +            if (timerInstalledRecord) {
> +                record.callerScriptName = timerInstalledRecord.callerScriptName;
> +                record.callerScriptLine = timerInstalledRecord.callerScriptLine;

This is wrong for another reason. Either timer was removed by user (so it should already have proper caller info). Or by some other reason, but not from the install record's caller.


> +        case WebInspector.TimelineAgent.RecordType.FunctionCall:
> +            return record.data.scriptName + ":" + record.data.scriptLine;

See hint on formatting urls above.

[Timothy Hatcher]

(In reply to comment #2)
> With the screenshots attached, I have hard time understanding what record this
> bubble belongs to. Maybe it is due to the fact that it is displayed 'above' the
> record.  
> 
> Is it hard to implement it as a drawer animating below the record?

I also had a hard time knowing what record it was for. I have attached a mockup of what I think it should look like. I can provide the balloon images with the arrow in different corners to account for record position constraints (bottom row, right edge, etc.)

Let me know what you think.

[Timothy Hatcher]

Created attachment 47267 [details]
Mockup

[Timothy Hatcher]

Created attachment 47268 [details]
Balloon images

Here are the 4 balloon images. These should be used as a border image to have flexable sizing.

We use to use these images for Tips, a feature that never came along with the new design. Here is the CSS for them:

http://trac.webkit.org/changeset/52170/trunk/WebCore/inspector/front-end/inspector.css

[Pavel Feldman]

(In reply to comment #9)
> Created an attachment (id=47268) [details]
> Balloon images
> 

Balloons look nice, but I am not sure whether they fit very well here. When suggesting a drawer, I was thinking of following:
1) We might potentially have a lot of information to show. CSS snippet on "recalculate styles", full js stack for those invoked from JS, resource metainfo, brief summary on self time vs total time such as SpeedTracer, etc.
2) In case of drawer, clicking anywhere in that row expands/collapses row-wide details box. Popup should be shown in place (i.e. either for the record label in the list or for a bar). Not a big deal, but given that we already have expand/collapse triangle near the bar, it makes aiming a bit more challenging. 
3) Popup covers sibling records and you might end up doing two clicks to get the next one (that was Ilya's finding).

Do you think we can show big enough balloon to mitigate (1) ?

[Timothy Hatcher]

If you think we wont have room in the balloon, the I agree, expanding the row height to show details makes sense. Just don't call it a drawer, it confused me, since we have a drawer for the console.

[Ilya Tikhonovsky]

Created attachment 47940 [details]
Preliminary patch for third step. It can be landed only after first WebKit patch and patch for v8 Debug API

[Ilya Tikhonovsky]

Created attachment 48902 [details]
Next version of Timeline improvenents

A bit redesigned version of Function Call event and Caller Info.
Should be landed after v8 patch.

[Ilya Tikhonovsky]

Created attachment 48903 [details]
Screenshot

Screenshot

[WebKit Review Bot]

Attachment 48902 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/276166

[Timothy Hatcher]

Why did you go with a balloon that has a very long tail? Could you not get the baloon image and CSS I pointed out to work?

Maybe we want to go with a design that is more like the mock up in bug 35003 (for consistency.)

https://bug-35003-attachments.webkit.org/attachment.cgi?id=48851

[Pavel Feldman]

(In reply to comment #16)
> Why did you go with a balloon that has a very long tail? Could you not get the
> baloon image and CSS I pointed out to work?
> 
> Maybe we want to go with a design that is more like the mock up in bug 35003
> (for consistency.)
> 
> https://bug-35003-attachments.webkit.org/attachment.cgi?id=48851

I will soon send popup class for review (it is more or less universal).

[Pavel Feldman]

Comment on attachment 48902 [details]
Next version of Timeline improvenents

> +        static bool getCallerScriptInfo(String& sourceName, int& sourceLineNumber);
> +

callScriptInfo (or even better callLocation)
return parameters should be passed by pointer.

>  {
>      ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, currentTimeInMilliseconds());
>      record.set("data", TimelineRecordFactory::createGenericTimerData(m_frontend, timerId));
> +    populateCallerInfo(record);
>      addRecordToTimeline(record, TimerRemoveTimelineRecordType);
>  }

We can inline populateCallerInfo into the createGenericRecord.

>  } // namespace WebCore

Warning, strange indent above this line.

>  
>          // Make resource receive record last since request was sent; make finish record last since response received.
>          if (record.type === WebInspector.TimelineAgent.RecordType.ResourceSendRequest) {
> @@ -210,12 +217,32 @@ WebInspector.TimelinePanel.prototype = {
>              if (sendRequestRecord) { // False if we started instrumentation in the middle of request.
>                  sendRequestRecord._responseReceivedFormattedTime = formattedRecord.startTime;
>                  formattedRecord.startTime = sendRequestRecord.startTime;
> -                sendRequestRecord.details = this._getRecordDetails(record);
> +                formattedRecord.details = this._getRecordDetails(sendRequestRecord);
> +                formattedRecord.callerScriptName = sendRequestRecord.callerScriptName;
> +                formattedRecord.callerScriptLine = sendRequestRecord.callerScriptLine;

No need to set javascript caller to the event that is dispatched from other places (timer, network, etc.).


> +                   record.type === WebInspector.TimelineAgent.RecordType.TimerRemove) {
> +            var timerInstalledRecord = this._timerRecords[record.data.timerId];
> +            if (timerInstalledRecord) {
> +                if (!formattedRecord.callerScriptName) {

Timer remove has its own caller info.
> +        if (document._bubble) {
> +            document._bubble.hide();
> +        }

Non need for { }

Let us look at bubble together and we will merge it with Popover.

[Ilya Tikhonovsky]

Created attachment 49168 [details]
Next iteration with new Popover balloons. Can be landed after v8 deps roll.

[Ilya Tikhonovsky]

Created attachment 49169 [details]
Screenshot

[WebKit Review Bot]

Attachment 49168 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/297409

[Timothy Hatcher]

Comment on attachment 49168 [details]
Next iteration with new Popover balloons. Can be landed after v8 deps roll.

I'm not awake enough for a detailed line by line review but here are soome highlevel comments and questions:

Why is there a thicker padding at the bottom of the popover in the screenshot?

The labels should be consistent and Title Case. But I think "Single shot" should be renamed to be "Repeats" and flip the boolean. And "installed at" should be "Call Site" or something not ending in "at".

hideAtClick should be named "hideWhenClicked" and would be more logical as a setter, but a function is fine if only just renamed.

You have some style problems in at least TimelinePanel.js:

 640             }
 641             else if ( this._record.type == WebInspector.TimelineAgent.RecordType.FunctionCall ) {
 642                 var link = WebInspector.linkifyResourceAsNode(this._record.data.scriptName, "scripts", this._record.data.scriptLine);
 643                 bubbleContentTable.appendChild(this._createLinkRow(WebInspector.UIString("Location"), link));
 644             }
 645             else
 646                 bubbleContentTable.appendChild(this._createRow(WebInspector.UIString("Details"),

[Timothy Hatcher]

Other questions:

Why isn't the timer identifer in the popover? It would give a label to the number that we already show and don't explain.

Consider highlighting the row when the popover is open. I think it would help connect the row and the popover.

[Pavel Feldman]

Few more comments on the UI:
1. "Name: Timer fired" should be rendered as a popover title "Timer fired" (https://bug-35003-attachments.webkit.org/attachment.cgi?id=49008).
2. There is no need in the Even Type since event's name gives much better clue on the event origin.
3. You should have sections for generic attributes (start time, duration, caller) and event specifics (timeout, timer id). Specifics should probably go first.
4. "Installed at" seems to be too sophisticated to me. I would appreciate a hyperlink to a Install Timer record in a form of timer id instead. Install timer record would have Caller link. Yes, it is two clicks, but we are not talking about a general operation here, while limiting term vocabulary to "Caller" makes it more clear to user.
5. I'd render start time and duration on single line
6. Timer Fired is likely to have nesting 'Function Call' record. That is true for all the native events that call into JavaScript. However, unlike DOM events, timer fired is only going to make single function call. It would be great if we could should Function Call information in-place in this Timer Fired popover. I.e. which function and where is being called by this event. We can also merge Event and Function Call records entirely in all places where Event leads to exactly single Function Call.

I think 1-5 are worth fixing right away, while (6) can wait. Thoughts?

[Timothy Hatcher]

Titles and labels should be Title Case, so: "Timer Fired". But I agree with everthing else Pavel said.

[Ilya Tikhonovsky]

Created attachment 49472 [details]
Patch with adjusted Popover.

1) Popup title was added;
2) Event type was removed;
3) Timer Id was added;
4) Installed at was renamed;
5) Start and Duration were mixed together;
6) Join of Function Call and Timer Fired event will be implemented later;
7) I'm not sure that jump from Timer Fired event to Timer Install event will be useful, because this step will add additional indirection 
and after such jump the user should seek again initial Timer Fire event if he wants to continue its investigation in Timeline Panel;
8) Row highlight will be implemented later;
9) padding at bottom was removed but we definitely need a space for scroller and scroller space was added at right side of Popover;
10) hideAtClick was renamed.

[WebKit Review Bot]

Attachment 49472 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/308590

[Pavel Feldman]

Comment on attachment 49472 [details]
Patch with adjusted Popover.

Please wait for v8 roll.

[Ilya Tikhonovsky]

Created attachment 49473 [details]
The same patch but adjusted WebKit/chromium/DEPS

[WebKit Review Bot]

Attachment 49473 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/309588

[Pavel Feldman]

Comment on attachment 49473 [details]
The same patch but adjusted WebKit/chromium/DEPS

Timeline tests fail when I apply this.

[Ilya Tikhonovsky]

Created attachment 49575 [details]
The same patch but fixed inspector's layout tests

Inspector's tests were fixed.
1) a try-catch and console.log  were added to show up the reason of exception;
2) notifyDone was inserted into catch statement for preventing the test timeout in case of an exception.

[WebKit Review Bot]

Attachment 49575 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/314301

[Pavel Feldman]

Comment on attachment 49575 [details]
The same patch but fixed inspector's layout tests

th * 2 };
> +        var newElementPosition = { x: 0, y: 0, width: preferredWidth + scrollerWidth, height: preferredHeight };
> +        newElementPosition.heightWithArrow = newElementPosition.height + arrowHeight;

Why not to assign inline?

> +            if (newElementPosition.y + newElementPosition.heightWithArrow - borderWidth >= totalHeight) {
> +                newElementPosition.height = totalHeight - anchorBox.y - anchorBox.height - borderRadius * 2 - arrowHeight;
>              }

No need for { }

> +            if (sendRequestRecord) {// False for main resource.
>                  formattedRecord.startTime = sendRequestRecord._responseReceivedFormattedTime;
> +                formattedRecord.callerScriptName = sendRequestRecord.callerScriptName;
> +                formattedRecord.callerScriptLine = sendRequestRecord.callerScriptLine;

We wanted to remove this, right?

> +            }
> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerInstall) {
> +            formattedRecord.timeout = record.data.timeout;
> +            formattedRecord.singleShot = record.data.singleShot;
> +            this._timerRecords[record.data.timerId] = formattedRecord;
> +        } else if (record.type === WebInspector.TimelineAgent.RecordType.TimerFire ||
> +                   record.type === WebInspector.TimelineAgent.RecordType.TimerRemove) {
> +            var timerInstalledRecord = this._timerRecords[record.data.timerId];
> +            if (timerInstalledRecord) {
> +                formattedRecord.callSiteScriptName = timerInstalledRecord.callerScriptName;
> +                formattedRecord.callSiteScriptLine = timerInstalledRecord.callerScriptLine;
> +                formattedRecord.timeout = timerInstalledRecord.timeout;
> +                formattedRecord.singleShot = timerInstalledRecord.singleShot;
> +            }

I am not a great fan of this code, would prefer link to the timer install instead.

[Pavel Feldman]

Things to fix in the follow up change:
1. Popover is triggered on click, should be true popover (tooltip)
2. Underline is 2px in popover, should be 1px
3. Text in popover should be selectable

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/inspector/timeline-enum-stability-expected.txt
	M	LayoutTests/inspector/timeline-test.js
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/bindings/js/ScriptCallStack.cpp
	M	WebCore/bindings/js/ScriptCallStack.h
	M	WebCore/bindings/v8/ScriptCallStack.cpp
	M	WebCore/bindings/v8/ScriptCallStack.h
	M	WebCore/bindings/v8/V8Proxy.cpp
	M	WebCore/inspector/InspectorTimelineAgent.cpp
	M	WebCore/inspector/InspectorTimelineAgent.h
	M	WebCore/inspector/TimelineRecordFactory.cpp
	M	WebCore/inspector/TimelineRecordFactory.h
	M	WebCore/inspector/front-end/Popover.js
	M	WebCore/inspector/front-end/TimelineAgent.js
	M	WebCore/inspector/front-end/TimelinePanel.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.js
	M	WebKit/chromium/DEPS
Committed r55277