[webkit-reviews] review denied: [Bug 38807] Web Inspector: Searching in multiple scripts in the scripts tab : [Attachment 110385] Patch (rebaselined)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 11 03:06:45 PDT 2011


Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 38807: Web Inspector: Searching in multiple scripts in the scripts tab
https://bugs.webkit.org/show_bug.cgi?id=38807

Attachment 110385: Patch (rebaselined)
https://bugs.webkit.org/attachment.cgi?id=110385&action=review

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


> Source/WebCore/ChangeLog:10
> +	   with Ctrl+Shift+F (Cmd+Shift+F) shortcut which is currently
disabled.

Could you attach a screenshot with the search results pane even though it is
currently disabled?

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:42
> +    return WebInspector.KeyboardShortcut.makeDescriptor("f",
WebInspector.KeyboardShortcut.Modifiers.Ctrl |
WebInspector.KeyboardShortcut.Modifiers.Shift);

Drive-by: we usually do CtrlOrMeta variable. Should we make it a part of the
keyboardShortcut?

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:53
> +	* @param {!Event} event

Here and below: drop ! we are non-nullable by default.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:74
> +	       this._searchView.wasShown();

You should never call wasShown explicitly.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:90
> +	   this._searchView.resultsView = this._searchResultsView; 

You don't really need different views, do you? I'd say you only need one that
is placed in the drawer.

> Source/WebCore/inspector/front-end/Drawer.js:57
> +WebInspector.Drawer.AnimationType = {

This can land as a separate patch.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1201
> +    canPerformAdvancedSearch: function()

I don't think that advanced search should be panel specific. You probably need
to put this code into a separate class that depends on the debugger
presentation model.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1356
> +WebInspector.ScriptsPanelSearchResultsView = function(panel)

Also could be defined outside the scripts panel.

> Source/WebCore/inspector/front-end/Settings.js:92
> +    this.advancedSearchOptions = this.createSetting("advancedSearchOptions",
{queries: [], ignoreCase: true, isRegex: false});

Could you define explicit type for the second parameter (SearchConfig)?

> Source/WebCore/inspector/front-end/inspector.css:4139
> +.search-view .search-panel label {

These are very expensive selectors. They start matching by label, input, input
of particular type, etc. Given that we don't use search that much, I'd suggest
implementing search results view in the iframe with its own css.


More information about the webkit-reviews mailing list