Bug 155324 – Web Inspector: Improve filtering in OpenResourceDialog

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Improve filtering in OpenResourceDialog. Currently the dialog matches the filterable text for each resource against three hardcoded filters:

1. Substring starting from the beginning (case-insensitive)
2. Substring anywhere (case-insensitive)
3. Uppercased substring matches the start of words:
   'MRF' => 'MyResourceFile'

Filters should be added/updated to match the "quick open" behavior of many popular editors (Xcode, SublimeText, TextMate, WebStorm, Chrome dev tools, etc).

[Radar WebKit Bug Importer]

<rdar://problem/25094504>

[Matt Baker]

Created attachment 275718 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 275720 [details]
[Image] Results from Web Inspector, TextMate, SublimeText

[Matt Baker]

Created attachment 275724 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 275724 [details]
[Patch] Proposed Fix

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

Very clean!

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:491
> +        return String(this) === this.toLowerCase();

I wonder if a regex would be better? I guess that would be hard to craft…

Did to you need the String(this)? this === this.toLowerCase() didn't work?

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:78
> +

Drop empty line between the two returns. (I normally say add newlines, I know!)

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:163
> +        const filenameSeparators = "_,.-";

Comma, really?

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:172
> +            let c = string[i];

char or character

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:262
> +            if (previousMatch && previousMatch.index === (match.index - 1))

No need for the ().

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:309
> +            let gap = " ".repeat((match.index - lastIndex) - 1);

No need for the ().

[Matt Baker]

