Bug 132073 – Web Inspector: probe sample should be revealed in details sidebar when selected in Timelines panel

• Component: Web Inspector
• Status: NEW

[Brian Burg]

I think the way to go is to add a supplementalRepresentedObject(s) parameter to showSourceCodeLocation(). When called, it will show the source code location, then ask the active toolbars if they can show the supplemental object. In this case, the supplemental object is a WebInspector.ProbeSample or a dummy object with ProbeSet+hitcount. The sidebar can check if the sample is in any of the probe sets and reveal the related ProbeSetDataFrame and its view (table row).

[Radar WebKit Bug Importer]

<rdar://problem/16703731>

[Timothy Hatcher]

Sounds right.

[Katie Madonna]

Created attachment 232977 [details]
Patch

[Timothy Hatcher]

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.

[Brian Burg]

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

[Katie Madonna]

Created attachment 242692 [details]
Patch

[Joseph Pecoraro]

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)"

[Katie Madonna]

Created attachment 243244 [details]
Patch

[Joseph Pecoraro]

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.

[Timothy Hatcher]

(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.