Bug 231604 – Web Inspector: Extract reusable logic from ResourceQueryController, ResourceQueryResult and ResourceQueryMatch

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

Web Inspector already supports fuzzy matching for file paths in ResourceQueryController.
The matching logic is very similar to what's needed for CSS fuzzy matching.

Relevant parts from ResourceQueryController can be extracted into a class that can be shared between the two use cases.

[Radar WebKit Bug Importer]

<rdar://problem/84160281>

[Razvan Caliman]

Created attachment 440971 [details]
Patch v1.0

Introduce generic QueryController with reusable logic extracted from ResouceQueryController;
Add CSSQueryController with logic tuned for CSS properties and values;
This lays the supporting structure for work in Bug 230351 which makes CSS completions in the Styles details sidebar use fuzzy matching for property names and values

[Patrick Angle]

Comment on attachment 440971 [details]
Patch v1.0

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

> Source/WebInspectorUI/ChangeLog:69
> +        * UserInterface/Test.html:

Can we add some basic tests for CSSQueryController?

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:33
> +        this._specialCharacterIndicesForValueMap = new Map;

IMO it would be easier to understand what this map is for if it had `cached` somewhere in the name. e.g. `cachedSpecialCharacterIndicesForValueMap`

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:40
> +        console.assert(Array.isArray(values), values);

Should probably also assert this in the constructor then.

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:52
> +        query = query.removeWhitespace().toLowerCase();

Is removing whitespace necessary or desirable? I would have thought it do be a bad state if we got here with whitespace in the string?

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:74
> +    _findSpecialCharacterIndices(string)

I think this could just a static function on QueryController that takes a string and an array of separators, instead of having near duplicate logic here and in ResourceQueryController.

> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:92
>      _findSpecialCharacterIndices(string)

See CSSQueryController.js:74

> Source/WebInspectorUI/UserInterface/Main.html:929
> +    <script src="Controllers/QueryController.js"></script>
> +    <script src="Controllers/CSSQueryController.js"></script>
>      <script src="Controllers/ResourceQueryController.js"></script>

Can we move these to below where we include the rest of the controllers, or at least define them in their own section of Main.html if they have to be included sooner?

> Source/WebInspectorUI/UserInterface/Models/ResourceQueryResult.js:30
>          console.assert(matches.length, "Query matches list can't be empty.");

This will be asserted by the call to super() below.

[Devin Rousso]

Comment on attachment 440971 [details]
Patch v1.0

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

> Source/WebInspectorUI/ChangeLog:3
> +        Web Inspector: Add CSSQueryController to support fuzzy matching

It's a bit odd for this patch to introduce a class that's not actually used anywhere.  I think I'd rather have the class be introduced at the same time that it's used.  Perhaps we can have the first patch split out the logic into the base class and then the second patch add the new subclass with its usage?

>> Source/WebInspectorUI/ChangeLog:69
>> +        * UserInterface/Test.html:
> 
> Can we add some basic tests for CSSQueryController?

definitely want some tests

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:28
> +    constructor(values = [])

Style: we usually prefer to have the fallback be where the variable is used as JS parameter defaults only apply when the argument is not `undefined`

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:41
> +        if (!values.length)

NIT: this check is unnecessary

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:44
> +        this._values.pushAll(values);

NIT: Do we want to `console.assert` that there are no duplicates?

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:73
> +

Style: missing a `// Private`

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:79
> +        const separators = "-";

How does this handle repeated CSS variables that have repeated -?

> Source/WebInspectorUI/UserInterface/Controllers/CSSQueryController.js:95
> +                let previousCharacterIsSeparator = separators.includes(previousCharacter);

NIT: I'd inline this

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:26
> +WI.QueryController = class QueryController extends WI.Object

While we're here, does this actually need to be `WI.Object`?  AFAICT it doesn't use any events.

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:32
> +

