Bug 146256

Summary: Web Inspector: Nested ContentBrowsers / ContentViewContainers cause too many ContentView updates
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 151058    
Bug Blocks:    

Joseph Pecoraro
Reported 2015-06-23 15:45:17 PDT
* SUMMARY Nested ContentBrowsers / ContentViewContainers cause too many ContentView updates. For resources the View hierarchy looks something like this: - TabBrowser - ContentViewContainer - ResourcesTabContentView (1) (ContentBrowserTabContentView with a ContentBrowser) - ContentViewContainer - ResourceClusterContentView (2) (ClusterContentView) - ContentViewContainer - TextResourceContentView (3) - TextEditor In the process of showing each of the 3 ContentViews, actions are taken on its sub-views that repeat over and over depending on the nested level. ContentViewContainer.showContentView triggers BackForwardEntry.prepareToShow, and BackForwardEntry.prepareToShow triggers multiple actions which may cascade: - _restoreFromCookie() may cascade actions to sub ContentViews - shown() may cascade actions to sub ContentViews - updateLayout() may cascade actions to sub ContentViews This situation is not ideal! It would be much better to reduce duplication of work here. * STEPS TO REPRODUCE 1. Load <http://apple.com> 2. Show page source (Web Inspector, Resources Tab, Main Resource selected) 3. Scroll down the text editor 4. Switch to the Elements tab 5. Switch back to the Resources tab => updateLayout was called 6+ times on the TextEditor * NOTES - Here are annotations of the different updateLayout calls I saw in this case: 1. prepareToShow ResourceClusterContentView triggers restoreFromCookie/showContentView for the TextResourceContentView (call shown false) and updateLayout 2. prepareToShow->shown ResourceClusterContentView triggers shown for the TextResourceContentView (call shown true) and updateLayout 3. prepareToShow->shown ResourceClusterContentView triggers another showContentView for TextResourceContentView (call shown false) and updateLayout 4. prepareToShow->updateLayout ResourceClusterContentView triggers another updateLayout for TextResourceContentView 5. prepareToShow->updateLayout Tab ContentView triggers another updateLayout down the cascade to TextResourceContentView 6. Left Sidebar opening (I came from Elements -> Resources) triggers another updateLayout 7. Quick Console claims it resized (probably didn't) triggers another updateLayout
Attachments
Radar WebKit Bug Importer
Comment 1 2015-06-23 15:45:37 PDT
Joseph Pecoraro
Comment 2 2015-06-23 15:47:19 PDT
> 1. prepareToShow ResourceClusterContentView triggers > restoreFromCookie/showContentView for the TextResourceContentView (call > shown false) and updateLayout This one is particularly nasty, because this is the ResourceClusterContentView triggering an updateLayout on its sub-ContentView (TextResourceContentView) without having shown() on that view first. So the elements may not actually be in the DOM, and therefore may get incorrect / unexpected values for its views.
Joseph Pecoraro
Comment 3 2015-06-23 16:05:01 PDT
> 7. Quick Console claims it resized (probably didn't) triggers another updateLayout Eliminating this one as part of: <https://webkit.org/b/146258> Web Inspector: Reduce QuickConsole DidResize events if it did not change
Blaze Burg
Comment 4 2015-11-18 15:13:05 PST
Matt, what's the status of this bug now that we have batched async layout?
Matt Baker
Comment 5 2015-11-18 16:10:37 PST
(In reply to comment #4) > Matt, what's the status of this bug now that we have batched async layout? Batching layouts reduced the updates to around 3 (from around 7). The fix for https://bugs.webkit.org/show_bug.cgi?id=151058 brings the total down to two (the first of which is skipped because the TextEditor hasn't become visible yet). Will close out when the above issue lands.
Note You need to log in before you can comment on or make changes to this bug.