Bug 111301 - Web Inspector: Refactorings: Prepare SuggestBox for reuse.
Summary: Web Inspector: Refactorings: Prepare SuggestBox for reuse.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eugene Klyuchnikov
URL:
Keywords:
Depends on: 111427
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-04 04:24 PST by Eugene Klyuchnikov
Modified: 2013-03-07 01:51 PST (History)
9 users (show)

See Also:


Attachments
Patch (16.57 KB, patch)
2013-03-04 04:35 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (12.59 KB, patch)
2013-03-05 06:57 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.