Bug 111301 – Web Inspector: Refactorings: Prepare SuggestBox for reuse.

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

[Eugene Klyuchnikov]

1) Remove userEnteredText from delegate interface. Pass prefix to updateSuggestions instead.
2) Clarify that suggestions array is never null.
3) Add option to align suggestion box to element specified during construction.

4) Avoid possible NPE in StylesSidebarPane.
5) Add some JSDocs to CSSMetadata and StylesSidebarPane.CSSPropertyPrompt.
6) Remove unused parameter of StylesSidebarPane.CSSPropertyPrompt constructor.

[Eugene Klyuchnikov]

Created attachment 191203 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 191203 [details]
Patch

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

It is quite hard to track the changes in this patch even for the author of this code. Let us separate the patch into a few others, and not land provisional code (you can make the respective changes in the patch that uses the new code.)

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2649
> + * @param {?WebInspector.CSSMetadata} cssCompletions

Why is it nullable???

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:-2714
> -        if (!word)

Unnecessary change.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2730
> +        var cssCompletions = this._cssCompletions;

Ditto.

> Source/WebCore/inspector/front-end/SuggestBox.js:60
> +    this._anchorElement = anchorElement;

This field is unused, hence may be removed altogether.

> Source/WebCore/inspector/front-end/SuggestBox.js:98
> +     * @param {AnchorBox=} anchorBox

Any cases where the anchorBox is not supplied to _updateBoxPosition()?

> Source/WebCore/inspector/front-end/SuggestBox.js:330
> +        if (this._canShowBox(completions, !!canShowForSingleItem, userEnteredText)) {

Why "!!canShowForSingleItem" if it is mandatory?

[Eugene Klyuchnikov]

Comment on attachment 191203 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SuggestBox.js:60
>> +    this._anchorElement = anchorElement;
> 
> This field is unused, hence may be removed altogether.

_anchorElement is now used in _updateBoxPosition

>> Source/WebCore/inspector/front-end/SuggestBox.js:98
>> +     * @param {AnchorBox=} anchorBox
> 
> Any cases where the anchorBox is not supplied to _updateBoxPosition()?

My next patch uses SuggestionBox this way.
But I think you are right - this change could be separate from refactoring.

>> Source/WebCore/inspector/front-end/SuggestBox.js:330
>> +        if (this._canShowBox(completions, !!canShowForSingleItem, userEnteredText)) {
> 
> Why "!!canShowForSingleItem" if it is mandatory?

oops. That was mandatory for previous signature. I'll revert this change.

[Eugene Klyuchnikov]

Created attachment 191490 [details]
Patch

[WebKit Review Bot]

Comment on attachment 191490 [details]
Patch

Clearing flags on attachment: 191490

Committed r145054: <http://trac.webkit.org/changeset/145054>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.