WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
135965
Web Inspector: Inspector frequently restores wrong view when opened (often Timelines instead of Resource)
https://bugs.webkit.org/show_bug.cgi?id=135965
Summary
Web Inspector: Inspector frequently restores wrong view when opened (often Ti...
Joseph Pecoraro
Reported
2014-08-14 18:07:03 PDT
* SUMMARY Inspector frequently restores wrong view when opened (often Timelines instead of Resources / DOM Tree) * STEPS TO REPRODUCE 1. Inspect bogojoker.com 2. Select Timelines sidebar 3. Select Network timeline 4. Close Inspector 5. Develop > Show Web Inspector (inspector opens to Timelines View, good) 6. Select Resources sidebar 7. Select an Image resource in the sidebar 8. Close Inspector 9. Develop > Show Web Inspector => Inspector opens to Timelines view instead of the images view in Resources sidebar * NOTES - Inspector listens for window "pagehide" events to save the state of the last view you had open. Unfortunately this does not get called until you re-open the inspector. This is way late and we re-ran a restore of the previous, wrong state.
Attachments
[PATCH] Debugging Patch
(11.51 KB, patch)
2014-08-15 13:24 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
[PATCH] Proposed Fix
(15.59 KB, patch)
2014-08-15 15:25 PDT
,
Joseph Pecoraro
timothy
: review-
Details
Formatted Diff
Diff
[PATCH] Proposed Fix
(14.88 KB, patch)
2014-09-08 14:36 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2014-08-14 18:07:33 PDT
<
rdar://problem/18026767
>
Joseph Pecoraro
Comment 2
2014-08-14 18:09:28 PDT
So, we could investigate why pagehide doesn't happen at the right time. But we could also investigate a workaround. For example, always update our cookie whenever the ContentView changes or navigation sidebar changes, instead of relying on pagehide. That kind of stinks, but at least it should fix this issue in common cases.
Joseph Pecoraro
Comment 3
2014-08-14 18:23:05 PDT
A workaround still doesn't seem like enough. There is another actual bug here. It looks like _loadNewRecording is triggering the Timelines panel to be the selected panel:
> Resources/Views/Sidebar.js:143:20: CONSOLE LOG setting timeline > Resources/Views/Sidebar.js:144:22: CONSOLE TRACE > 0: selectedSidebarPanel(Resources/Views/Sidebar.js:144:22) > 1: _contentBrowserCurrentContentViewDidChange(Resources/Base/Main.js:1096:31) > 2: dispatch(Resources/Base/Object.js:141:55) > 3: dispatchEventToListeners(Resources/Base/Object.js:148:17) > 4: _currentContentViewDidChange(Resources/Views/ContentBrowser.js:509:38) > 5: dispatch(Resources/Base/Object.js:141:55) > 6: dispatchEventToListeners(Resources/Base/Object.js:148:17) > 7: showBackForwardEntryForIndex(Resources/Views/ContentViewContainer.js:207:38) > 8: showContentView(Resources/Views/ContentViewContainer.js:179:42) > 9: showContentView(Resources/Views/ContentBrowser.js:172:58) > 10: _recordingLoaded(Resources/Views/TimelineSidebarPanel.js:436:52) > 11: dispatch(Resources/Base/Object.js:141:55) > 12: dispatchEventToListeners(Resources/Base/Object.js:148:17) > 13: _loadNewRecording(Resources/Controllers/TimelineManager.js:421:38) > 14: delayedWork(Resources/Controllers/TimelineManager.js:44:31) > 15: delayedWork([native code])
Joseph Pecoraro
Comment 4
2014-08-14 18:57:45 PDT
(In reply to
comment #3
)
> A workaround still doesn't seem like enough. There is another actual bug here. > > It looks like _loadNewRecording is triggering the Timelines panel to be the selected panel:
Ugh, and even fixing that we run into another issue. 1. Open inspector 2. Switch to Timelines sidebar (puts a TimelineContentView into back forward history) 3. Switch to Resources sidebar 4. Select any resource (puts a ResourceView into back forward history) 5. Cmd+R to reload => ends up with Timelines sidebar selected So again, looking at a Resource in the Resources sidebar, suddenly we get the Resource in Timelines. The issue here is because of the back forward history: - ResourceViews are torn down => last valid view in the back forward list is selected, that is the TimelineContentView => timelines sidebar becomes selected - Page loads the old resource - Resource is restored => state restoration checks if timelines is a valid sidebar for this resource, it is. We can't really fix one of these issues without fixing all of them. Each is easy to reproduce.
Joseph Pecoraro
Comment 5
2014-08-15 13:24:28 PDT
Created
attachment 236673
[details]
[PATCH] Debugging Patch
Joseph Pecoraro
Comment 6
2014-08-15 15:25:53 PDT
Created
attachment 236678
[details]
[PATCH] Proposed Fix
Timothy Hatcher
Comment 7
2014-08-19 11:20:00 PDT
Comment on
attachment 236678
[details]
[PATCH] Proposed Fix View in context:
https://bugs.webkit.org/attachment.cgi?id=236678&action=review
> Source/WebInspectorUI/UserInterface/Base/Main.js:871 > WebInspector._pageHidden = function(event) > { > - this._updateCookieForInspectorViewState(); > + // FIXME: pagehide events are not firing at the correct time. > + // Ignore the event for now, and instead update view state > + // every time the ContentView changes instead of only when we close. > + // this._updateCookieForInspectorViewState(); > }
Unregister the event, remove the function and just file a bug?
> Source/WebInspectorUI/UserInterface/Base/Main.js:926 > + // FIXME: Selecting a CallFrameTreeElement selects the resource, does the code below work?
Good catch. For the Debugger sidebar panel, which has two content tree outlines, selectedSidebarPanel.contentTreeOutline.processingSelectionChange is the wrong check. The check should iterate over _visibleContentTreeOutlines and look for processingSelectionChange on any of them.
> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:684 > + if (!this._pendingViewStateCookie) { > + if (this._viewStateCookieMatchingRepresentedObject && !Array.isArray(treeElements) && treeElements.representedObject === this._viewStateCookieMatchingRepresentedObject) { > + treeElements.revealAndSelect(); > + delete this._viewStateCookieMatchingRepresentedObject; > + } > return; > + }
We talked about a better fox for this in FrameTreeElement.
> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:735 > + // FIXME: Due to the order of tree elements getting inserted and onpopulate > + // getting called on the FrameTreeElement, this tree element could get > + // immediately removed and re-inserted. At which point, it will not > + // get its selected state restored. This is a workaround for that particular > + // chain of events. We should fix this in a cleaner way. > + this._viewStateCookieMatchingRepresentedObject = matchedElement.representedObject;
Ditto.
> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:741 > + delete this._viewStateCookieMatchingRepresentedObject;
Ditto.
Joseph Pecoraro
Comment 8
2014-09-08 14:25:37 PDT
(In reply to
comment #7
)
> (From update of
attachment 236678
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=236678&action=review
> > > Source/WebInspectorUI/UserInterface/Base/Main.js:871 > > WebInspector._pageHidden = function(event) > > { > > - this._updateCookieForInspectorViewState(); > > + // FIXME: pagehide events are not firing at the correct time. > > + // Ignore the event for now, and instead update view state > > + // every time the ContentView changes instead of only when we close. > > + // this._updateCookieForInspectorViewState(); > > } > > Unregister the event, remove the function and just file a bug?
Hmm, interestingly this is working for me now. I decided to keep this and NOT update view state on every content view change. Things are working well for now.
> > Source/WebInspectorUI/UserInterface/Base/Main.js:926 > > + // FIXME: Selecting a CallFrameTreeElement selects the resource, does the code below work? > > Good catch. For the Debugger sidebar panel, which has two content tree outlines, selectedSidebarPanel.contentTreeOutline.processingSelectionChange is the wrong check.
I'll take a look at this now.
> The check should iterate over _visibleContentTreeOutlines and look for processingSelectionChange on any of them. > > > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:684 > > + if (!this._pendingViewStateCookie) { > > + if (this._viewStateCookieMatchingRepresentedObject && !Array.isArray(treeElements) && treeElements.representedObject === this._viewStateCookieMatchingRepresentedObject) { > > + treeElements.revealAndSelect(); > > + delete this._viewStateCookieMatchingRepresentedObject; > > + } > > return; > > + } > > We talked about a better fox for this in FrameTreeElement.
Yep, this is much nicer. It does less work too.
Joseph Pecoraro
Comment 9
2014-09-08 14:36:39 PDT
Created
attachment 237811
[details]
[PATCH] Proposed Fix Addresses Tim's comments. Also fixes the CallFrame issue, good suggestion Tim!
WebKit Commit Bot
Comment 10
2014-09-08 17:02:22 PDT
Comment on
attachment 237811
[details]
[PATCH] Proposed Fix Clearing flags on attachment: 237811 Committed
r173407
: <
http://trac.webkit.org/changeset/173407
>
WebKit Commit Bot
Comment 11
2014-09-08 17:02:27 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 12
2014-09-08 17:09:28 PDT
Yay!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug