Bug 101907 – Web Inspector: Extract common interface for StatusBarButton and StatusBarCombo

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

Status bar control element should have common interface for easier management.

Created attachment 173579 [details]
Patch

[Pavel Feldman]

Comment on attachment 173579 [details]
Patch

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

> Source/WebCore/inspector/front-end/CPUProfileView.js:74
> +    this.focusButton.setDisabled(true);

If we change the API, I would introduce setEnabled.

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

Where is it used? Maybe it should have default implementation that manages element's state based on the calls below? I.e. be not an interface, but a base class?

Created attachment 173610 [details]
Patch

Comment on attachment 173579 [details]
Patch

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

>> Source/WebCore/inspector/front-end/CPUProfileView.js:74
>> +    this.focusButton.setDisabled(true);
> 
> If we change the API, I would introduce setEnabled.

Done

>> Source/WebCore/inspector/front-end/StatusBarButton.js:34
>> +WebInspector.StatusBarItem = function()
> 
> Where is it used? Maybe it should have default implementation that manages element's state based on the calls below? I.e. be not an interface, but a base class?

Done

[Pavel Feldman]

Comment on attachment 173610 [details]
Patch

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

> Source/WebCore/inspector/front-end/StatusBarButton.js:32
>   * @extends {WebInspector.Object}

@constructor should go first (we use alphabetic order)

> Source/WebCore/inspector/front-end/StatusBarButton.js:34
> + * @param {!Element} element

We don't use ! notation

> Source/WebCore/inspector/front-end/StatusBarButton.js:46
> +    setEnabled: function(value) {

{ on the next line

> Source/WebCore/inspector/front-end/StatusBarButton.js:58
> + * @constructor

ditto

> Source/WebCore/inspector/front-end/StatusBarButton.js:272
> + * @extends {WebInspector.StatusBarItem}

ditto

> Source/WebCore/inspector/front-end/StatusBarButton.js:291
> +     * @param {!Element} option

ditto

> Source/WebCore/inspector/front-end/StatusBarButton.js:301
> +    setEnabled: function(value)

Hm. This looks like copy-paste, but I don't have a good suggestion for you :(

Comment on attachment 173610 [details]
Patch

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

>> Source/WebCore/inspector/front-end/StatusBarButton.js:34
>> + * @param {!Element} element
> 
> We don't use ! notation

Why not? Without "!" closure can't check for potential NPEs.

Comment on attachment 173610 [details]
Patch

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

>> Source/WebCore/inspector/front-end/StatusBarButton.js:32
>>   * @extends {WebInspector.Object}
> 
> @constructor should go first (we use alphabetic order)

Actually, this code fragment hasn't been modified.

>> Source/WebCore/inspector/front-end/StatusBarButton.js:46
>> +    setEnabled: function(value) {
> 
> { on the next line

Fixed.

>> Source/WebCore/inspector/front-end/StatusBarButton.js:58
>> + * @constructor
> 
> ditto

Fixed.

>> Source/WebCore/inspector/front-end/StatusBarButton.js:272
>> + * @extends {WebInspector.StatusBarItem}
> 
> ditto

Fixed.

>> Source/WebCore/inspector/front-end/StatusBarButton.js:301
>> +    setEnabled: function(value)
> 
> Hm. This looks like copy-paste, but I don't have a good suggestion for you :(

Addressed.

Created attachment 174067 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174067 [details]
Patch

Clearing flags on attachment: 174067

Committed r134550: <http://trac.webkit.org/changeset/134550>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.