Bug 85711 – Web Inspector: Turn HelpScreen to be View.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

For further UI changes (Bug 85061), HelpSceen need to be View.
Also this change will make "helpScreen.css" to be lazy-loaded (as a benefit of being View).

Created attachment 140400 [details]
Patch

[Pavel Feldman]

Comment on attachment 140400 [details]
Patch

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

Overall looks good. Note that since you are going to make it tabbed pane, you will not need the custom show / hide logic - your help screen will become singleton. So most of the code here will vanish.

> Source/WebCore/inspector/front-end/HelpScreen.js:68
>      show: function(onHide)

Do we actually use onHide?

> Source/WebCore/inspector/front-end/HelpScreen.js:-62
> -        if (this._isShown)

You are overriding View's show with different parameters. I would pick another name.

> Source/WebCore/inspector/front-end/HelpScreen.js:70
> +        var visibleHelpScreen = WebInspector.HelpScreen.visibleScreen_;

_ is before the name, not after

> Source/WebCore/inspector/front-end/HelpScreen.js:79
> +        WebInspector.View.prototype.show.call(this, document.body);

document.body -> WebInspector.inspectorView.element

Comment on attachment 140400 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HelpScreen.js:68
>>      show: function(onHide)
> 
> Do we actually use onHide?

For sure. It is used, at least, to remove highlight from "Settings" icon.

>> Source/WebCore/inspector/front-end/HelpScreen.js:-62
>> -        if (this._isShown)
> 
> You are overriding View's show with different parameters. I would pick another name.

Done.

>> Source/WebCore/inspector/front-end/HelpScreen.js:70
>> +        var visibleHelpScreen = WebInspector.HelpScreen.visibleScreen_;
> 
> _ is before the name, not after

Wasn't me =)
Fixed.

>> Source/WebCore/inspector/front-end/HelpScreen.js:79
>> +        WebInspector.View.prototype.show.call(this, document.body);
> 
> document.body -> WebInspector.inspectorView.element

Done.

Created attachment 141151 [details]
Patch

[Pavel Feldman]

Comment on attachment 141151 [details]
Patch

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

> Source/WebCore/inspector/front-end/HelpScreen.js:39
> +    this.markAsRoot();

You are showing it as a child of inspector view, no need to mark as root.

> Source/WebCore/inspector/front-end/HelpScreen.js:70
> +    present: function(onHide)

Naming of the function is not clear.

Created attachment 141357 [details]
Patch

[Pavel Feldman]

Comment on attachment 141357 [details]
Patch

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

> Source/WebCore/inspector/front-end/SettingsController.js:34
> +WebInspector.SettingsController = function()

This can be a part of the settings screen for simplicity.

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:54
> +    willShow: function()

Why new callback? Doesn't wasShown work?

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:57
> +        WebInspector.HelpScreen.prototype.willShow.call(this);

Is there anything in this prototype? You should not override View's methods here, HelpScreen should provide appropriate apis for extensibility.

Comment on attachment 141357 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SettingsController.js:34
>> +WebInspector.SettingsController = function()
> 
> This can be a part of the settings screen for simplicity.

Moved.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:54
>> +    willShow: function()
> 
> Why new callback? Doesn't wasShown work?

IMHO it is much better to manipulate DOM until it became visible, not after.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:57
>> +        WebInspector.HelpScreen.prototype.willShow.call(this);
> 
> Is there anything in this prototype? You should not override View's methods here, HelpScreen should provide appropriate apis for extensibility.

willShow/wasShown/willHide are clearly hook methods created specially for purposes like that.
It seems to be inappropriate to create a new layer of hooks.

Created attachment 141395 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 141395 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Motivation: for further UI changes, HelpSceen need to be View.

s/need/needs. Also, you may want to mention the intended future changes (e.g. make it a tabbed view).

> Source/WebCore/inspector/front-end/SettingsScreen.js:607
> +    var button = new WebInspector.StatusBarButton(WebInspector.UIString("Settings"), "settings-status-bar-item");
> +    button.addEventListener("click", this._buttonClicked.bind(this), false);
> +
> +    /** @type {!WebInspector.StatusBarButton} */
> +    this._statusBarButton = button;

Can we make this simpler by not using the intermediate local variable?

> Source/WebCore/inspector/front-end/SettingsScreen.js:610
> +    /** @type {?WebInspector.SettingsScreen} */
> +    this._settingsScreen;

What's that for?

> Source/WebCore/inspector/front-end/SettingsScreen.js:634
> +    showSettingsScreen: function()

does this have to be public?

> Source/WebCore/inspector/front-end/SettingsScreen.js:643
> +    hideSettingsScreen: function()

ditto

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:56
>          this._buildTable(this.contentElement);

Does it really make a difference whether we render this before, not after this is shown? I.e., are there strong reasons we can't live with wasShown()?

Comment on attachment 141395 [details]
Patch

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

>> Source/WebCore/ChangeLog:6
>> +        Motivation: for further UI changes, HelpSceen need to be View.
> 
> s/need/needs. Also, you may want to mention the intended future changes (e.g. make it a tabbed view).

Fixed.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:607
>> +    this._statusBarButton = button;
> 
> Can we make this simpler by not using the intermediate local variable?

We can. But doesn't local variable here make code more readable?
Fixed.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:610
>> +    this._settingsScreen;
> 
> What's that for?

That is member type declaration.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:634
>> +    showSettingsScreen: function()
> 
> does this have to be public?

This method is planned to be used outside of controller (with optional parameter "tabToShowId").
Fixed for now.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:643
>> +    hideSettingsScreen: function()
> 
> ditto

Fixed.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:56
>>          this._buildTable(this.contentElement);
> 
> Does it really make a difference whether we render this before, not after this is shown? I.e., are there strong reasons we can't live with wasShown()?

There are no strong reasons. Reverted.

Created attachment 141417 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 141417 [details]
Patch

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

LGTM with one nit. Pavel, could you please have a look?

> Source/WebCore/inspector/front-end/SettingsScreen.js:603
> +    /** @type {!WebInspector.StatusBarButton} */

I think annotation is redundant there, type should be inferred by the JS compiler from the initialization.

[Yury Semikhatsky]

Comment on attachment 141417 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This line should go before the description.

Created attachment 141567 [details]
Patch

Rebased, removed redundant type annotation, fixed ChangeLog.

Created attachment 141572 [details]
Patch

Darkened background color, fixed scrollbar spacing.

[Andrey Kosyakov]

Committed r116854: <http://trac.webkit.org/changeset/116854>