[webkit-reviews] review granted: [Bug 188270] Web Inspector: Global search sometimes returns duplicate results for a resource : [Attachment 346432] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 188270: Web Inspector: Global search sometimes returns duplicate results
for a resource
https://bugs.webkit.org/show_bug.cgi?id=188270

Attachment 346432: [PATCH] Proposed Fix

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




--- Comment #9 from Brian Burg <bburg at apple.com> ---
Comment on attachment 346432
  --> https://bugs.webkit.org/attachment.cgi?id=346432
[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.