[webkit-reviews] review denied: [Bug 91394] Web Inspector: implement search / replace in source files (behind experiment flag) : [Attachment 152541] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 91394: Web Inspector: implement search / replace in source files (behind
experiment flag)
https://bugs.webkit.org/show_bug.cgi?id=91394

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152541&action=review


> Source/WebCore/inspector/front-end/ConsolePanel.js:106
> +    jumpToNextSearchResult: function(loop)

Please annotate return value.

> Source/WebCore/inspector/front-end/ConsolePanel.js:119
> +    jumpToPreviousSearchResult: function(loop)

Ditto.

> Source/WebCore/inspector/front-end/ElementsPanel.js:499
> +    jumpToNextSearchResult: function(loop)

Ditto.

> Source/WebCore/inspector/front-end/ElementsPanel.js:518
> +    jumpToPreviousSearchResult: function(loop)

Ditto.

> Source/WebCore/inspector/front-end/SearchController.js:200
> +	   this._replaceCheckboxElement.disabled = !panel.canSearchAndReplace()
&& !enabled;

This behavior seems odd to me.
Replace check box is currently disabled in two cases: there is no search
matches and/or panel does not support replace.
In one case user can make it enabled by typing a good search query, in the
other it will always stay disabled.
I think we can make it always enabled on scripts panel and keep disabled on
other panels (for discovery) OR make it hidden on other panels.
I would actually like to see both of these changes.

The current behavior is also incorrect:
1. type good search query
2. enable replace
3. erase search query

Replace checkbox becomes disabled but replace input field and buttons are still
shown.

> Source/WebCore/inspector/front-end/SearchController.js:331
> +	   this._performSearch(query, true, false, false);

It seems to me that all this loop=false machinery does not work here when
replacement does not contain search query.
When you press replace while standing on the last search match you run perform
search again and it starts from the file start again.

I would actually prefer if this feature allowed replacement loop but showed a
message when I pass the same line again (not the file start)