Style: missing a `// Public`

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:33
> +    executeQuery(query) {

Style: `{` on separate line

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:34
> +        // Implemented by subclasses.

NIT: if the base class isn't gonna do anything and the subclass isn't gonna call `super.executeQuery(...)` then I'd replace this with
```
    throw WI.NotImplementedError.subclassMustOverride();
```

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:46
> +        let deadBranches = new Array(query.length).fill(Infinity);

Style: I like to wrap any `new` with `(` `)` so it's clear what the next `.` is operating on
```
    (new Array(query.length)).fill(Infinity)
```

> Source/WebInspectorUI/UserInterface/Models/QueryMatch.js:44
> +    Special: Symbol("special")

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Models/QueryResult.js:30
> +        console.assert(matches.length, "Query matches list can't be empty.");

NIT: the text is redundant

> Source/WebInspectorUI/UserInterface/Models/ResourceQueryResult.js:38
> +    get resource() { return this._value; }

This is a private member of a baseclass.  Rather than use it here, can we find all places where `.resource` is used and switch to `.value` instead?

[Razvan Caliman]

Retitled:
Web Inspector: Extract reusable logic from ResourceQueryController, ResourceQueryResult and ResourceQueryMatch

Was:
Add CSSQueryController to support fuzzy matching

[Razvan Caliman]

Comment on attachment 440971 [details]
Patch v1.0

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

>> Source/WebInspectorUI/ChangeLog:3
>> +        Web Inspector: Add CSSQueryController to support fuzzy matching
> 
> It's a bit odd for this patch to introduce a class that's not actually used anywhere.  I think I'd rather have the class be introduced at the same time that it's used.  Perhaps we can have the first patch split out the logic into the base class and then the second patch add the new subclass with its usage?

Ok. I split this patch to introduce just the generalized QueryController logic and update ResourceQueryController accordingly.
The CSSQueryController logic will come in Bug 230351

>>> Source/WebInspectorUI/ChangeLog:69
>>> +        * UserInterface/Test.html:
>> 
>> Can we add some basic tests for CSSQueryController?
> 
> definitely want some tests

Coming in Bug 230351

>> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:26
>> +WI.QueryController = class QueryController extends WI.Object
> 
> While we're here, does this actually need to be `WI.Object`?  AFAICT it doesn't use any events.

Indeed, no need to extend `WI.Object`. This was just copy-pasta from the original.

>> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:32
>> +
> 
> Style: missing a `// Public`

Done

>> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:33
>> +    executeQuery(query) {
> 
> Style: `{` on separate line

Done

>> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:34
>> +        // Implemented by subclasses.
> 
> NIT: if the base class isn't gonna do anything and the subclass isn't gonna call `super.executeQuery(...)` then I'd replace this with
> ```
>     throw WI.NotImplementedError.subclassMustOverride();
> ```

Good idea! Done.

>> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:46
>> +        let deadBranches = new Array(query.length).fill(Infinity);
> 
> Style: I like to wrap any `new` with `(` `)` so it's clear what the next `.` is operating on
> ```
>     (new Array(query.length)).fill(Infinity)
> ```

Done. Eslint complains but it's not that big of a deal:

```
eslint: error
no-extra-parens - Unnecessary parentheses around expression.
```

>> Source/WebInspectorUI/UserInterface/Controllers/ResourceQueryController.js:92
>>      _findSpecialCharacterIndices(string)
> 
> See CSSQueryController.js:74

I'll chose to keep `findSpecialCharacterIndices()` independent between `CSSQueryController` and `ResourceController`. 
This is one of the bits we'll likely tweak to suit CSS property matching. For example, uppercase doesn't matter _unless_ it's a CSS variable name.

>> Source/WebInspectorUI/UserInterface/Main.html:929
>>      <script src="Controllers/ResourceQueryController.js"></script>
> 
> Can we move these to below where we include the rest of the controllers, or at least define them in their own section of Main.html if they have to be included sooner?

I'm not sure I understand. This block is where controllers are loaded.

>> Source/WebInspectorUI/UserInterface/Models/QueryMatch.js:44
>> +    Special: Symbol("special")
> 
> Style: missing trailing `,`

Done.

>> Source/WebInspectorUI/UserInterface/Models/ResourceQueryResult.js:30
>>          console.assert(matches.length, "Query matches list can't be empty.");
> 
> This will be asserted by the call to super() below.

Indeed. Removed.

>> Source/WebInspectorUI/UserInterface/Models/ResourceQueryResult.js:38
>> +    get resource() { return this._value; }
> 
> This is a private member of a baseclass.  Rather than use it here, can we find all places where `.resource` is used and switch to `.value` instead?

I updated it to use the public member `this.value`.

I'd prefer to keep this mapping and avoid changing the tests and `WI.OpenResourceDialog`.
It makes code more understandable to have something call e.g. `result.resource.displayName` since `displayName` is a member of `WI.Resource`.

If you feel strongly, I can update the consumers.

[Razvan Caliman]

Created attachment 443815 [details]
Patch 1.1

Split patch to introduce just the generalization of fuzzy matching logic. Address code review feedback

[Devin Rousso]

Comment on attachment 443815 [details]
Patch 1.1

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

r=me

> Source/WebInspectorUI/UserInterface/Main.html:927
> +    <script src="Controllers/QueryController.js"></script>

I think we should move this below `CodeMirrorEditingController.js` so that it's for sure declared before any possible subclasses that use it.  Right now, it's only pure luck that we happen to create `WI.QueryController` before using it in `WI.ResourceQueryController` simply because Q < R 😅

> Source/WebInspectorUI/UserInterface/Models/QueryMatch.js:28
> +    constructor(type, index, queryIndex)

Can/Should we add some basic assertions?
```
    console.assert(Object.values(WI.QueryMatch.Type).includes(type), type);
    console.assert(index >= 0, index);
    console.assert(queryIndex >= 0, queryIndex);
```

> Source/WebInspectorUI/UserInterface/Models/QueryResult.js:43
> +        if (this._rank === undefined)
> +            this._calculateRank();

```
    this._rank ??= this._calculateRank();
```

> Source/WebInspectorUI/UserInterface/Models/QueryResult.js:51
> +        if (!this._matchingTextRanges)
> +            this._matchingTextRanges = this._createMatchingTextRanges();

```
    this._matchingTextRanges ??= this._createMatchingTextRanges();
```

> Source/WebInspectorUI/UserInterface/Models/ResourceQueryResult.js:30
> +        super(resource, matches);

ditto (Source/WebInspectorUI/UserInterface/Models/QueryMatch.js:28)
```
    console.assert(resource instanceof WI.Resource, resource);
```

> Source/WebInspectorUI/UserInterface/Test.html:268
> +    <script src="Controllers/QueryController.js"></script>

ditto (Source/WebInspectorUI/UserInterface/Main.html:927)

[Devin Rousso]

Comment on attachment 443815 [details]
Patch 1.1

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

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:48
> +        {

NIT: we only require that `{` be on a separate line for top-level functions, not locals/lambdas :)

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:55
> +        {

ditto (:48)

> Source/WebInspectorUI/UserInterface/Controllers/QueryController.js:79
> +        {

ditto (:48)

[Razvan Caliman]

Created attachment 443947 [details]
Patch 1.2

Carry over R+; Address code review feedback

[Razvan Caliman]

Created attachment 443961 [details]
Patch 1.3

Carry over R+; Fix condition for failing test.

[EWS]

Committed r285711 (244170@main): <https://commits.webkit.org/244170@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443961 [details].