[webkit-reviews] review granted: [Bug 231604] Web Inspector: Extract reusable logic from ResourceQueryController, ResourceQueryResult and ResourceQueryMatch : [Attachment 443815] Patch 1.1

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 231604: Web Inspector: Extract reusable logic from ResourceQueryController,
ResourceQueryResult and ResourceQueryMatch
https://bugs.webkit.org/show_bug.cgi?id=231604

Attachment 443815: Patch 1.1

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




--- Comment #8 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 443815
  --> https://bugs.webkit.org/attachment.cgi?id=443815
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)