Bug 177981

Summary: Web Inspector: Network Tab - Search Headers Detail View
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 177896    
Bug Blocks:    
Attachments:
Description Flags
[IMAGE] Headers - search
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix bburg: review+

Description Joseph Pecoraro 2017-10-05 17:19:29 PDT
Network Tab - Search Headers Detail View

Should be able to search to find particular headers or search forward/back.
Comment 1 Joseph Pecoraro 2017-10-05 17:19:48 PDT
Created attachment 322943 [details]
[IMAGE] Headers - search
Comment 2 Joseph Pecoraro 2017-10-05 17:21:16 PDT
Created attachment 322944 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2017-10-06 16:52:20 PDT
Created attachment 323066 [details]
[PATCH] Proposed Fix

Rebaselined.
Comment 4 Devin Rousso 2017-10-06 21:37:21 PDT
Comment on attachment 323066 [details]
[PATCH] Proposed Fix

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

I'm pretty unfamiliar with the way we do search/find banners, so I am probably not the best to review this.  :(

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:419
> +            let match = searchRegex.exec(text);
> +            while (match) {
> +                matchRanges.push({offset: match.index, length: match[0].length});
> +                match = searchRegex.exec(text);
> +            }

You could borrow a thing from C++ and assign inside the condition.

    let match = null;
    while (match = searchRegex.exec(text))
        matchRanges.push({offset: match.index, length: match[0].length});

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:441
> +        this._bouncyHighlightElement = document.createElement("div");
> +        this._bouncyHighlightElement.className = "bouncy-highlight";
> +        this._bouncyHighlightElement.textContent = highlightElement.textContent;

This seems to be the pattern we follow with `_boundyHighlightElement`, but it seems like a waste to keep creating an element with the exact same style/function.  Instead of recreating the element, we could just remove it, set the text/top/left, and then add to have it restart the animation.
Comment 5 Joseph Pecoraro 2017-10-08 14:38:34 PDT
Created attachment 323142 [details]
[PATCH] Proposed Fix
Comment 6 Radar WebKit Bug Importer 2017-10-08 15:19:06 PDT
<rdar://problem/34877133>
Comment 7 BJ Burg 2017-10-09 10:26:16 PDT
Comment on attachment 323142 [details]
[PATCH] Proposed Fix

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

r=me

We might benefit from some sort of SearchController[Delegate] to extract out the common search related state thats duplicated in each content view that overrides the various ContentView search methods. That might give us a uniform search delegate protocol between ContentViews and Sidebars.

> Source/WebInspectorUI/ChangeLog:11
> +        Rename "Dom" to "DOM" in utility function.

OK

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:43
> +        this._searchDOMChanges = [];

Nit: searchDOMHighlightChanges to match other content views?

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:428
> +        let highlightElement = this._searchResults[index];

Nit: highlightedElement
Comment 8 Joseph Pecoraro 2017-10-09 11:10:36 PDT
> We might benefit from some sort of SearchController[Delegate] to extract out
> the common search related state thats duplicated in each content view that
> overrides the various ContentView search methods. That might give us a
> uniform search delegate protocol between ContentViews and Sidebars.

I agree. This was a little tricky, if this pops up in yet another place (its only 2 right now in the Console and here) I'll look into doing this. That said, each place has a bit of custom logic that may make this a bit messy.
Comment 9 Joseph Pecoraro 2017-10-09 12:28:34 PDT
<https://trac.webkit.org/r223057>