[webkit-reviews] review denied: [Bug 85586] Web Inspector: "Goto Function" filtering should be less restrictive. : [Attachment 140171] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied eustas.bug at gmail.com's request
for review:
Bug 85586: Web Inspector: "Goto Function" filtering should be less restrictive.
https://bugs.webkit.org/show_bug.cgi?id=85586

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

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


> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:129
> +	       if (this._checkItemAt(i)) {

To make things simple, I would structure the code as follows:

var regex = this._createSearchRegex(this._promptElement.value);
for (var i = ...) {
    if (regex.test(this._delegate.itemKeyAt(i))
	...
}

Then method below would not be needed.

> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:195
> +	   if (!query) {

The WebKit style for thise would be:

if (!query) {
    delete this._checkItemAt;
    return;
}

rest of the code here.

> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:204
> +	       this._checkItemAt = checker.bind(this);

You should  never overwrite methods on the prototype.

> Source/WebCore/inspector/front-end/FilteredItemSelectionDialog.js:231
> +		   c = c + "\\";

Escaping via appending \\ looks strange. Having test cases suggesting the
behavior pattern would help understanding the logic.