Bug 177981 - Web Inspector: Network Tab - Search Headers Detail View
Summary: Web Inspector: Network Tab - Search Headers Detail View
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on: 177896
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-05 17:19 PDT by Joseph Pecoraro
Modified: 2017-10-09 12:28 PDT (History)
6 users (show)

See Also:


Attachments
[IMAGE] Headers - search (223.42 KB, image/png)
2017-10-05 17:19 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (9.58 KB, patch)
2017-10-05 17:21 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (9.60 KB, patch)
2017-10-06 16:52 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (9.37 KB, patch)
2017-10-08 14:38 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>