Bug 52212 – Web Inspector: Employ TextPrompt for CSS property name/value autocompletion

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

[Alexander Pavlov (apavlov)]

TextPrompt features text autocompletion, which is a great feature we could use for autocompleting CSS property names/values instead of writing ad-hoc autocompletions.

[Alexander Pavlov (apavlov)]

Created attachment 78819 [details]
[PATCH] Suggested solution

[Yury Semikhatsky]

Comment on attachment 78819 [details]
[PATCH] Suggested solution

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

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1630
> +                    return { keyIdentifier: "___", handled: false };

___ looks weird, can you use null or undefined instead?

> Source/WebCore/inspector/front-end/TextPrompt.js:76
> +        if (this._customKeyDownHandler) {

Let's extract Up, Down and Tab handlers into their own methods to allow overriding them. r- for this.

> Source/WebCore/inspector/front-end/TextPrompt.js:400
> +    _tabKeyPressed: function(reverse)

This method can be inlined.

[Alexander Pavlov (apavlov)]

Created attachment 78939 [details]
[PATCH] Comments addressed

Introduced a customKeyHandlers parameter into the TextPrompt ctor to pass in custom Up, Down, and Tab handlers.

[Alexander Pavlov (apavlov)]

Created attachment 78946 [details]
[PATCH] Inherited TextPrompt to override key handlers

[Pavel Feldman]

Comment on attachment 78946 [details]
[PATCH] Inherited TextPrompt to override key handlers

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

r- is for managing selection in the sidebar, rest seems fine.

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:45
> +WebInspector.CSSKeywordCompletions._colors = [

Nit: please compress to ~120 chars per line.

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:197
> +WebInspector.CSSKeywordCompletions._colorAwareProperties = [

ditto

> Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:224
> +WebInspector.CSSKeywordCompletions._propertyKeywordMap = {

ditto

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690
> +            finalSelectionRange.setStart(replacementTextNode, 0);

I would expect all the selection management code to move to the prompt. Why is it still here?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1902
> +

Nit: blank line.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (From update of attachment 78946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78946&action=review
> 
> r- is for managing selection in the sidebar, rest seems fine.
> 
> > Source/WebCore/inspector/front-end/CSSKeywordCompletions.js:45
> > +WebInspector.CSSKeywordCompletions._colors = [
> 
> Nit: please compress to ~120 chars per line.

Done.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690
> > +            finalSelectionRange.setStart(replacementTextNode, 0);
> 
> I would expect all the selection management code to move to the prompt. Why is it still here?

I don't understand why you would expect this. I'd rather understand you expect all the _prompt_ management code move there. But scrolling numeric values is a whole different thing, isn't it? It does not match the TextPrompt responsibilities altogether.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1902
> > +
> 
> Nit: blank line.

Done.

[Alexander Pavlov (apavlov)]

Created attachment 79146 [details]
[PATCH] Comments from pfeldman addressed

[Alexander Pavlov (apavlov)]

Landed with minor fixes: http://trac.webkit.org/changeset/76116