[webkit-reviews] review denied: [Bug 79912] Web Inspector: Implement suggestions in Watch Expressions : [Attachment 129686] Patch

[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 79912: Web Inspector: Implement suggestions in Watch Expressions
https://bugs.webkit.org/show_bug.cgi?id=79912

Attachment 129686: Patch
https://bugs.webkit.org/attachment.cgi?id=129686&action=review

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


> Source/WebCore/inspector/front-end/ConsoleView.js:345
> +    completionsForElement: function(promptElement, wordRange, force,
completionsReadyCallback)

This looks like a bad design. TextPrompt should pass itself or its element into
the callback.

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:324
> +	   this.update();

Why did this change?

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:361
> +	   this._prompt = new
WebInspector.WatchExpressionPrompt(this.editingCommitted.bind(this, null,
this.nameElement.textContent, context.previousContent, context),
this.editingCancelled.bind(this, null, context));

This seems wrong: you bind this and this's properties such as nameElement,
context.previousContent, etc.

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:396
> +	  
WebInspector.ObjectPropertyTreeElement.prototype.editingEnded.call(this,
context);

Why do we need this? It would be great if ObjectPropertyTreeElement supported
the same functionality.

> Source/WebCore/inspector/front-end/WatchExpressionsSidebarPane.js:419
> +    const ExpressionStopCharacters = " =:[({;,!+-*/&|^<>."; // Same as in
ConsoleView.js + "."

Move to a common place?