Bug 172304 – Web Inspector: Use initialLayout for Settings tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

The creation of the SettingView objects does not need to happen when the Settings tab is created.

[Devin Rousso]

Created attachment 310537 [details]
Patch

[Devin Rousso]

Created attachment 310538 [details]
[Image] Timeline Before

[Devin Rousso]

Created attachment 310539 [details]
[Image] Timeline After

[Matt Baker]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

(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.

[Matt Baker]

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.

[Matt Baker]

Comment on attachment 310537 [details]
Patch

Clearing r+ in light of Joe's comments.

[Devin Rousso]

Created attachment 310554 [details]
Patch

[Matt Baker]

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%

[Devin Rousso]

Created attachment 310666 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 310666 [details]
Patch

Clearing flags on attachment: 310666

Committed r217132: <http://trac.webkit.org/changeset/217132>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.