Bug 192451 – Web Inspector: subclasses of WI.ClusterContentView don't save/restore content views after the initial view

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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.

[Devin Rousso]

*** Bug 190967 has been marked as a duplicate of this bug. ***

[Devin Rousso]

*** Bug 192445 has been marked as a duplicate of this bug. ***

[Devin Rousso]

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.

[Devin Rousso]

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.

[Radar WebKit Bug Importer]

<rdar://problem/46800958>

[Joseph Pecoraro]

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.

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Devin Rousso]

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

[Joseph Pecoraro]

Created attachment 358390 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 358390 [details]
[PATCH] For Landing

Clearing flags on attachment: 358390

Committed r239649: <https://trac.webkit.org/changeset/239649>

[EWS Watchlist]

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

[EWS Watchlist]

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