RESOLVED FIXED 33995
Web Inspector: Additional instrumentation for Timeline
https://bugs.webkit.org/show_bug.cgi?id=33995
Summary Web Inspector: Additional instrumentation for Timeline
Ilya Tikhonovsky
Reported 2010-01-22 02:05:12 PST
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.
Attachments
caller information screenshot (53.12 KB, image/png)
2010-01-22 02:05 PST, Ilya Tikhonovsky
no flags
Function call information screenshot (53.74 KB, image/png)
2010-01-22 02:08 PST, Ilya Tikhonovsky
no flags
Additional Timeline instrumentation. (25.12 KB, patch)
2010-01-22 06:07 PST, Ilya Tikhonovsky
pfeldman: review-
Second iteration for the first patch to WebCore (70.07 KB, patch)
2010-01-22 08:36 PST, Ilya Tikhonovsky
pfeldman: review-
Mockup (116.09 KB, image/png)
2010-01-23 03:07 PST, Timothy Hatcher
no flags
Balloon images (36.09 KB, application/zip)
2010-01-23 03:22 PST, Timothy Hatcher
no flags
Preliminary patch for third step. It can be landed only after first WebKit patch and patch for v8 Debug API (1.27 KB, patch)
2010-02-02 09:09 PST, Ilya Tikhonovsky
no flags
Next version of Timeline improvenents (79.72 KB, patch)
2010-02-17 07:47 PST, Ilya Tikhonovsky
pfeldman: review-
Screenshot (24.20 KB, image/png)
2010-02-17 07:49 PST, Ilya Tikhonovsky
no flags
Next iteration with new Popover balloons. Can be landed after v8 deps roll. (53.05 KB, patch)
2010-02-21 13:38 PST, Ilya Tikhonovsky
timothy: review-
Screenshot (43.60 KB, image/png)
2010-02-21 13:41 PST, Ilya Tikhonovsky
no flags
Patch with adjusted Popover. (57.93 KB, patch)
2010-02-25 03:05 PST, Ilya Tikhonovsky
pfeldman: review+
The same patch but adjusted WebKit/chromium/DEPS (58.28 KB, patch)
2010-02-25 04:42 PST, Ilya Tikhonovsky
pfeldman: review-
The same patch but fixed inspector's layout tests (60.48 KB, patch)
2010-02-26 01:45 PST, Ilya Tikhonovsky
pfeldman: review+
pfeldman: commit-queue-
Ilya Tikhonovsky
Comment 1 2010-01-22 02:08:00 PST
Created attachment 47183 [details] Function call information screenshot
Pavel Feldman
Comment 2 2010-01-22 02:15:07 PST
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
Comment 3 2010-01-22 06:07:57 PST
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 4 2010-01-22 06:28:28 PST
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
Comment 5 2010-01-22 08:36:06 PST
Created attachment 47208 [details] Second iteration for the first patch to WebCore
Pavel Feldman
Comment 6 2010-01-23 00:15:52 PST
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
Comment 7 2010-01-23 03:07:24 PST
(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
Comment 8 2010-01-23 03:07:53 PST
Timothy Hatcher
Comment 9 2010-01-23 03:22:07 PST
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
Comment 10 2010-01-23 09:42:35 PST
(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
Comment 11 2010-01-23 09:48:25 PST
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
Comment 12 2010-02-02 09:09:36 PST
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
Comment 13 2010-02-17 07:47:21 PST
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
Comment 14 2010-02-17 07:49:22 PST
Created attachment 48903 [details] Screenshot Screenshot
WebKit Review Bot
Comment 15 2010-02-17 07:52:48 PST
Timothy Hatcher
Comment 16 2010-02-17 09:19:17 PST
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
Comment 17 2010-02-17 09:24:51 PST
(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 18 2010-02-18 02:22:17 PST
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
Comment 19 2010-02-21 13:38:14 PST
Created attachment 49168 [details] Next iteration with new Popover balloons. Can be landed after v8 deps roll.
Ilya Tikhonovsky
Comment 20 2010-02-21 13:41:01 PST
Created attachment 49169 [details] Screenshot
WebKit Review Bot
Comment 21 2010-02-21 13:44:43 PST
Timothy Hatcher
Comment 22 2010-02-22 03:01:28 PST
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
Comment 23 2010-02-22 03:04:00 PST
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
Comment 24 2010-02-22 03:36:31 PST
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
Comment 25 2010-02-22 03:49:02 PST
Titles and labels should be Title Case, so: "Timer Fired". But I agree with everthing else Pavel said.
Ilya Tikhonovsky
Comment 26 2010-02-25 03:05:09 PST
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
Comment 27 2010-02-25 03:41:20 PST
Pavel Feldman
Comment 28 2010-02-25 04:24:30 PST
Comment on attachment 49472 [details] Patch with adjusted Popover. Please wait for v8 roll.
Ilya Tikhonovsky
Comment 29 2010-02-25 04:42:42 PST
Created attachment 49473 [details] The same patch but adjusted WebKit/chromium/DEPS
WebKit Review Bot
Comment 30 2010-02-25 05:22:00 PST
Pavel Feldman
Comment 31 2010-02-25 07:02:02 PST
Comment on attachment 49473 [details] The same patch but adjusted WebKit/chromium/DEPS Timeline tests fail when I apply this.
Ilya Tikhonovsky
Comment 32 2010-02-26 01:45:16 PST
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
Comment 33 2010-02-26 01:51:06 PST
Pavel Feldman
Comment 34 2010-02-26 02:48:53 PST
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
Comment 35 2010-02-26 03:26:57 PST
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
Note You need to log in before you can comment on or make changes to this bug.