(In reply to comment #5)
> Comment on attachment 275724 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275724&action=review
> 
> Very clean!
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:491
> > +        return String(this) === this.toLowerCase();
> 
> I wonder if a regex would be better? I guess that would be hard to craft…
> 
> Did to you need the String(this)? this === this.toLowerCase() didn't work?
It is needed.

> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:78
> > +
> 
> Drop empty line between the two returns. (I normally say add newlines, I
> know!)
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:163
> > +        const filenameSeparators = "_,.-";
> 
> Comma, really?
I've never seen a comma used to separate a filename in the wild. I'm ok with dropping it.

> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:172
> > +            let c = string[i];
> 
> char or character
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:262
> > +            if (previousMatch && previousMatch.index === (match.index - 1))
> 
> No need for the ().
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:309
> > +            let gap = " ".repeat((match.index - lastIndex) - 1);
> 
> No need for the ().

[Joseph Pecoraro]

Comment on attachment 275718 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:489
> +    value: function()

Style: Lately I've been just doing "value()" instead of "value: function()".

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:491
> +        return String(this) === this.toLowerCase();

I don't think this needs to call String(this). We already expect `this` to be a String, because we are on String.prototype. Is there a case where this was necessary?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:499
> +        return String(this) === this.toUpperCase();

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:58
> +        query = query.replace(/[\s\xA0]+/g, "").toLowerCase();

This is pretty similar to our "collapseWhitespace" utility. Maybe you can add a "removeWhitespace" Utility alongside it. That said, do we really expect people to type whatever "\xA0" is? Maybe we can simplify to .replace(/\s/g, "").

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:126
> +            if (matches.length < 2)
> +                return false;

This looks like it would be a problem if we want to backtrack to do normal searching before a special character match.

Scenario:

    searchString: "TeamMate.js"
    queryString: "mm"

Currently we would get a special match at "M", fail to find a normal 'm' and backtracking would bail.

Seems we want this code here to do a backtrack, if (matches.length === 1 && matches[0].type === Special) => start search over from the beginning but with type Normal.

I haven't gotten to the patch with Tests yet, but it would be good to have a test for this.

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:156
> +    // Private

Nit: Put this about _findQueryMatches.

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:217
> +WebInspector.QueryResult = class QueryResult extends WebInspector.Object

Nit: This feels like a Model class. Move it to its own file?

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:234
> +        if (!this._rank)
> +            this._calculateRank();

This will recalculate the rank if rank is 0. That doesn't seem intentional, and can be pretty bad since .rank is used in sort() and you wouldn't want to recompute it over and over.

Perhaps this._rank should be initialized to undefined in the constructor, with a "=== undefined" check here instead of the falsey check. Or (after reading below) maybe -Infinity, Number.MIN_VALUE, etc.

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:269
> +            this._rank -= match.index;

So this could produce a negative value? As long as this means something with no matches is not sorted before it.

Scenario:

    searchString: "aaaaaaaaaaaaaaaz.js"
    queryString: "z"

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:302
> +    __test_createMatchesMask()

This implies this would be used by testing code.

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:309
> +            let gap = new Array((match.index - lastIndex) - 1).fill(" ").join("");

Nit: I think this is just " ".repeat(match.index - lastIndex)

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:61
>          this._resources = [];

Looks like you can remove _resources. It does not appear to be used anymore.

> Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:137
> +                this._queryController.addResource(resource);

Hmm, so every time we present the dialog we add each resource, which clears the queryController's cached data for the resource.

Currently the cached data for a resource will never change (it only uses Resource.displayName) so maybe QueryController.prototype.addResource could do nothing if the resource was already added.

Also, I don't see calls to queryController.reset() or removeResource(resource). So, given this scenario:
1. Inspect Page A
2. Trigger OpenResourceDialog
3. Navigate Page A to Page B
4. Trigger OpenResourceDialog

Will the 2nd resource dialog show me resources from Page A that had otherwise gone away? This would also leak/grow.

[Joseph Pecoraro]

Comment on attachment 275724 [details]
[Patch] Proposed Fix

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

Excellent test!

> LayoutTests/inspector/unit-tests/resource-query-controller.html:22
> +                {filename: "AxBxCxDxExFx", indices: [0, 2, 4, 6, 8, 10]},

Test could be visual like:

    {
        filename: "a-bc_de.f"
        expected: "^^^ ^^ ^^"
    }

> LayoutTests/inspector/unit-tests/resource-query-controller.html:109
> +                {query: "abcd",  filename: "abcde", matchesMask: "abcd"},
> +                {query: " abcd",  filename: "abcde", matchesMask: "abcd"},
> +                {query: "abcd ",  filename: "abcde", matchesMask: "abcd"},
> +                {query: "a b c d",  filename: "abcde", matchesMask: "abcd"},
> +                {query: "a-bcde", filename: "abcde-abcde", matchesMask: "a    - bcde"},
> +                {query: "abcde", filename: "AaBbCcDdEe", matchesMask: "A B C D E"},
> +                {query: "abcde", filename: "AbcdBcdCdDe", matchesMask: "A   B  C De"},
> +                {query: "abcdex", filename: "AxBxCxdxexDxyxEF", matchesMask: "A B C d ex"},

These would read easier if the filename and mask are right next to each other:

    {
        query: "abcd",
        filename: "AbcdBcdCdDe",
        expected: "A   B  C De",
    }

Here (with a monospace font) it becomes very easy to see the matches. Side by side the number of spaces is tricky.

Also add a test for the reverse duplicate character case. So instead of "AaBbCcDdEe", "aAbBcCdDeE" to address the "mm" in "TeamMate.js" scenario.

[Matt Baker]

(In reply to comment #7)
> Comment on attachment 275718 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275718&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:489
> > +    value: function()
> 
> Style: Lately I've been just doing "value()" instead of "value: function()".

I haven't seen this style in use outside of class methods. Example?

> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:491
> > +        return String(this) === this.toLowerCase();
> 
> I don't think this needs to call String(this). We already expect `this` to
> be a String, because we are on String.prototype. Is there a case where this
> was necessary?
> 
> > Source/WebInspectorUI/UserInterface/Base/Utilities.js:499
> > +        return String(this) === this.toUpperCase();
> 
> Ditto.

Without the conversion to String, strict equality always fails. Debugging the property shows the reason:

typeof this === "object" && typeof this.toLowerCase() === "string"

> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:58
> > +        query = query.replace(/[\s\xA0]+/g, "").toLowerCase();
> 
> This is pretty similar to our "collapseWhitespace" utility. Maybe you can
> add a "removeWhitespace" Utility alongside it. That said, do we really
> expect people to type whatever "\xA0" is? Maybe we can simplify to
> .replace(/\s/g, "").

I like making this a utility function. The collapseWhitespace function includes a check for \xA0 (non-breaking space), so it makes sense to be consistent. In practice a space (non-breaking or otherwise) is unlikely input in the Quick Open dialog.

> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:126
> > +            if (matches.length < 2)
> > +                return false;
> 
> This looks like it would be a problem if we want to backtrack to do normal
> searching before a special character match.
> 
> Scenario:
> 
>     searchString: "TeamMate.js"
>     queryString: "mm"
> 
> Currently we would get a special match at "M", fail to find a normal 'm' and
> backtracking would bail.
> 
> Seems we want this code here to do a backtrack, if (matches.length === 1 &&
> matches[0].type === Special) => start search over from the beginning but
> with type Normal.

Good catch!

> I haven't gotten to the patch with Tests yet, but it would be good to have a
> test for this.
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:156
> > +    // Private
> 
> Nit: Put this about _findQueryMatches.
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:217
> > +WebInspector.QueryResult = class QueryResult extends WebInspector.Object
> 
> Nit: This feels like a Model class. Move it to its own file?
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:234
> > +        if (!this._rank)
> > +            this._calculateRank();
> 
> This will recalculate the rank if rank is 0. That doesn't seem intentional,
> and can be pretty bad since .rank is used in sort() and you wouldn't want to
> recompute it over and over.
> 
> Perhaps this._rank should be initialized to undefined in the constructor,
> with a "=== undefined" check here instead of the falsey check. Or (after
> reading below) maybe -Infinity, Number.MIN_VALUE, etc.
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:269
> > +            this._rank -= match.index;
> 
> So this could produce a negative value? As long as this means something with
> no matches is not sorted before it.
> 
> Scenario:
> 
>     searchString: "aaaaaaaaaaaaaaaz.js"
>     queryString: "z"
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:302
> > +    __test_createMatchesMask()
> 
> This implies this would be used by testing code.
> 
> > Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:309
> > +            let gap = new Array((match.index - lastIndex) - 1).fill(" ").join("");
> 
> Nit: I think this is just " ".repeat(match.index - lastIndex)
> 
> > Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:61
> >          this._resources = [];
> 
> Looks like you can remove _resources. It does not appear to be used anymore.
> 
> > Source/WebInspectorUI/UserInterface/Views/OpenResourceDialog.js:137
> > +                this._queryController.addResource(resource);
> 
> Hmm, so every time we present the dialog we add each resource, which clears
> the queryController's cached data for the resource.
> 
> Currently the cached data for a resource will never change (it only uses
> Resource.displayName) so maybe QueryController.prototype.addResource could
> do nothing if the resource was already added.
> 
> Also, I don't see calls to queryController.reset() or
> removeResource(resource). So, given this scenario:
> 1. Inspect Page A
> 2. Trigger OpenResourceDialog
> 3. Navigate Page A to Page B
> 4. Trigger OpenResourceDialog
> 
> Will the 2nd resource dialog show me resources from Page A that had
> otherwise gone away? This would also leak/grow.

[Joseph Pecoraro]

> > > Source/WebInspectorUI/UserInterface/Base/Utilities.js:491
> > > +        return String(this) === this.toLowerCase();
> > 
> > I don't think this needs to call String(this). We already expect `this` to
> > be a String, because we are on String.prototype. Is there a case where this
> > was necessary?
> > 
> 
> Without the conversion to String, strict equality always fails. Debugging
> the property shows the reason:
> 
> typeof this === "object" && typeof this.toLowerCase() === "string"

Ahh, okay. What you have looks good then!

[Matt Baker]

Created attachment 275842 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 275842 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 275842

Committed r199143: <http://trac.webkit.org/changeset/199143>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 275842 [details]
[Patch] Proposed Fix

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

> LayoutTests/inspector/unit-tests/resource-query-controller-expected.txt:132
> +PASS: Permutation "aBcDE".
> +PASS: Permutation "ABcDE".
> +PASS: Permutation "abCDE".
> +PASS: Permutation "AbCDE".
> +PASS: Permutation "aBCDE".
> +PASS: Permutation "ABCDE".

Do we really want all of these in the output?