Bug 111301

Summary: Web Inspector: Refactorings: Prepare SuggestBox for reuse.
Product: WebKit Reporter: Eugene Klyuchnikov <eustas>
Component: Web Inspector (Deprecated)Assignee: Eugene Klyuchnikov <eustas>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 111427    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Eugene Klyuchnikov 2013-03-04 04:24:21 PST
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.
Comment 1 Eugene Klyuchnikov 2013-03-04 04:35:59 PST
Created attachment 191203 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2013-03-04 05:28:17 PST
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?
Comment 3 Eugene Klyuchnikov 2013-03-05 06:26:35 PST
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.
Comment 4 Eugene Klyuchnikov 2013-03-05 06:57:09 PST
Created attachment 191490 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-07 01:51:52 PST
Comment on attachment 191490 [details]
Patch

Clearing flags on attachment: 191490

Committed r145054: <http://trac.webkit.org/changeset/145054>
Comment 6 WebKit Review Bot 2013-03-07 01:51:55 PST
All reviewed patches have been landed.  Closing bug.