[webkit-reviews] review denied: [Bug 65511] Web Inspector: autocomplete combobox for CSS style property names/values. : [Attachment 104048] [PATCH] Comments addressed

[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 104048: [PATCH] Comments addressed
https://bugs.webkit.org/attachment.cgi?id=104048&action=review

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


Too much copypaste.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:930
> +	   if (!curSection) {

Could you submit this as a separate change?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:952
> +	   if (!curSection) {

same other change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1139
> +	   this._selectorElement.scrollIntoViewIfNeeded(false);

same other change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2047
> +	       else if (moveDirection === "backward") {

same other change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2248
> +	   //this._clearAutoComplete();

Should this code be commented?

> Source/WebCore/inspector/front-end/SuggestBox.js:32
> +    this.parentElement = inputElement.ownerDocument.body;

Please make these fields private

> Source/WebCore/inspector/front-end/SuggestBox.js:43
> +    window.addEventListener("scroll", this._boundOnScroll, true);

No need for scroll / resize listeners.

> Source/WebCore/inspector/front-end/SuggestBox.js:85
> +	   const anchorBox = this.inputElement.boxInWindow(window,
this.parentElement);

Could you style it using CSS instead?

> Source/WebCore/inspector/front-end/SuggestBox.js:135
> +	   if (event.handled)

event.handled is not standard. how can this happen? The one setting it should
stop propagation instead as you do below.

> Source/WebCore/inspector/front-end/SuggestBox.js:146
> +		   this.tabKeyPressed(event);

These all should be private.

> Source/WebCore/inspector/front-end/SuggestBox.js:192
> +	   this.inputElement.pruneEmptyTextNodes();

This looks a lot like copypaste from the TextPrompt.

> Source/WebCore/inspector/front-end/SuggestBox.js:222
> +    acceptAutoComplete: function(event)

This method looks exactly as the one in the TextPrompt. I think the way you
should implement it is to extend TextPrompt instead. It could be TextPrompt +
drop box.

> Source/WebCore/inspector/front-end/SuggestBox.js:378
> +    _completionsReady: function(selection, completions)

Again giant copypaste?