[webkit-reviews] review denied: [Bug 59596] Web Inspector: Move Resources Panel search to backend : [Attachment 91499] Patch (rebaselined, refactored)

[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 91499: Patch (rebaselined, refactored)
https://bugs.webkit.org/attachment.cgi?id=91499&action=review

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

> Source/WebCore/inspector/InspectorPageAgent.cpp:424
> +	   result.append("[");

"^" will convert into [^], will it work?

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

Could you break this down for better readability?

> Source/WebCore/inspector/InspectorPageAgent.cpp:458
> +void InspectorPageAgent::searchInResources(ErrorString* errorString, const
String& text, const bool* const optionalCaseSensitive, const bool* const
optionalIsRegex, RefPtr<InspectorArray>* object)

Order of methods in the .cpp should match the one in .h when possible.

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

It would be great if we could extract cached resource traversal logic into one
place. I think I mentioned it earlier.

> Source/WebCore/inspector/InspectorPageAgent.cpp:474
> +	       switch (InspectorPageAgent::cachedResourceType(*cachedResource))
{

Ditto. Can we extract method for this logic (and use
::resourceContent(cachedResource))?

> Source/WebCore/inspector/front-end/ResourcesPanel.js:690
> +	   function searchInEditedResource(treeElement)

Lets add it in a separate patch.