Bug 59596 – Web Inspector: Move Resources Panel search to backend

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

Move Resources Panel search to backend. 
We should not need to request all content to do search.

[Vsevolod Vlasov]

Created attachment 91267 [details]
Patch

[Pavel Feldman]

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 :)

[Vsevolod Vlasov]

Created attachment 91453 [details]
Patch with fixes

[Vsevolod Vlasov]

> > 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.

[Alexander Pavlov (apavlov)]

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))

[Vsevolod Vlasov]

Created attachment 91495 [details]
Patch + matchesCount renaming + fixes

[Vsevolod Vlasov]

Created attachment 91499 [details]
Patch (rebaselined, refactored)

[Pavel Feldman]

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.

[Vsevolod Vlasov]

Created attachment 91676 [details]
Patch with fixes

[WebKit Review Bot]

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.

[Vsevolod Vlasov]

Created attachment 91680 [details]
Patch

[Pavel Feldman]

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.

[Vsevolod Vlasov]

Created attachment 93433 [details]
Patch with fixes

[Vsevolod Vlasov]

Done.

> > Source/WebCore/inspector/InspectorPageAgent.cpp:531
> > +void InspectorPageAgent::domContentEventFired()
> 
> I thought these were already a part of the file.
Rebaselined

[Vsevolod Vlasov]

> > 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.

[WebKit Commit Bot]

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

[Vsevolod Vlasov]

Created attachment 93631 [details]
Fixed mac build

[WebKit Commit Bot]

Comment on attachment 93631 [details]
Fixed mac build

Clearing flags on attachment: 93631

Committed r86562: <http://trac.webkit.org/changeset/86562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.