[webkit-reviews] review denied: [Bug 59596] Web Inspector: Move Resources Panel search to backend : [Attachment 91267] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 59596: Web Inspector: Move Resources Panel search to backend
https://bugs.webkit.org/show_bug.cgi?id=59596

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

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

> Source/WebCore/inspector/Inspector.json:101
> +		   "id": "ResourceSearchResult",

"SearchResult"

> Source/WebCore/inspector/Inspector.json:108
> +			       "properties": [

I'd suggest making "SearchResult" type for this item and return array of these
in the protocol method.

> Source/WebCore/inspector/Inspector.json:183
> +		       { "name": "query", "type": "string", "description":
"String to search for."  },

query -> "text" or "regex"

> Source/WebCore/inspector/Inspector.json:184
> +		       { "name": "ignoreCase", "type": "boolean",
"description": "Specifies whether case should be ignored or not."  }

"optional": true

> Source/WebCore/inspector/Inspector.json:187
> +		       { "name": "searchResult", "$ref":
"ResourceSearchResult", "description": "Resource content search result." }

name: "result", "type":"array", "items": { "$ref":"SearchResult" }

> Source/WebCore/inspector/InspectorPageAgent.cpp:478
> +int InspectorPageAgent::countRegularExpressionMatches(const
RegularExpression& regex, const String& content)

You could define static functions in WebCore namespace for helper routines.

> Source/WebCore/inspector/InspectorPageAgent.cpp:483
> +    while ((position = regex.match(content, start)) != -1) {

Sounds like you could provide offsets easily too.

> Source/WebCore/inspector/InspectorPageAgent.cpp:501
> +PassRefPtr<InspectorObject>
InspectorPageAgent::buildObjectForSearchMatch(Frame* frame, const String& url,
int matches)

ditto

> Source/WebCore/inspector/InspectorPageAgent.cpp:513
> +    const CachedResourceLoader::DocumentResourceMap& allResources =
frame->document()->cachedResourceLoader()->allCachedResources();

We already have this logic somewhere. Could you reuse it?

> Source/WebCore/inspector/InspectorPageAgent.cpp:520
> +	   if (equalIgnoringFragmentIdentifier(kurl,
frame->loader()->iconURL()))

Why this check?

> Source/WebCore/inspector/InspectorPageAgent.cpp:530
> +	       matches = countMatchesInResourceContent(frame, kurl, regex);

You could shortcut and use cached resource instance to get its content.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:730
> +		   var match = searchResult.matchesList[i];

this code is way too messy. you should refactor it.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:748
> +	       PageAgent.searchInResources(regexes[i].source,
regexes[i].ignoreCase, callback.bind(this));

can there be multiple search queries?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:826
> +    jumpToNextSearchResult: function()

It looks like too much code. I am sure it can be simpler :)