Bug 188270

<rdar://problem/42867498>

Hmm, the backend points to this resource twice in the result of `Page.searchInResources`:

    [Log] request: {
        "id": 596,
        "method": "Page.searchInResources",
        "params": {
            "text": "touchMoved:",
            "caseSensitive": false,
            "isRegex": false
        }
    }

    [Log] response: {
        "result": {
            "result": [{
                "url": "https://www.cio.com/www.idge.ans/js/select2-3.5.0/select2.js",
                "frameId": "0.2",
                "matchesCount": 2
            }, {
                "url": "https://www.cio.com/www.idge.ans/js/select2-3.5.0/select2.js",
                "frameId": "0.2",
                "matchesCount": 2,
                "requestId": "0.857"
            }]
        },
        "id": 596
    }

Looks to be the same resource, why is it getting duplicated?

Page.searchInResources does two searches:

  (1) PageAgent search cached resources in frame tree
  (2) NetworkAgent searchOtherRequests to search non-cached resources

So it would seem (2) may be searching in a resource that has a CachedResource that could otherwise be skipped.

(In reply to Joseph Pecoraro from comment #3)
> Page.searchInResources does two searches:
> 
>   (1) PageAgent search cached resources in frame tree
>   (2) NetworkAgent searchOtherRequests to search non-cached resources
> 
> So it would seem (2) may be searching in a resource that has a
> CachedResource that could otherwise be skipped.

Hmm, unfortunately that suggested change doesn't work for some Worker/XHR resources, which this test catches:
LayoutTests/inspector/page/searchInResources.html

Worst case the frontend can avoid duplicates. That might be an easy fix for now, I'm not sure off-hand what the backend can do to address this.

Created attachment 346432 [details]
[PATCH] Proposed Fix

Filed a follow-up for a backend fix. (I'm not investigating that right now)
https://bugs.webkit.org/show_bug.cgi?id=188287

Comment on attachment 346432 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=346432&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:175
> +            for (let i = 0; i < result.length; ++i) {

Alternatively, use reduce()

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:180
> +                // FIXME: Backend sometimes searches files twice.

Nit: add link to backend bug

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:182
> +                let key = searchResult.frameId + ":" + searchResult.url;

Nit: could use destructuring of searchResult above and template literal. Or not, doesn't matter.

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:187
>                  // COMPATIBILITY (iOS 9): Page.searchInResources did not have the optional requestId parameter.

Not a blocker, but... it seems problematic that the View should be doing this sort of backend-specific deduplication logic at all. It may make sense to introduce a SearchManager or some other model/controller object to make requests and keep track of cached search results apart from the sidebar view class. That would also make this testable.

If there is any reason to wait on the searchInResource backend command to finish, we could iterate preventDuplicates and issue all the commands at once and use Promise.all to wait the results.

Comment on attachment 346432 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=346432&action=review

>> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:180
>> +                // FIXME: Backend sometimes searches files twice.
> 
> Nit: add link to backend bug

Will do.

>> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:187
>>                  // COMPATIBILITY (iOS 9): Page.searchInResources did not have the optional requestId parameter.
> 
> Not a blocker, but... it seems problematic that the View should be doing this sort of backend-specific deduplication logic at all. It may make sense to introduce a SearchManager or some other model/controller object to make requests and keep track of cached search results apart from the sidebar view class. That would also make this testable.
> 
> If there is any reason to wait on the searchInResource backend command to finish, we could iterate preventDuplicates and issue all the commands at once and use Promise.all to wait the results.

I think we want to issue each of the backend requests individually such that we populate the UI incrementally with each result. It is effectively chunking the work for us.

https://trac.webkit.org/changeset/234637/webkit