Bug 132073

<rdar://problem/16703731>

Sounds right.

Created attachment 232977 [details] Patch

Comment on attachment 232977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232977&action=review WIP patch looks right so far. Planning to finish this up? > Source/WebInspectorUI/UserInterface/Base/Main.js:1556 > + console.log(event); This should be removed in the final patch. > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:187 > + if (supplementalRepresentedObject !== undefined) { No need for the "!== undefined". > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:195 > + var currentSidebarPanels = WebInspector.detailsSidebar.sidebarPanels; > + for (var sidebarPanel of currentSidebarPanels) { > + if (sidebarPanel.inspect(supplementalRepresentedObject)) > + console.log("Can show that object"); > + else > + console.log("Cannot show object"); > + } > + } You should use WebInspector.sidebarPanelForRepresentedObject to find the right panel. Maybe move this to a helper function to share with the other places that do this. What will you do with the sidebarPanel once you have it? > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:202 > + if (supplementalRepresentedObject !== undefined) { Ditto. > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:210 > + var currentSidebarPanels = WebInspector.detailsSidebar.sidebarPanels; > + for (var sidebarPanel of currentSidebarPanels) { > + if (sidebarPanel.inspect(supplementalRepresentedObject)) > + console.log("Can show that object"); > + else > + console.log("Cannot show object"); > + } > + } Ditto. > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:217 > + if (supplementalRepresentedObject !== undefined) { Ditto. > Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:224 > + var currentSidebarPanels = WebInspector.detailsSidebar.sidebarPanels; > + for (var sidebarPanel of currentSidebarPanels) { > + if (sidebarPanel.inspect(supplementalRepresentedObject)) > + console.log("Can show that object"); > + else > + console.log("Cannot show object"); > + } Ditto. > Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:278 > + console.log(treeElement.representedObject); This should be removed.

Katie is gone for the summer. I will finish off this patch on the weekend or Monday.

Created attachment 242692 [details] Patch

Comment on attachment 242692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242692&action=review > Source/WebInspectorUI/UserInterface/Base/Main.js:407 > + if (representedObject && representedObject.eventType === WebInspector.ScriptTimelineRecord.EventType.ProbeSampleRecorded) > + return this.probeDetailsSidebarPanel; I would feel safer with stricter type checks since this is in the generic namespace and "object" could be anything.. Something like: if (!representedObject) return null; if (representedObject instanceof WebInspector.ScriptTimelineRecord) { if (representedObject.eventType === WebInspector.ScriptTimelineRecord.EventType.ProbeSampleRecorded) return this.probeDetailsSidebarPanel; } return null; That said, it isn't always possible to narrow down a specific representedObject to a single detailsSidebarPanel. For example, a WebInspector.DOMNode is associated with the Styles/Node/LayerTree details sidebars. So I'm not sure this is the right solution. > Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:350 > + var details = { > + "probeId": recordPayload.data.probeId, > + "sampleId": recordPayload.data.sampleId > + }; > + > // Pass the startTime as the endTime since this record type has no duration. > sourceCodeLocation = WebInspector.probeManager.probeForIdentifier(recordPayload.data.probeId).breakpoint.sourceCodeLocation; > - this._addRecord(new WebInspector.ScriptTimelineRecord(WebInspector.ScriptTimelineRecord.EventType.ProbeSampleRecorded, startTime, startTime, callFrames, sourceCodeLocation, recordPayload.data.probeId)); > + this._addRecord(new WebInspector.ScriptTimelineRecord(WebInspector.ScriptTimelineRecord.EventType.ProbeSampleRecorded, startTime, startTime, callFrames, sourceCodeLocation, details)); How about just passing recordPayload.data and not making the duplicate temp object. > Source/WebInspectorUI/UserInterface/Views/DetailsSidebarPanel.js:81 > + reveal: function(objects) What is "objects"? Usage seems to be a single represented object. Perhaps this should just be "object"? > Source/WebInspectorUI/UserInterface/Views/ProbeDetailsSidebarPanel.js:100 > + reveal: function(objects) > + { > + var probeId = objects.details.probeId; This feels dangerous you are assuming the passed object has some properties. You should assert and bail if it is not an expected type. (instanceof check). > Source/WebInspectorUI/UserInterface/Views/ProbeDetailsSidebarPanel.js:103 > + return probeSet.probes.findIndex(function (probe) { return probe.id === probeId; }) !== -1; Style: "function (probe)" => "function(probe)" > Source/WebInspectorUI/UserInterface/Views/ProbeSetDataGrid.js:83 > + node.element.classList.add(WebInspector.ProbeSetDataGrid.HighlightedFrameStyleClassName); > + window.setTimeout(function() { > + node.element.classList.remove(WebInspector.ProbeSetDataGrid.HighlightedFrameStyleClassName); > + }, WebInspector.ProbeSetDataGrid.DataUpdatedAnimationDuration); > + }, Style: "window.setTimeout" => "setTimeout" Should this handle multiple animations gracefully? Something like: if (this._blinkTimeoutIdentifier) clearTimeout(this._blinkTimeoutIdentifier); this._blinkTimeoutIdentifier = setTimeout(function() { ... }, WebInspector.ProbeSetDataGrid.DataUpdatedAnimationDuration); > Source/WebInspectorUI/UserInterface/Views/ProbeSetDetailsSection.js:90 > + var selectedFrame = this._probeSet.dataTable.frames.find(function (frame) { return frame[probeId].sampleId === sampleId; }); Style: "function (frame)" => "function(frame)"

Created attachment 243244 [details] Patch

Comment on attachment 243244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243244&action=review This patch is certainly much better, but now that I think about it some more, I think we may want to change how this works. Details sidebars should automatically show based on whatever is selected. As is the case for selecting a Resource in the timeline. Therefore the Probe sidebar should automatically show when selecting a probe timeline record. If the sidebar is forced to show (like this), then what determines when it should hide or how it will stay shown? The existing mechanism hooks are having the ContentView change its path components or triggering a SupplementalRepresentedObjectsDidChange event. You could make the ScriptTimelineRecord / ProbeSet / Probe a supplemental representedObject and trigger the event. Then make sure that ProbeDetailsSidebarPanel.prototype.inspect recognizes it, and determines that it should show it. Does that sound good? > Source/WebInspectorUI/UserInterface/Views/ProbeDetailsSidebarPanel.js:110 > + return probeSet.probes.findIndex(function(probe) { return probe.id === probeId; }) !== -1; Seems this inner could just be another find, thereby avoiding the comparison to -1. > Source/WebInspectorUI/UserInterface/Views/ProbeSetDataGrid.js:85 > + node.element.classList.remove(WebInspector.ProbeSetDataGrid.HighlightedFrameStyleClassName); Insider here you should: delete this._blinkTimeoutIdentifier; > Source/WebInspectorUI/UserInterface/Views/ProbeSetDetailsSection.js:90 > + var selectedFrame = this._probeSet.dataTable.frames.find(function(frame) { return frame[probeId].sampleId === sampleId; }); Is frame[probeId] guarenteed to exist? > Source/WebInspectorUI/UserInterface/Views/ScriptTimelineView.js:285 > + > + Nit: Extra newline.

(In reply to comment #9) > Comment on attachment 243244 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243244&action=review > > This patch is certainly much better, but now that I think about it some > more, I think we may want to change how this works. > > Details sidebars should automatically show based on whatever is selected. As > is the case for selecting a Resource in the timeline. Therefore the Probe > sidebar should automatically show when selecting a probe timeline record. If > the sidebar is forced to show (like this), then what determines when it > should hide or how it will stay shown? > > The existing mechanism hooks are having the ContentView change its path > components or triggering a SupplementalRepresentedObjectsDidChange event. > You could make the ScriptTimelineRecord / ProbeSet / Probe a supplemental > representedObject and trigger the event. Then make sure that > ProbeDetailsSidebarPanel.prototype.inspect recognizes it, and determines > that it should show it. > > Does that sound good? That is the correct approach. SupplementalRepresentedObjectsDidChange can force the details sidebar open, if it was open before and it had to close automatically because it became empty (from switching views, etc.) This would be similar to how ScopeChainDetailsSidebar works and how TextResourceContentView and ScriptContentView puts the active CallFrame into the array in the supplementalRepresentedObjects getter.