Bug 73303 – Web Inspector: Implement the CSS scope for Advanced search

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

We should be able to search for elements using selector matches and, perhaps, property values.

[Alexander Pavlov (apavlov)]

Created attachment 116972 [details]
Patch

[Early Warning System Bot]

Comment on attachment 116972 [details]
Patch

Attachment 116972 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10676759

[Alexander Pavlov (apavlov)]

Created attachment 116973 [details]
[PATCH] Added a file left behind in the previous patch

[Vsevolod Vlasov]

Comment on attachment 116973 [details]
[PATCH] Added a file left behind in the previous patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116973&action=review

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:78
> +            this._currentScopeIndex = 0;

I don't think you need this._currentSearchIndex - you already pass all search scopes to SearchView, the code would be cleaner if you just pass the selected one back. No need to make SearchView depend on controller search scopes storage implementation details. We will probably sort scopes alphabetically in the select field, won't we?

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:81
> +    setActiveSearchScope: function(index)

I don't think you need that either. AdvancedSearchController should not know anything about selection changes in UI unless search is started.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:143
> +    startSearch: function(searchConfig, selectedIndex)

searchScope?

Please update annotations for changed methods and add them for added methods.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:152
> +        var totalSearchResultsCount = this._currentSearchScope.prepareSearch(searchConfig);

I understand the only reason to extract this method is to deal with totalSearchResultsCount === 0 case?
Why not just check for that before calling searchStarted?
If we really want to call searchStarted right after the preparation I would suggest adding another searchStartedCallback to the original performSearch, as this preparation could generally be asynchronous.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:217
> +    this._scopeSelect.addEventListener("change", boundScopeChangeHandler, false);

I don't think we need these unless we have different parameters for different scopes (which we don't) or save all settings on each change (which does not seem right to me).

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:225
> +        this._scopeSelect.add(option);

Let's sort them alphabetically here.

> Source/WebCore/inspector/front-end/ResourcesSearchScope.js:33
> +WebInspector.ResourcesSearchScope = function(id, title, filter)

This looks very similar to ScriptsSearchScope. Can we extract something like FileBasedSearchScope with common search  in multiple files performing logic?

[Pavel Feldman]

Comment on attachment 116973 [details]
[PATCH] Added a file left behind in the previous patch

As per Vsevolod's comment. Please make sure to add a test case for this change as well.

[Alexander Pavlov (apavlov)]

Reassigning as agreed upon offline.

[Pavel Feldman]

I think I fixed it already.