| Summary: | Web Inspector: Timeline: when Network Requests view is selected, in progress requests are absent. | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Wells <jonowells> | ||||||||||||||||
| Component: | Web Inspector | Assignee: | Jonathan Wells <jonowells> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 243994 [details]
[PATCH] Proposed fix.
Created attachment 243995 [details]
[SCREENSHOT] Correct behavior after patch.
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. 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.
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. Created attachment 244228 [details]
[PATCH] Proposed fix, different approach.
This approach was recommended by Timothy.
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. 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? 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. Created attachment 244288 [details]
[PATCH] Proposed fix, adjusted.
Added back lines to OverviewTimelineView.js
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. 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. (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()? (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()? 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. Created attachment 244484 [details]
[PATCH] Proposed fix 3.
Comment on attachment 244484 [details] [PATCH] Proposed fix 3. Clearing flags on attachment: 244484 Committed r178315: <http://trac.webkit.org/changeset/178315> All reviewed patches have been landed. Closing bug. |
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.