Bug 59596

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 Flags
Patch
pfeldman: review-, pfeldman: commit-queue-
Patch with fixes
none
Patch + matchesCount renaming + fixes
none
Patch (rebaselined, refactored)
pfeldman: review-
Patch with fixes
none
Patch
none
Patch with fixes
pfeldman: review+, commit-queue: commit-queue-
Fixed mac build none

Description Vsevolod Vlasov 2011-04-27 04:30:10 PDT
Move Resources Panel search to backend. 
We should not need to request all content to do search.
Comment 1 Vsevolod Vlasov 2011-04-27 04:36:55 PDT
Created attachment 91267 [details]
Patch
Comment 2 Pavel Feldman 2011-04-27 05:34:39 PDT
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 :)
Comment 3 Vsevolod Vlasov 2011-04-28 03:04:30 PDT
Created attachment 91453 [details]
Patch with fixes
Comment 4 Vsevolod Vlasov 2011-04-28 03:08:47 PDT
> > 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 5 Alexander Pavlov (apavlov) 2011-04-28 03:32:31 PDT
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))
Comment 6 Vsevolod Vlasov 2011-04-28 08:28:10 PDT
Created attachment 91495 [details]
Patch + matchesCount renaming + fixes
Comment 7 Vsevolod Vlasov 2011-04-28 08:49:29 PDT
Created attachment 91499 [details]
Patch (rebaselined, refactored)
Comment 8 Pavel Feldman 2011-04-28 10:43:17 PDT
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.
Comment 9 Vsevolod Vlasov 2011-04-29 07:05:33 PDT
Created attachment 91676 [details]
Patch with fixes
Comment 10 WebKit Review Bot 2011-04-29 07:07:01 PDT
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.
Comment 11 Vsevolod Vlasov 2011-04-29 07:55:48 PDT
Created attachment 91680 [details]
Patch
Comment 12 Pavel Feldman 2011-05-12 13:02:15 PDT
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.
Comment 13 Vsevolod Vlasov 2011-05-13 04:46:34 PDT
Created attachment 93433 [details]
Patch with fixes
Comment 14 Vsevolod Vlasov 2011-05-13 04:47:31 PDT
Done.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:531
> > +void InspectorPageAgent::domContentEventFired()
> 
> I thought these were already a part of the file.
Rebaselined
Comment 15 Vsevolod Vlasov 2011-05-13 04:52:59 PDT
> > 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 16 WebKit Commit Bot 2011-05-13 12:59:43 PDT
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
Comment 17 Vsevolod Vlasov 2011-05-16 03:43:54 PDT
Created attachment 93631 [details]
Fixed mac build
Comment 18 WebKit Commit Bot 2011-05-16 05:57:09 PDT
Comment on attachment 93631 [details]
Fixed mac build

Clearing flags on attachment: 93631

Committed r86562: <http://trac.webkit.org/changeset/86562>
Comment 19 WebKit Commit Bot 2011-05-16 05:57:16 PDT
All reviewed patches have been landed.  Closing bug.