Bug 145080 - Web Inspector: Tab Restoration incorrectly makes ContentViews "shown" in background tabs
Summary: Web Inspector: Tab Restoration incorrectly makes ContentViews "shown" in back...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-15 16:21 PDT by Joseph Pecoraro
Modified: 2015-11-20 13:31 PST (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (9.08 KB, patch)
2015-05-15 17:35 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (9.08 KB, patch)
2015-05-15 17:36 PDT, Joseph Pecoraro
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (778.17 KB, application/zip)
2015-05-15 18:38 PDT, Build Bot
no flags Details
[PATCH] For Landing (9.00 KB, patch)
2015-05-18 12:04 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-05-15 16:21:58 PDT
* SUMMARY
Tab Restoration incorrectly makes ContentViews "shown" in background tabs.

There seem to a be a couple issues with tab restoration that may affect performance.

  - Tab state restoration triggers "shown" twice on the current content view per-tab'
    => this seems like wasted work

  - Tab state restoration leaves ContentViews as "visible" / "shown" in background tabs
    => if Type Annotators are enabled, this can mean constant ongoing polling, potentially for multiple editors in multiple tabs

* STEPS TO REPRODUCE
1. Inspect <http://bogojoker.com/shell/>
2. In Debugger Tab select the "easySlider.min.js" resource => creates a SourceCodeTextEditor
3. Enable the Type Profiler => enables Type Annotations
4. Switch to the Elements Tab => Type Annotations paused
5. Close Inspector => saves state of tabs
6. Open Inspector
  => Unexpectedly, the background Debugger tab's SourceCodeTextEditor had its "shown" called twice
  => Unexpectedly, the background Debugger tab's SourceCodeTextEditor is cluttering the protocol with type annotation updates.

* NOTES
- Duplicate calls to shown:

    Views/SourceCodeTextEditor.js:100:20: CONSOLE LOG SourceCodeTextEditor.shown http://bogojoker.com/shell/
    Views/SourceCodeTextEditor.js:101:22: CONSOLE TRACE
    0: shown(Views/SourceCodeTextEditor.js:101:22)
    1: shown(Views/TextResourceContentView.js:104:31)
    2: prepareToShow(Models/BackForwardEntry.js:61:35)
    3: _showEntry(Views/ContentViewContainer.js:472:28)
    4: showBackForwardEntryForIndex(Views/ContentViewContainer.js:212:28)
    5: showContentView(Views/ContentViewContainer.js:188:42)
    6: _showContentViewForIdentifier(Views/ResourceClusterContentView.js:236:57)
    7: restoreFromCookie(Views/ResourceClusterContentView.js:147:61)
    8: _restoreFromCookie(Models/BackForwardEntry.js:78:43)
    9: prepareToShow(Models/BackForwardEntry.js:57:32)
    10: _showEntry(Views/ContentViewContainer.js:472:28)
    11: showBackForwardEntryForIndex(Views/ContentViewContainer.js:212:28)
    12: showContentView(Views/ContentViewContainer.js:188:42)
    13: showContentViewForRepresentedObject(Views/ContentBrowser.js:168:58)
    14: showDefaultContentViewForTreeElement(Views/NavigationSidebarPanel.js:196:64)
    15: delayedWork(Views/ResourceSidebarPanel.js:192:58)
    16: delayedWork([native code])

    Views/SourceCodeTextEditor.js:100:20: CONSOLE LOG SourceCodeTextEditor.shown http://bogojoker.com/shell/
    Views/SourceCodeTextEditor.js:101:22: CONSOLE TRACE
    0: shown(Views/SourceCodeTextEditor.js:101:22)
    1: shown(Views/TextResourceContentView.js:104:31)
    2: prepareToShow(Models/BackForwardEntry.js:61:35)
    3: _showEntry(Views/ContentViewContainer.js:472:28)
    4: shown(Views/ContentViewContainer.js:425:24)
    5: shown(Views/ClusterContentView.js:75:41)
    6: shown(Views/ResourceClusterContentView.js:125:61)
    7: prepareToShow(Models/BackForwardEntry.js:61:35)
    8: _showEntry(Views/ContentViewContainer.js:472:28)
    9: showBackForwardEntryForIndex(Views/ContentViewContainer.js:212:28)
    10: showContentView(Views/ContentViewContainer.js:188:42)
    11: showContentViewForRepresentedObject(Views/ContentBrowser.js:168:58)
    12: showDefaultContentViewForTreeElement(Views/NavigationSidebarPanel.js:196:64)
    13: delayedWork(Views/ResourceSidebarPanel.js:192:58)
    14: delayedWork([native code])
Comment 1 Radar WebKit Bug Importer 2015-05-15 16:22:45 PDT
<rdar://problem/20983950>
Comment 2 Joseph Pecoraro 2015-05-15 16:28:15 PDT
Oops, I pasted the duplicate shown for the Resources tab (again, unexpected). But specifically this was the trace for the Debugger tab:

    Views/SourceCodeTextEditor.js:100:20: CONSOLE LOG SourceCodeTextEditor.shown http://bogojoker.com/shell/js/easySlider.min.js
    Views/SourceCodeTextEditor.js:101:22: CONSOLE TRACE
    0: shown(Views/SourceCodeTextEditor.js:101:22)
    1: shown(Views/TextResourceContentView.js:104:31)
    2: prepareToShow(Models/BackForwardEntry.js:61:35)
    3: _showEntry(Views/ContentViewContainer.js:472:28)
    4: showBackForwardEntryForIndex(Views/ContentViewContainer.js:212:28)
    5: showContentView(Views/ContentViewContainer.js:188:42)
    6: _showContentViewForIdentifier(Views/ResourceClusterContentView.js:236:57)
    7: restoreFromCookie(Views/ResourceClusterContentView.js:147:61)
    8: _restoreFromCookie(Models/BackForwardEntry.js:78:43)
    9: prepareToShow(Models/BackForwardEntry.js:57:32)
    10: _showEntry(Views/ContentViewContainer.js:472:28)
    11: showBackForwardEntryForIndex(Views/ContentViewContainer.js:212:28)
    12: showContentView(Views/ContentViewContainer.js:188:42)
    13: showContentViewForRepresentedObject(Views/ContentBrowser.js:168:58)
    14: showDefaultContentViewForTreeElement(Views/NavigationSidebarPanel.js:196:64)
    15: _checkElementsForPendingViewStateCookie(Views/NavigationSidebarPanel.js:701:54)
    16: _checkOutlinesForPendingViewStateCookie(Views/NavigationSidebarPanel.js:656:60)
    17: finalAttemptToRestoreViewStateFromCookie(Views/NavigationSidebarPanel.js:242:57)
    18: finalAttemptToRestoreViewStateFromCookie([native code])

    Views/SourceCodeTextEditor.js:100:20: CONSOLE LOG SourceCodeTextEditor.shown http://bogojoker.com/shell/js/easySlider.min.js
    Views/SourceCodeTextEditor.js:101:22: CONSOLE TRACE
    0: shown(Views/SourceCodeTextEditor.js:101:22)
    1: shown(Views/TextResourceContentView.js:104:31)
    2: prepareToShow(Models/BackForwardEntry.js:61:35)
    3: _showEntry(Views/ContentViewContainer.js:472:28)
    4: shown(Views/ContentViewContainer.js:425:24)
    5: shown(Views/ClusterContentView.js:75:41)
    6: shown(Views/ResourceClusterContentView.js:125:61)
    7: prepareToShow(Models/BackForwardEntry.js:61:35)
    8: _showEntry(Views/ContentViewContainer.js:472:28)
    9: showBackForwardEntryForIndex(Views/ContentViewContainer.js:212:28)
    10: showContentView(Views/ContentViewContainer.js:188:42)
    11: showContentViewForRepresentedObject(Views/ContentBrowser.js:168:58)
    12: showDefaultContentViewForTreeElement(Views/NavigationSidebarPanel.js:196:64)
    13: _checkElementsForPendingViewStateCookie(Views/NavigationSidebarPanel.js:701:54)
    14: _checkOutlinesForPendingViewStateCookie(Views/NavigationSidebarPanel.js:656:60)
    15: finalAttemptToRestoreViewStateFromCookie(Views/NavigationSidebarPanel.js:242:57)
    16: finalAttemptToRestoreViewStateFromCookie([native code])
Comment 3 Joseph Pecoraro 2015-05-15 17:25:57 PDT
I'll file a separate bug about the duplicate shown.

After discussing this with Timothy Hatcher on IRC, he came up with the idea of delaying Tab state restoration until the tab is shown. That is easy to do and seems like a great idea. Instead of potentially initializing the content views of all the tabs on startup, we only initialize the visible tab.

Patch soon.
Comment 4 Joseph Pecoraro 2015-05-15 17:35:06 PDT
Created attachment 253246 [details]
[PATCH] Proposed Fix
Comment 5 Joseph Pecoraro 2015-05-15 17:36:04 PDT
Created attachment 253247 [details]
[PATCH] Proposed Fix
Comment 6 Build Bot 2015-05-15 18:38:28 PDT
Comment on attachment 253247 [details]
[PATCH] Proposed Fix

Attachment 253247 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6131852188319744

New failing tests:
compositing/visible-rect/iframe-no-layers.html
Comment 7 Build Bot 2015-05-15 18:38:31 PDT
Created attachment 253254 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 8 Timothy Hatcher 2015-05-15 19:18:56 PDT
Comment on attachment 253247 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:113
> +            this._shouldRestoreStateWhenShown = false;

Might be best to do this in restoreStateFromCookie() so it can't be forgotten.
Comment 9 Joseph Pecoraro 2015-05-18 12:04:59 PDT
Created attachment 253335 [details]
[PATCH] For Landing
Comment 10 WebKit Commit Bot 2015-05-18 12:53:36 PDT
Comment on attachment 253335 [details]
[PATCH] For Landing

Clearing flags on attachment: 253335

Committed r184508: <http://trac.webkit.org/changeset/184508>