[webkit-reviews] review denied: [Bug 65511] Web Inspector: autocomplete combobox for CSS style property names/values. : [Attachment 112319] [PATCH] A major rewrite, which extends the TextPrompt functionality by aggregating SuggestBox

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 65511: Web Inspector: autocomplete combobox for CSS style property
names/values.
https://bugs.webkit.org/show_bug.cgi?id=65511

Attachment 112319: [PATCH] A major rewrite, which extends the TextPrompt
functionality by aggregating SuggestBox
https://bugs.webkit.org/attachment.cgi?id=112319&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112319&action=review


> Source/WebCore/inspector/front-end/ConsoleView.js:353
> +	   if (force && !expressionString)

You should nuke the prefix check below instead.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:110
> +WebInspector.StylesSidebarPane.StyleValueDelimiters = " \xA0\t\n\"':;,/(";

You should not suggest upon ")" type.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1878
> +		   if (isEnterKey(event)) {

It would be nice if TextPrompt could be configured to handle it internally.
Worst case, you should do:

if (this._prompt.handleEvent(event))
  return;

in all entry points.

> Source/WebCore/inspector/front-end/TextPrompt.js:605
> +    this.completions = completions;

please make things private.

> Source/WebCore/inspector/front-end/inspector.css:2507
> +.suggest-box {

please move this to the newly added textPrompt.css and require it from
ConsoleView and ElementsPanel. Alternatively you could make TextPrompt a view,
but I am not sure it is worth it.