Web Inspector: Inspector frequently restores wrong view when opened (often Timelines...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Sep 2014 00:02:02 +0000 (00:02 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Sep 2014 00:02:02 +0000 (00:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135965

Patch by Joseph Pecoraro <pecoraro@apple.com> on 2014-09-08
Reviewed by Timothy Hatcher.

There were numerous subtle race conditions in state restoration.
This patch intends to fix a number of them to get what feels
like sane behavior for selected sidebar state restoration.

1. Starting a Timeline recording no longer automatically switches to the TimelineContentView.
This was making every reload switch to Timelines. If you had
a resource selected (e.g. the DOM Tree) we should go back
to showing the DOM tree.

2. Background sidebars should not reveal and select tree elements.
This was making resources get selected in Timelines when reloading the page
because the background Resources sidebar was restoring selection of the resource.
That is an unexpected selection and breaks the experience of Timelines.

3. ContentView changes during page navigation / reload should not be saved, and improve Resource restoration.
If a TimelineContentView was in the ContentBrowser back/forward history,
a reload with a resource selected in the Resources sidebar would end up
showing the Timelines sidebar. This was because when ContentViews are
closed during the navigation, the ContentBrowser would fall back to the
remaining TimelineContentView and switch to its only allowed sidebar
(Timelines). ResourceSidebarPanel, knowing a resource was selected,
would select the MainFrame intending to stay in the Resource sidebar,
but the resource is okay with showing in any sidebar, so we stay on Timelines.

4. Resource sidebar state restoration should propertly restore selection.
On reload, state restoration would know the resource to re-select in the
Resources sidebar. As ResourceTreeElements are added to the sidebar
they are checked against the state restoration cookie. If they match,
they would select the element and delete the cookie. Unfortunately,
if this was the first TreeElement child being added to a FrameTreeElement,
the FrameTreeElement onpopulate would remove all children and re-add
them in a unique way. Unfortunately this means the TreeElement we
selected based on the cookie, immediately got thrown away and recreated,
and we no longer reveal and select it. This is a special case for
FrameTreeElements which use the onpopulate SPI. So, instead of starting
processing the new element queue, if this is the first time just trigger
the onpopulate and elements are made just once.

5. Show Error Console triggering early, could have unexpected sidebar behavior.
Opening Web Inspector directly to the console can run before
WebInspector.contentLoaded (DOMContentLoaded). So in that case
WebInspector.showFullHeightConsole was not handling if the contentBrowser
had no content view yet, and the sidebar might be-reopened later on
in contentLoaded based on the setting value.

6. Improve automatic resource selection for sidebars with multiple tree outlines.
Selecting a call frame tree element was unexpectedly changing the
selection to a Resource where the breakpoint was set. This was
because we were only looking at one of potentially many content
tree outlines in the sidebar to see if there was a user action.

* UserInterface/Base/Main.js:
(WebInspector.contentLoaded):
(WebInspector.showFullHeightConsole):
(WebInspector.toggleConsoleView):
(WebInspector._mainResourceDidChange):
(WebInspector._provisionalLoadStarted):
(WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar):
(WebInspector._updateCookieForInspectorViewState):
(WebInspector._contentBrowserCurrentContentViewDidChange):
* UserInterface/Views/NavigationSidebarPanel.js:
(WebInspector.NavigationSidebarPanel.prototype._treeElementAddedOrChanged):
* UserInterface/Views/ResourceSidebarPanel.js:
(WebInspector.ResourceSidebarPanel.prototype._mainFrameMainResourceDidChange.delayedWork):
(WebInspector.ResourceSidebarPanel.prototype._mainFrameMainResourceDidChange):
* UserInterface/Views/TimelineSidebarPanel.js:
(WebInspector.TimelineSidebarPanel.prototype._recordingLoaded):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@173407 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Base/Main.js
Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js
Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js
Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js
Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js

index 4548593..2c812c6 100644 (file)
@@ -1,3 +1,78 @@
+2014-09-08  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: Inspector frequently restores wrong view when opened (often Timelines instead of Resource)
+        https://bugs.webkit.org/show_bug.cgi?id=135965
+
+        Reviewed by Timothy Hatcher.
+
+        There were numerous subtle race conditions in state restoration.
+        This patch intends to fix a number of them to get what feels
+        like sane behavior for selected sidebar state restoration.
+
+        1. Starting a Timeline recording no longer automatically switches to the TimelineContentView.
+        This was making every reload switch to Timelines. If you had
+        a resource selected (e.g. the DOM Tree) we should go back
+        to showing the DOM tree.
+
+        2. Background sidebars should not reveal and select tree elements.
+        This was making resources get selected in Timelines when reloading the page
+        because the background Resources sidebar was restoring selection of the resource.
+        That is an unexpected selection and breaks the experience of Timelines.
+
+        3. ContentView changes during page navigation / reload should not be saved, and improve Resource restoration.
+        If a TimelineContentView was in the ContentBrowser back/forward history,
+        a reload with a resource selected in the Resources sidebar would end up
+        showing the Timelines sidebar. This was because when ContentViews are
+        closed during the navigation, the ContentBrowser would fall back to the
+        remaining TimelineContentView and switch to its only allowed sidebar
+        (Timelines). ResourceSidebarPanel, knowing a resource was selected,
+        would select the MainFrame intending to stay in the Resource sidebar,
+        but the resource is okay with showing in any sidebar, so we stay on Timelines.
+
+        4. Resource sidebar state restoration should propertly restore selection.
+        On reload, state restoration would know the resource to re-select in the
+        Resources sidebar. As ResourceTreeElements are added to the sidebar
+        they are checked against the state restoration cookie. If they match,
+        they would select the element and delete the cookie. Unfortunately,
+        if this was the first TreeElement child being added to a FrameTreeElement,
+        the FrameTreeElement onpopulate would remove all children and re-add
+        them in a unique way. Unfortunately this means the TreeElement we
+        selected based on the cookie, immediately got thrown away and recreated,
+        and we no longer reveal and select it. This is a special case for
+        FrameTreeElements which use the onpopulate SPI. So, instead of starting
+        processing the new element queue, if this is the first time just trigger
+        the onpopulate and elements are made just once.
+
+        5. Show Error Console triggering early, could have unexpected sidebar behavior.
+        Opening Web Inspector directly to the console can run before
+        WebInspector.contentLoaded (DOMContentLoaded). So in that case
+        WebInspector.showFullHeightConsole was not handling if the contentBrowser
+        had no content view yet, and the sidebar might be-reopened later on
+        in contentLoaded based on the setting value.
+
+        6. Improve automatic resource selection for sidebars with multiple tree outlines.
+        Selecting a call frame tree element was unexpectedly changing the
+        selection to a Resource where the breakpoint was set. This was
+        because we were only looking at one of potentially many content
+        tree outlines in the sidebar to see if there was a user action.
+        
+        * UserInterface/Base/Main.js:
+        (WebInspector.contentLoaded):
+        (WebInspector.showFullHeightConsole):
+        (WebInspector.toggleConsoleView):
+        (WebInspector._mainResourceDidChange):
+        (WebInspector._provisionalLoadStarted):
+        (WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar):
+        (WebInspector._updateCookieForInspectorViewState):
+        (WebInspector._contentBrowserCurrentContentViewDidChange):
+        * UserInterface/Views/NavigationSidebarPanel.js:
+        (WebInspector.NavigationSidebarPanel.prototype._treeElementAddedOrChanged):
+        * UserInterface/Views/ResourceSidebarPanel.js:
+        (WebInspector.ResourceSidebarPanel.prototype._mainFrameMainResourceDidChange.delayedWork):
+        (WebInspector.ResourceSidebarPanel.prototype._mainFrameMainResourceDidChange):
+        * UserInterface/Views/TimelineSidebarPanel.js:
+        (WebInspector.TimelineSidebarPanel.prototype._recordingLoaded):
+
 2014-09-08  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: Add layout test for lowercase CSSProperty names
index f55af47..0d2b878 100644 (file)
@@ -346,6 +346,8 @@ WebInspector.contentLoaded = function()
 
     if (this._showingSplitConsoleSetting.value)
         this.showSplitConsole();
+
+    this._contentLoaded = true;
 }
 
 WebInspector.sidebarPanelForCurrentContentView = function()
@@ -610,7 +612,7 @@ WebInspector.showFullHeightConsole = function(scope)
 
     this.consoleContentView.scopeBar.item(scope).selected = true;
 
-    if (this.contentBrowser.currentContentView !== this.consoleContentView) {
+    if (!this.contentBrowser.currentContentView || this.contentBrowser.currentContentView !== this.consoleContentView) {
         this._wasShowingNavigationSidebarBeforeFullHeightConsole = !this.navigationSidebar.collapsed;
 
         // Collapse the sidebar before showing the console view, so the check for the collapsed state in
@@ -618,6 +620,11 @@ WebInspector.showFullHeightConsole = function(scope)
         // tree elements in the current sidebar.
         this.navigationSidebar.collapsed = true;
 
+        // If this is before the content has finished loading update the collapsed value setting
+        // ourselves so that we don't uncollapse the navigation sidebar when it is loaded.
+        if (!this._contentLoaded)
+            this._navigationSidebarCollapsedSetting.value = true;
+
         // Be sure to close any existing log view in the split content browser before showing it in the
         // main content browser. We can only show a content view in one browser at a time.
         this.splitContentBrowser.contentViewContainer.closeAllContentViewsOfPrototype(WebInspector.LogContentView);
@@ -645,8 +652,11 @@ WebInspector.toggleConsoleView = function()
     if (this.isShowingConsoleView()) {
         if (this.contentBrowser.canGoBack())
             this.contentBrowser.goBack();
-        else
+        else {
+            if (!this.navigationSidebar.selectedSidebarPanel)
+                this.navigationSidebar.selectedSidebarPanel = this.resourceSidebarPanel;
             this.resourceSidebarPanel.showDefaultContentView();
+        }
 
         if (this._wasShowingNavigationSidebarBeforeFullHeightConsole)
             this.navigationSidebar.collapsed = false;
@@ -762,6 +772,8 @@ WebInspector._mainResourceDidChange = function(event)
     if (!event.target.isMainFrame())
         return;
 
+    this._inProvisionalLoad = false;
+
     this._restoreInspectorViewStateFromCookie(this._lastInspectorViewStateCookieSetting.value, true);
 
     this.updateWindowTitle();
@@ -773,6 +785,8 @@ WebInspector._provisionalLoadStarted = function(event)
         return;
 
     this._updateCookieForInspectorViewState();
+
+    this._inProvisionalLoad = true;
 }
 
 WebInspector._windowFocused = function(event)
@@ -893,11 +907,13 @@ WebInspector._revealAndSelectRepresentedObjectInNavigationSidebar = function(rep
     if (!selectedSidebarPanel)
         return;
 
-    // If the tree outline is processing a selection currently then we can assume the selection does not
-    // need to be changed. This is needed to allow breakpoints tree elements to be selected without jumping
-    // back to selecting the resource tree element.
-    if (selectedSidebarPanel.contentTreeOutline.processingSelectionChange)
-        return;
+    // If a tree outline is processing a selection currently then we can assume the selection does not
+    // need to be changed. This is needed to allow breakpoint and call frame tree elements to be selected
+    // without jumping back to selecting the resource tree element.
+    for (var contentTreeOutline of selectedSidebarPanel.visibleContentTreeOutlines) {
+        if (contentTreeOutline.processingSelectionChange)
+            return;
+    }
 
     var treeElement = selectedSidebarPanel.treeElementForRepresentedObject(representedObject);
 
@@ -1031,6 +1047,12 @@ WebInspector._updateCookieForInspectorViewState = function()
         return;
     }
 
+    // Ignore saving the sidebar state for provisional loads. The currently selected sidebar
+    // may have been the result of content views closing as a result of a page navigation,
+    // but those content views may come back very soon.
+    if (this._inProvisionalLoad)
+        return;
+
     var selectedSidebarPanel = this.navigationSidebar.selectedSidebarPanel;
     if (!selectedSidebarPanel)
         return;
index 0aede89..867faf3 100644 (file)
@@ -304,6 +304,12 @@ WebInspector.FrameTreeElement.prototype = {
 
     _populateFromNewChildQueue: function()
     {
+        if (!this.children.length) {
+            this._updateParentStatus();
+            this.shouldRefreshChildren = true;
+            return;
+        }
+
         for (var i = 0; i < this._newChildQueue.length; ++i)
             this._addChildForRepresentedObject(this._newChildQueue[i]);
 
index ba5c8e8..ce3ad4f 100644 (file)
@@ -147,6 +147,11 @@ WebInspector.NavigationSidebarPanel.prototype = {
         return this._contentTreeOutline;
     },
 
+    get visibleContentTreeOutlines()
+    {
+        return this._visibleContentTreeOutlines;
+    },
+
     get hasSelectedElement()
     {
         return !!this._contentTreeOutline.selectedTreeElement;
@@ -533,7 +538,9 @@ WebInspector.NavigationSidebarPanel.prototype = {
 
         this._checkForEmptyFilterResults();
         this._updateContentOverflowShadowVisibilitySoon();
-        this._checkElementsForPendingViewStateCookie(treeElement);
+
+        if (this.selected)
+            this._checkElementsForPendingViewStateCookie(treeElement);
     },
 
     _treeElementExpandedOrCollapsed: function(treeElement)
@@ -709,7 +716,7 @@ WebInspector.NavigationSidebarPanel.prototype = {
         }, this);
 
         if (matchedElement) {
-            matchedElement.revealAndSelect(true, false);
+            matchedElement.revealAndSelect();
 
             delete this._pendingViewStateCookie;
 
index 81da79d..b43c02c 100644 (file)
@@ -553,9 +553,12 @@ WebInspector.ResourceSidebarPanel.prototype = {
 
     _mainFrameMainResourceDidChange: function(event)
     {
+        var wasShowingResourceSidebar = this.selected;
         var currentContentView = WebInspector.contentBrowser.currentContentView;
         var wasShowingResourceContentView = currentContentView instanceof WebInspector.ResourceContentView
-            || currentContentView instanceof WebInspector.FrameContentView || currentContentView instanceof WebInspector.ScriptContentView;
+            || currentContentView instanceof WebInspector.ResourceClusterContentView
+            || currentContentView instanceof WebInspector.FrameContentView
+            || currentContentView instanceof WebInspector.ScriptContentView;
 
         // Close all resource and frame content views since the main frame has navigated and all resources are cleared.
         WebInspector.contentBrowser.contentViewContainer.closeAllContentViewsOfPrototype(WebInspector.ResourceClusterContentView);
@@ -565,9 +568,18 @@ WebInspector.ResourceSidebarPanel.prototype = {
         function delayedWork()
         {
             // Show the main frame since there is no content view showing or we were showing a resource before.
-            // FIXME: We could try to select the same resource that was selected before in the case of a reload.
-            if (!WebInspector.contentBrowser.currentContentView || wasShowingResourceContentView)
+            // Cookie restoration will attempt to re-select the resource we were showing.
+            if (!WebInspector.contentBrowser.currentContentView || wasShowingResourceContentView) {
+                // If we were showing a resource inside of the ResourceSidebar, we should
+                // re-show the resource inside of the resource sidebar. It is possible that
+                // the sidebar panel could have switched to another view in the back-forward list.
+                if (wasShowingResourceSidebar)
+                    WebInspector.navigationSidebar.selectedSidebarPanel = this;
+
+                // NOTE: This selection, during provisional loading, won't be saved, so it is
+                // safe to do and not-clobber cookie restoration.
                 this._mainFrameTreeElement.revealAndSelect(true, false);
+            }
         }
 
         // Delay this work because other listeners of this event might not have fired yet. So selecting the main frame
index 27dfdc9..c57add6 100644 (file)
@@ -417,7 +417,9 @@ WebInspector.TimelineSidebarPanel.prototype = {
     _recordingLoaded: function()
     {
         this._activeContentView = WebInspector.contentBrowser.contentViewForRepresentedObject(WebInspector.timelineManager.activeRecording);
-        WebInspector.contentBrowser.showContentView(this._activeContentView);
+
+        if (this.selected)
+            WebInspector.contentBrowser.showContentView(this._activeContentView);
     },
 
     _recordGlyphMousedOver: function(event)