Summary: | Web Inspector: Move Resources Panel search to backend | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vsevolod Vlasov <vsevik> | ||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, commit-queue, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Vsevolod Vlasov
2011-04-27 04:30:10 PDT
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. |