Bug 192451

Summary: Web Inspector: subclasses of WI.ClusterContentView don't save/restore content views after the initial view
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
hi: review+, ews-watchlist: commit-queue-
[PATCH] For Landing
none
Archive of layout-test-results from ews114 for mac-sierra none

Description Devin Rousso 2018-12-05 20:37:53 PST
It seems like whatever was selected when it was shown for the first time is the setting that is used.

* STEPS TO REPRODUCE:
1. Inspect any page
2. Go to Timelines
3. Take a recording
4. Click "JavaScript & Events"
5. Switch from "Events" to "Call Trees"
6. Go to a different tab
7. Go back to Timelines
 => "Events" is selected

The same is true for heap snapshots, SVG images, and regular Resources.
Comment 1 Devin Rousso 2018-12-05 20:38:03 PST
*** Bug 190967 has been marked as a duplicate of this bug. ***
Comment 2 Devin Rousso 2018-12-05 20:38:08 PST
*** Bug 192445 has been marked as a duplicate of this bug. ***
Comment 3 Devin Rousso 2018-12-05 21:10:50 PST
I think the reason this is happening is because each `WI.ClusterContentView` (C) has a `WI.ContentViewContainer`, which contains a `_backForwardList` (C list).  The `WI.ClusterContentView` itself is contained within a `WI.ContentViewContainer` (P), meaning that it's parent also has a `_backForwardList` (P list).  When we change views within C, we add a `WI.BackForwardEntry` to C list, but NOT to P list, meaning that when we re-show P after switching tabs, which will contain only 1 `WI.BackForwardEntry` in P list, which was for the original first time that P was shown.

tl;dr: Switching views within the cluster will add history entires to the cluster, but not to it's parent, meaning that the parent will be unaware of how it's child has changed and will default to showing what it knows, which is what the child originally showed.
Comment 4 Devin Rousso 2018-12-06 09:20:53 PST
This is compounded by the fact that when P calls `shown()`, we use the initial `WI.BackForwardEntry` (I) of P to insert a new `WI.BackForwardEntry` (N) of C, meaning that when C calls `shown()` it sees N instead of I and displays that.
Comment 5 Radar WebKit Bug Importer 2018-12-17 21:33:35 PST
<rdar://problem/46800958>
Comment 6 Joseph Pecoraro 2019-01-04 13:42:16 PST
I think ClusterContentViews will probably want to save their state when they hide, otherwise they will be restored incorrectly. I can try a fix like that.
Comment 7 Devin Rousso 2019-01-04 14:03:55 PST
(In reply to Joseph Pecoraro from comment #6)
> I think ClusterContentViews will probably want to save their state when they hide, otherwise they will be restored incorrectly. I can try a fix like that.
The issue I think is more the idea that changes to the `WI.ClusterContentView` (C) are only saved by C itself, not any of its parents.  In the example I gave (comment #3), the main problem was that changes to C didn't propagate to P, which meant that when P was show (P is the "first responder"), it would use the last item in it's list, which was for the original showing of C.

One possible thing we could do is have all `WI.ClusterContentView` somehow propagate their changes to their parent (if any) so that the next time the parent is shown, it has the new state.
Comment 8 Joseph Pecoraro 2019-01-04 14:28:50 PST
Created attachment 358372 [details]
[PATCH] Proposed Fix

Tested this with:

  - JS Timeline - Call Trees / Events - Toggle between tabs after toggling this picker
  - Heap Timelines - Instances / Object Graph - Toggle between tabs after toggling this picker
  - SVG Image view - Image / Source - Toggle between an SVG and PNG after toggling this picker

The back/forward list felt a little messed up in Timelines but it already feels a little confused, so it is no worse. This however is much better default behavior for switching tabs with Timelines.
Comment 9 Devin Rousso 2019-01-04 15:30:56 PST
Comment on attachment 358372 [details]
[PATCH] Proposed Fix

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

r=me, nice fix :)

One thing I just noticed (not sure if this is new) is that switching from "JavaScript Allocations" to "Memory" to "JavaScript & Events" causes the path components of "JavaScript Allocations" to briefly appear before switching to those of "JavaScript & Events".  Probably uses a rAF to update?

> Source/WebInspectorUI/UserInterface/Models/BackForwardEntry.js:97
> +        if (this._contentView.shouldSaveStateOnHide) {

NIT: I'd call this `shouldSaveStateWhenHidden`.  I usually don't like shorter prepositions in names (e.g. using `Did` instead of the past tense).
Comment 10 Joseph Pecoraro 2019-01-04 16:07:09 PST
Created attachment 358390 [details]
[PATCH] For Landing
Comment 11 WebKit Commit Bot 2019-01-04 17:24:00 PST
Comment on attachment 358390 [details]
[PATCH] For Landing

Clearing flags on attachment: 358390

Committed r239649: <https://trac.webkit.org/changeset/239649>
Comment 12 EWS Watchlist 2019-01-04 17:29:58 PST
Comment on attachment 358372 [details]
[PATCH] Proposed Fix

Attachment 358372 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10634375

New failing tests:
http/wpt/css/css-animations/start-animation-001.html
Comment 13 EWS Watchlist 2019-01-04 17:29:59 PST
Created attachment 358404 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6