Bug 85711 - Web Inspector: Turn HelpScreen to be View.
: Web Inspector: Turn HelpScreen to be View.
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-05 07:51 PST by
Modified: 2012-10-12 04:06 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-05 07:51:21 PST
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 From 2012-05-05 07:59:40 PST -------
Created an attachment (id=140400) [details]
Patch
------- Comment #2 From 2012-05-05 08:31:37 PST -------
(From update of attachment 140400 [details])
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 From 2012-05-10 05:20:00 PST -------
(From update of attachment 140400 [details])
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 From 2012-05-10 05:23:36 PST -------
Created an attachment (id=141151) [details]
Patch
------- Comment #5 From 2012-05-10 06:34:12 PST -------
(From update of attachment 141151 [details])
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 From 2012-05-11 02:02:41 PST -------
Created an attachment (id=141357) [details]
Patch
------- Comment #7 From 2012-05-11 05:02:24 PST -------
(From update of attachment 141357 [details])
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 From 2012-05-11 06:05:22 PST -------
(From update of attachment 141357 [details])
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 From 2012-05-11 06:16:06 PST -------
Created an attachment (id=141395) [details]
Patch
------- Comment #10 From 2012-05-11 06:48:14 PST -------
(From update of attachment 141395 [details])
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 From 2012-05-11 07:32:55 PST -------
(From update of attachment 141395 [details])
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 From 2012-05-11 07:38:58 PST -------
Created an attachment (id=141417) [details]
Patch
------- Comment #13 From 2012-05-11 07:49:26 PST -------
(From update of attachment 141417 [details])
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 From 2012-05-11 08:03:38 PST -------
(From update of attachment 141417 [details])
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 From 2012-05-12 03:47:43 PST -------
Created an attachment (id=141567) [details]
Patch
------- Comment #16 From 2012-05-12 03:48:54 PST -------
Rebased, removed redundant type annotation, fixed ChangeLog.
------- Comment #17 From 2012-05-12 05:45:12 PST -------
Created an attachment (id=141572) [details]
Patch
------- Comment #18 From 2012-05-12 05:46:08 PST -------
Darkened background color, fixed scrollbar spacing.
------- Comment #19 From 2012-05-12 06:38:34 PST -------
Committed r116854: <http://trac.webkit.org/changeset/116854>