Bug 85711 - Web Inspector: Turn HelpScreen to be View.
: Web Inspector: Turn HelpScreen to be View.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: eustas.bug
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-05 07:51 PDT by eustas.bug
Modified: 2012-10-12 04:06 PDT (History)
12 users (show)

See Also:


Attachments
Patch (5.98 KB, patch)
2012-05-05 07:59 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (10.85 KB, patch)
2012-05-10 05:23 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (22.58 KB, patch)
2012-05-11 02:02 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2012-05-11 06:16 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (18.43 KB, patch)
2012-05-11 07:38 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (18.45 KB, patch)
2012-05-12 03:47 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (19.07 KB, patch)
2012-05-12 05:45 PDT, eustas.bug
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eustas.bug 2012-05-05 07:51:21 PDT
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).
Comment 1 eustas.bug 2012-05-05 07:59:40 PDT
Created attachment 140400 [details]
Patch
Comment 2 Pavel Feldman 2012-05-05 08:31:37 PDT
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 3 eustas.bug 2012-05-10 05:20:00 PDT
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.
Comment 4 eustas.bug 2012-05-10 05:23:36 PDT
Created attachment 141151 [details]
Patch
Comment 5 Pavel Feldman 2012-05-10 06:34:12 PDT
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.
Comment 6 eustas.bug 2012-05-11 02:02:41 PDT
Created attachment 141357 [details]
Patch
Comment 7 Pavel Feldman 2012-05-11 05:02:24 PDT
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 8 eustas.bug 2012-05-11 06:05:22 PDT
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.
Comment 9 eustas.bug 2012-05-11 06:16:06 PDT
Created attachment 141395 [details]
Patch
Comment 10 Andrey Kosyakov 2012-05-11 06:48:14 PDT
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 11 eustas.bug 2012-05-11 07:32:55 PDT
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.
Comment 12 eustas.bug 2012-05-11 07:38:58 PDT
Created attachment 141417 [details]
Patch
Comment 13 Andrey Kosyakov 2012-05-11 07:49:26 PDT
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.
Comment 14 Yury Semikhatsky 2012-05-11 08:03:38 PDT
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.
Comment 15 eustas.bug 2012-05-12 03:47:43 PDT
Created attachment 141567 [details]
Patch
Comment 16 eustas.bug 2012-05-12 03:48:54 PDT
Rebased, removed redundant type annotation, fixed ChangeLog.
Comment 17 eustas.bug 2012-05-12 05:45:12 PDT
Created attachment 141572 [details]
Patch
Comment 18 eustas.bug 2012-05-12 05:46:08 PDT
Darkened background color, fixed scrollbar spacing.
Comment 19 Andrey Kosyakov 2012-05-12 06:38:34 PDT
Committed r116854: <http://trac.webkit.org/changeset/116854>