Bug 140090 – Web Inspector: Timeline: when Network Requests view is selected, in progress requests are absent.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonathan Wells]

Created attachment 243991 [details]
[SCREENSHOT] Incorrect behavior.

When a network request is in progress and the timeline is recording, the request shows in the table if the combined (default) view state is active. However if Network Requests is selected, the request will not appear until/unless the request finishes.

[Radar WebKit Bug Importer]

<rdar://problem/19377211>

[Jonathan Wells]

Created attachment 243994 [details]
[PATCH] Proposed fix.

[Jonathan Wells]

Created attachment 243995 [details]
[SCREENSHOT] Correct behavior after patch.

[Joseph Pecoraro]

Comment on attachment 243994 [details]
[PATCH] Proposed fix.

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:134
> +        WebInspector.timelineSidebarPanel.updateFilter();

updateFilter is a big hammer. Putting and updateFilter in updateLayout, means the filter compares every tree element on every rAF. I wonder if there is a more performant way to fix this. We don't need to constantly check every request.

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:138
>          this._processPendingRecords();

I would expect something along the lines of _processPendingRecords, e.g. a new _updateOngoingRequests which would make something visible if an ongoing request enters the selected time range.

We could keep a small list of all the ongoing network requests, and just iterate through that list in each updateLayout to see if we should show a record or not.

[Jonathan Wells]

Created attachment 244127 [details]
[PATCH] Proposed fix 2.

Updated patch with a drive-by fix. Leaving the code as is per conversation with Joe and per the pattern set up in OverviewTimelineView.js. Will look to optimize in the future.

[Timothy Hatcher]

Comment on attachment 244127 [details]
[PATCH] Proposed fix 2.

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineView.js:134
> +        WebInspector.timelineSidebarPanel.updateFilter();

This still seems heavy handed. Can you try hooking up this to a new event listener? Yes, OverviewTimelineView does it, but only if the current time changed, not during any other layout. updateLayout is called in many other cases, like resizing the Inspector. And calling updateFilter during those cases would be wrong and cause slowness in the UI.

Listen for WebInspector.Timeline.Event.TimesUpdated in the NetworkTimelineView constructor (next to the Event.RecordAdded listener). I suspect that will fire when we need to update the filters. Only the network timeline has dynamic timeline records, so it is the only one that needs to listen for that event.

Maybe even set a temp property flag in the TimesUpdated listener, that causes updateLayout to call updateFilter() next time. That way the updateFilter() happens during layout and not counter to the layout.

[Jonathan Wells]

Created attachment 244228 [details]
[PATCH] Proposed fix, different approach.

This approach was recommended by Timothy.

[Timothy Hatcher]

Comment on attachment 244228 [details]
[PATCH] Proposed fix, different approach.

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

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);

We need to find a new home for this code.

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:369
> +        WebInspector.timelineSidebarPanel.updateFilter();

My gut says this should happen before the updateLayoutIfNeeded calls.

[Jonathan Wells]

Comment on attachment 244228 [details]
[PATCH] Proposed fix, different approach.

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

>> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
>> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);
> 
> We need to find a new home for this code.

I'm wondering if this shouldn't just stay put, but with the updateFilter() line removed?

[Jonathan Wells]

Comment on attachment 244228 [details]
[PATCH] Proposed fix, different approach.

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

>>> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
>>> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);
>> 
>> We need to find a new home for this code.
> 
> I'm wondering if this shouldn't just stay put, but with the updateFilter() line removed?

Actually I think the whole piece should probably stay. I'll submit with this back in place.

[Jonathan Wells]

Created attachment 244288 [details]
[PATCH] Proposed fix, adjusted.

Added back lines to OverviewTimelineView.js

[Timothy Hatcher]

Comment on attachment 244228 [details]
[PATCH] Proposed fix, different approach.

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

