Bug 146256
Summary: | Web Inspector: Nested ContentBrowsers / ContentViewContainers cause too many ContentView updates | ||
---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> |
Component: | Web Inspector | Assignee: | 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
* 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 | ||
---|---|---|
Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/21513227>
Joseph Pecoraro
> 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
> 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
Matt, what's the status of this bug now that we have batched async layout?
Matt Baker
(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.