Move Resources Panel search to backend. We should not need to request all content to do search.
Created attachment 91267 [details] Patch
Comment on attachment 91267 [details] Patch 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 :)
Created attachment 91453 [details] Patch with fixes
> > Source/WebCore/inspector/Inspector.json:101 > > + "id": "ResourceSearchResult", > > "SearchResult" Comments about protocol addressed. > > 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. Done. > > Source/WebCore/inspector/InspectorPageAgent.cpp:483 > > + while ((position = regex.match(content, start)) != -1) { > > Sounds like you could provide offsets easily too. I could if we need that. We do not need that on front-end currently. > > Source/WebCore/inspector/InspectorPageAgent.cpp:513 > > + const CachedResourceLoader::DocumentResourceMap& allResources = frame->document()->cachedResourceLoader()->allCachedResources(); > > We already have this logic somewhere. Could you reuse it? This is rewritten using frame traversing. > > Source/WebCore/inspector/InspectorPageAgent.cpp:520 > > + if (equalIgnoringFragmentIdentifier(kurl, frame->loader()->iconURL())) > > Why this check? Removed. > > Source/WebCore/inspector/InspectorPageAgent.cpp:530 > > + matches = countMatchesInResourceContent(frame, kurl, regex); > > You could shortcut and use cached resource instance to get its content. Done. > > 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? Search changed to always use only one regular expression. > > Source/WebCore/inspector/front-end/ResourcesPanel.js:826 > > + jumpToNextSearchResult: function() > > It looks like too much code. I am sure it can be simpler :) Extracted logic for selecting next/previous search result.
Comment on attachment 91453 [details] Patch with fixes View in context: https://bugs.webkit.org/attachment.cgi?id=91453&action=review > Source/WebCore/inspector/Inspector.json:107 > + { "name": "matches", "type": "number", "description": "Number of matches in resource." } a better name for the property is "matchCount", as it is not immediately clear whether "matches" is boolean or holds the actual match results (in our frontend, we use everywhere: var matches = /regex/.match(string))
Created attachment 91495 [details] Patch + matchesCount renaming + fixes
Created attachment 91499 [details] Patch (rebaselined, refactored)
Comment on attachment 91499 [details] Patch (rebaselined, refactored) 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.
Created attachment 91676 [details] Patch with fixes
Attachment 91676 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/inspector/InspectorPageAgent.cpp:65: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4] Source/WebCore/inspector/InspectorPageAgent.cpp:65: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 91680 [details] Patch
Comment on attachment 91680 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91680&action=review Couple of nits, otherwise looks good. > Source/WebCore/inspector/Inspector.json:107 > + { "name": "matchesCount", "type": "number", "description": "Number of matches in resource content." } in _the_ resource content. > Source/WebCore/inspector/InspectorPageAgent.cpp:531 > +void InspectorPageAgent::domContentEventFired() I thought these were already a part of the file. > Source/WebCore/inspector/front-end/ResourcesPanel.js:706 > + var resource = frameTreeElement.resourceByURL(searchResult.url); You should check frame tree elements for null - they are very dynamic.
Created attachment 93433 [details] Patch with fixes
Done. > > Source/WebCore/inspector/InspectorPageAgent.cpp:531 > > +void InspectorPageAgent::domContentEventFired() > > I thought these were already a part of the file. Rebaselined
> > Source/WebCore/inspector/InspectorPageAgent.cpp:531 > > +void InspectorPageAgent::domContentEventFired() > > I thought these were already a part of the file. I checked once again and apparently the thing is I inserted some methods before these ones and diff treats them weirdly. These methods are not changed at all.
Comment on attachment 93433 [details] Patch with fixes Rejecting attachment 93433 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build'..." exit_code: 2 Last 500 characters of output: 20 setenv YACC /Developer/usr/bin/yacc /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh ** BUILD FAILED ** The following build commands failed: WebCore: CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/InspectorPageAgent.o /mnt/git/webkit-commit-queue/Source/WebCore/inspector/InspectorPageAgent.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/8695186
Created attachment 93631 [details] Fixed mac build
Comment on attachment 93631 [details] Fixed mac build Clearing flags on attachment: 93631 Committed r86562: <http://trac.webkit.org/changeset/86562>
All reviewed patches have been landed. Closing bug.