Summary: | Web Inspector: Use initialLayout for Settings tab | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, inspector-bugzilla-changes, joepeck, mattbaker | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 172303 | ||||||||||||||
Attachments: |
|
Description
Devin Rousso
2017-05-18 12:47:36 PDT
Created attachment 310537 [details]
Patch
Created attachment 310538 [details]
[Image] Timeline Before
Created attachment 310539 [details]
[Image] Timeline After
Nice work! Eventually we should audit our view classes for more opportunities to use initialLayout (as we discussed IRL the other day). In the past this has only been problematic when a view does background work before being shown, and assumes the existence of it's DOM. Ideally, hidden views would never modify their DOM, but our ad-hoc visibility model makes consistency difficult here. Comment on attachment 310537 [details]
Patch
Lets do more then one of these in a single patch. You can probably find 5-10 cases of this instead of just doing SettingsTab, and hopefully from there we will see a meaningful total impact.
(In reply to Joseph Pecoraro from comment #5) > Lets do more then one of these in a single patch. You can probably find 5-10 > cases of this instead of just doing SettingsTab, and hopefully from there we > will see a meaningful total impact. You should also verify that initialLayout doesn't happen until you open the Settings tab. If it is still happening at some point in the loading process then all we've done would be moving the time somewhere else but still doing it at startup. I trust you've done that but just making certain. Comment on attachment 310537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310537&action=review r=me. Please add at least a short summary to the change log mentioning the motivation for the change. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:171 > + WebInspector.settings.zoomFactor.addEventListener(WebInspector.Setting.Event.Changed, boundNeedsLayout); I realize this was just moved from the constructor, but it seems like a layering violation having the tab know about specific settings. It should be up to GeneralSettingsView to update itself. A general solution would be even better, where SettingEditor updates itself whenever the value of its backing setting changes. Comment on attachment 310537 [details]
Patch
Clearing r+ in light of Joe's comments.
Created attachment 310554 [details]
Patch
Comment on attachment 310554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310554&action=review r=me, with a comment > Source/WebInspectorUI/ChangeLog:16 > + Move the code for doing this to initialLayout ensures it to happens once, but only after the Awkward wording, maybe: "Use initialLayout to ensure UI gets created exactly once, after the tab is selected." > Source/WebInspectorUI/UserInterface/Views/GeneralSettingsView.js:84 > + WebInspector.settings.zoomFactor.addEventListener(WebInspector.Setting.Event.Changed, () => { zoomEditor.value = WebInspector.getZoomFactor().maxDecimals(2); }); Nice, it looks like the control wasn't updating at all before: 1. Open Settings (pre-patch) 2. Scroll / resize so Zoom level is visible 3. Hit Cmd-+/- => Still says 100% Created attachment 310666 [details]
Patch
Comment on attachment 310666 [details] Patch Clearing flags on attachment: 310666 Committed r217132: <http://trac.webkit.org/changeset/217132> All reviewed patches have been landed. Closing bug. |