>>>> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
>>>> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);
>>> 
>>> We need to find a new home for this code.
>> 
>> I'm wondering if this shouldn't just stay put, but with the updateFilter() line removed?
> 
> Actually I think the whole piece should probably stay. I'll submit with this back in place.

Then updateFilter would happen twice. This updateLayout is happening because of the time change in _updateTimes, where you added updateFilter below.

The filtering can hide or show the selected row. In that case we need to update the navigation bar selection. I'm thinking updateFilter() should do that itself.

[Timothy Hatcher]

Comment on attachment 244288 [details]
[PATCH] Proposed fix, adjusted.

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

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:365
> +        WebInspector.timelineSidebarPanel.updateFilter();

OverviewTimelineView will do two updateFilter calls now. See previous comment.

[Jonathan Wells]

(In reply to comment #12)
> Comment on attachment 244228 [details]
> [PATCH] Proposed fix, different approach.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244228&action=review
> 
> >>>> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
> >>>> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);
> >>> 
> >>> We need to find a new home for this code.
> >> 
> >> I'm wondering if this shouldn't just stay put, but with the updateFilter() line removed?
> > 
> > Actually I think the whole piece should probably stay. I'll submit with this back in place.
> 
> Then updateFilter would happen twice. This updateLayout is happening because
> of the time change in _updateTimes, where you added updateFilter below.
> 
> The filtering can hide or show the selected row. In that case we need to
> update the navigation bar selection. I'm thinking updateFilter() should do
> that itself.

That works. What is the best approach to dispatch the SelectionPathComponentsDidChange event properly from the OverviewTimelineView? ContentView#showContentViewForRepresentedObject()?

[Jonathan Wells]

(In reply to comment #12)
> Comment on attachment 244228 [details]
> [PATCH] Proposed fix, different approach.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244228&action=review
> 
> >>>> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
> >>>> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);
> >>> 
> >>> We need to find a new home for this code.
> >> 
> >> I'm wondering if this shouldn't just stay put, but with the updateFilter() line removed?
> > 
> > Actually I think the whole piece should probably stay. I'll submit with this back in place.
> 
> Then updateFilter would happen twice. This updateLayout is happening because
> of the time change in _updateTimes, where you added updateFilter below.
> 
> The filtering can hide or show the selected row. In that case we need to
> update the navigation bar selection. I'm thinking updateFilter() should do
> that itself.

How best can I accurately gather whether the selectedTreeElement has changed to hidden inside updateFilter()?

[Timothy Hatcher]

Comment on attachment 244228 [details]
[PATCH] Proposed fix, different approach.

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

>>>>>>> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:-123
>>>>>>> -                this.dispatchEventToListeners(WebInspector.TimelineView.Event.SelectionPathComponentsDidChange);
>>>>>> 
>>>>>> We need to find a new home for this code.
>>>>> 
>>>>> I'm wondering if this shouldn't just stay put, but with the updateFilter() line removed?
>>>> 
>>>> Actually I think the whole piece should probably stay. I'll submit with this back in place.
>>> 
>>> Then updateFilter would happen twice. This updateLayout is happening because of the time change in _updateTimes, where you added updateFilter below.
>>> 
>>> The filtering can hide or show the selected row. In that case we need to update the navigation bar selection. I'm thinking updateFilter() should do that itself.
>> 
>> That works. What is the best approach to dispatch the SelectionPathComponentsDidChange event properly from the OverviewTimelineView? ContentView#showContentViewForRepresentedObject()?
> 
> How best can I accurately gather whether the selectedTreeElement has changed to hidden inside updateFilter()?

Check selectedTreeElement && selectedTreeElement.hidden before doing the filtering, like this code did.

[Jonathan Wells]

Created attachment 244484 [details]
[PATCH] Proposed fix 3.

[WebKit Commit Bot]

Comment on attachment 244484 [details]
[PATCH] Proposed fix 3.

Clearing flags on attachment: 244484

Committed r178315: <http://trac.webkit.org/changeset/178315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.