WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
227098
Web Inspector: Styles: should autocomplete `var()` and `attr()` values
https://bugs.webkit.org/show_bug.cgi?id=227098
Summary
Web Inspector: Styles: should autocomplete `var()` and `attr()` values
Devin Rousso
Reported
2021-06-16 15:07:44 PDT
# STEPS TO REPRODUCE 1. inspect any page 2. go to the Styles panel in the details sidebar in the Elements Tab 3. add a new CSS property 4. type `--xyz: red` 5. add a new CSS property 6. type `color: var(` # EXPECTED should show `--xyz` etc. in autocompletion # ACTUAL no autocompletions
Attachments
WIP
(5.44 KB, patch)
2021-06-21 07:12 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
WIP1.1
(6.10 KB, patch)
2021-06-21 12:26 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch 1.0
(12.50 KB, patch)
2021-06-23 10:06 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch 1.1
(13.76 KB, patch)
2021-06-24 06:53 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch 1.2
(13.24 KB, patch)
2021-06-24 07:18 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch 1.3
(14.21 KB, patch)
2021-07-01 08:18 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch 1.4
(14.32 KB, patch)
2021-07-02 06:32 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-06-16 15:08:10 PDT
<
rdar://problem/79418247
>
Razvan Caliman
Comment 2
2021-06-21 07:12:46 PDT
Created
attachment 431858
[details]
WIP Request for feedback on general direction
Razvan Caliman
Comment 3
2021-06-21 07:37:38 PDT
This WIP patch is building on the patch from
bug 227097
which hasn't landed yet, therefore it's now only a request for feedback. There is prior work for augmenting function completions in `Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorCompletionController.js:588`: ``` this._delegate.completionControllerCSSFunctionValuesNeeded(this, functionName, functionCompletions) ``` However, the delegate doesn't implement `.completionControllerCSSFunctionValuesNeeded()`. The delegate changed over time. It used to be `WI.CSSStyleDeclarationTextEditor`, bug got removed with the legacy style editor in
bug 189808
. But the idea seems good. --- This patch implements `WI.SpreadsheetCSSStyleDeclarationEditor.completionControllerCSSFunctionValuesNeeded()`, delegated for `WI.SpreadsheetStyleProperty`. Depending on the function name: - for "var", return a list of CSS variable names collected in WI.DOMNodeStyles.allCSSVariables - for "attr", return a list of node attribute names (restoring functionality lost when removing `WI.CSSStyleDeclarationTextEditor` in
bug 189808
) Should we delegate a few more levels up to `WI.SpreadsheetRulesStyleDetailsPanel`? Or is it acceptable to stop at `WI.SpreadsheetCSSStyleDeclarationEditor`? --- This does yet handle CSS variable completions in the Sources tab (i.e. implementing `WI.SourceCodeTextEditor.completionControllerCSSFunctionValuesNeeded()`). For that, we need to collect *all CSS Variables* declared in all stylesheets since the Sources code editor doesn't have a concept of a selected node nor does that make sense. Probably left to don in a follow-up.
Razvan Caliman
Comment 4
2021-06-21 07:40:40 PDT
Comment on
attachment 431858
[details]
WIP View in context:
https://bugs.webkit.org/attachment.cgi?id=431858&action=review
> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264 > + set values(values = [])
Doing this to ensure values can be added on-demand and the resulting list remains sorted.
> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:107 > + if (functionName === "var" || functionName === "attr")
`attr(var(--something))` is not supported.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:456 > + values = this.style.node.attributes().map(attribute => attribute.name);
Restoring functionality lost with
bug 189808
. Should this be in a separate bug/patch?
Devin Rousso
Comment 5
2021-06-21 09:53:24 PDT
Comment on
attachment 431858
[details]
WIP View in context:
https://bugs.webkit.org/attachment.cgi?id=431858&action=review
>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264 >> + set values(values = []) > > Doing this to ensure values can be added on-demand and the resulting list remains sorted.
This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly. Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction. Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`. I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern).
>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:107 >> + if (functionName === "var" || functionName === "attr") > > `attr(var(--something))` is not supported.
I wonder if we want to autocomplete `attr` on any property other than `content` given that it only works there 🤔
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:952 > + this._allCSSVariables = new Set;
This isn't really great IMO as a function named `_collectUsedCSSVariables` has a side effect of modifying `_allCSSVariables`. I'd recommend renaming this function to something more generic (e.g. `_collectCSSVariables`) so that this is more "acceptable".
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:453 > + values = this.style.nodeStyles.allCSSVariables;
Style: change this into a `return`
>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:456 >> + values = this.style.node.attributes().map(attribute => attribute.name); > > Restoring functionality lost with
bug 189808
. Should this be in a separate bug/patch?
Yeah this should probably be a different bug (or you could rename this bug to also include `attr()`. Style: argument of arrow functions should always have parenthesis
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:988 > + return { prefix: currentTokenValue, completions: completions.startsWith(currentTokenValue) };
Style: no space after `{` or before `}`
Razvan Caliman
Comment 6
2021-06-21 10:37:35 PDT
Comment on
attachment 431858
[details]
WIP View in context:
https://bugs.webkit.org/attachment.cgi?id=431858&action=review
>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264 >>> + set values(values = []) >> >> Doing this to ensure values can be added on-demand and the resulting list remains sorted. > > This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly. Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction. > > Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`. I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern).
> or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction.
Funny, I had a version of the patch doing this and thought it would get flagged during review :) Do you mean something like this? ``` -WI.CSSKeywordCompletions.forFunction = function(functionName) +WI.CSSKeywordCompletions.forFunction = function(functionName, supplementalCompletions = {}) { let suggestions = ["var()"]; if (functionName === "var") - suggestions = []; + suggestions = supplementalCompletions?.var || []; ```
>>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:107 >>> + if (functionName === "var" || functionName === "attr") >> >> `attr(var(--something))` is not supported. > > I wonder if we want to autocomplete `attr` on any property other than `content` given that it only works there 🤔
Improbable, but one could put it in a var and that still works. ``` --myvar: attr(class); content: var(--myvar); ```
>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:952 >> + this._allCSSVariables = new Set; > > This isn't really great IMO as a function named `_collectUsedCSSVariables` has a side effect of modifying `_allCSSVariables`. I'd recommend renaming this function to something more generic (e.g. `_collectCSSVariables`) so that this is more "acceptable".
Yes, the naming is off indeed. I'll change. I wanted to avoid doing another pass to grab all variables so I tried to leverage this loop here. Still scarred by the 1500 CSS variable scenarios of sites that dump all custom properties to the :root.
Devin Rousso
Comment 7
2021-06-21 10:42:20 PDT
Comment on
attachment 431858
[details]
WIP View in context:
https://bugs.webkit.org/attachment.cgi?id=431858&action=review
>>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264 >>>> + set values(values = []) >>> >>> Doing this to ensure values can be added on-demand and the resulting list remains sorted. >> >> This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly. Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction. >> >> Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`. I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern). > >
I think I'd either rename this to `addAll(values)` and keep mostly the same behavior (this is nice for extensibility in other scenarios) or modify `WI.CSSKeywordCompletions.forFunction` to also accept an optional `WI.CSSStyleDeclaration` which would instead take the place of the `completionControllerCSSFunctionValuesNeeded` delegate (this is also nice because then _every_ caller of `WI.CSSKeywordCompletions.forFunction` gets this behavior (assuming they provide a `WI.CSSStyleDeclaration`), instead of only the ones that have a delegate).
Razvan Caliman
Comment 8
2021-06-21 10:55:44 PDT
Comment on
attachment 431858
[details]
WIP View in context:
https://bugs.webkit.org/attachment.cgi?id=431858&action=review
>>>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:264 >>>>> + set values(values = []) >>>> >>>> Doing this to ensure values can be added on-demand and the resulting list remains sorted. >>> >>> This is a bit odd as `WI.CSSCompletions` is really meant to be basically readonly. Instead of allowing the `WI.CSSCompletion` to be entirely modifiable, I wonder if we could just have an `addAll` (i.e. can only extend, not replace) or really more ideally have a way for `WI.CSSKeywordCompletions.forFunction` to do this logic before construction. >>> >>> Also, having a default parameter here isn't ideal as that's only going to be used if `undefined` is provided, and we generally avoid using `undefined` instead of `null`. I think you're better off having a `values ||= [];` inside the function instead (FWIW this is our normal pattern). >> >> > > I think I'd either rename this to `addAll(values)` and keep mostly the same behavior (this is nice for extensibility in other scenarios) or modify `WI.CSSKeywordCompletions.forFunction` to also accept an optional `WI.CSSStyleDeclaration` which would instead take the place of the `completionControllerCSSFunctionValuesNeeded` delegate (this is also nice because then _every_ caller of `WI.CSSKeywordCompletions.forFunction` gets this behavior (assuming they provide a `WI.CSSStyleDeclaration`), instead of only the ones that have a delegate).
I'd avoid passing in `WI.CSSStyleDeclaration` to `WI.CSSKeywordCompletions.forFunction()` because the Sources panel won't have one to pass in for context when getting all CSS variables. Its mechanism for getting CSS variables will be different because it's not constrained to the selected node. Augmenting `WI.CSSKeywordCompletions.forFunction()` to get an optional list of extra items to pass to `WI.CSSCompletion` constructor will make it possible for Styles and Sources to pass in their own contextual data. I'm thinking of something like: ``` WI.CSSKeywordCompletions.forFunction = function(functionName, extraFunctionCompletions ={}) { let suggestions = ["var()"]; if (functionName === "var") suggestions = extraFunctionCompletions?.var || []; ... return new WI.CSSCompletions(suggestions, true); } ```
Razvan Caliman
Comment 9
2021-06-21 12:26:54 PDT
Created
attachment 431894
[details]
WIP1.1 Address feedback
Nikita Vasilyev
Comment 10
2021-06-21 13:43:44 PDT
Comment on
attachment 431894
[details]
WIP1.1 View in context:
https://bugs.webkit.org/attachment.cgi?id=431894&action=review
Patch doesn't apply; looks like it needs to be rebaselined.
> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:267 > + this._values = this._values.concat(values.slice());
Nit: No need to call `slice`; concat doesn't modify passed array.
Razvan Caliman
Comment 11
2021-06-23 10:06:05 PDT
Created
attachment 432065
[details]
Patch 1.0 Patch based on top of the one from
bug 227097
Patrick Angle
Comment 12
2021-06-23 11:15:42 PDT
Comment on
attachment 432065
[details]
Patch 1.0 View in context:
https://bugs.webkit.org/attachment.cgi?id=432065&action=review
Awesome work, mostly just some thoughts on naming throughout.
> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:51 > +WI.CSSKeywordCompletions.forPartialPropertyValue = function(text, propertyName, {caretPosition, getFunctionValueCompletions} = {})
Nit: We might want to tweak this variable name, as currently it makes it sound like it is the only provider of function value completions, instead of an additional source of those completions. `additionalFunctionValueCompletionsProvider`?
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:50 > + this._allCSSVariables = new Set;
Nit: If I understand correctly, this isn't really `all` CSS variables, but the `available` (not convinced this is precisely the right prefix either) CSS variables.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:342 > + getFunctionValueCompletions(functionName)
See CSSKeywordCompletions:51
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:348 > + if (functionName === "var") > + return Array.from(this._property.ownerStyle.nodeStyles.allCSSVariables); > + > + if (functionName === "attr") > + return this._property.ownerStyle.node.attributes().map((attribute) => attribute.name);
Any reason not to express this as a switch statement? I know with two cases that's a bit of a wash, but I could see us needing to add to this someday. I'm fine either way, just something to consider.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:926 > + return WI.CSSKeywordCompletions.forPartialPropertyValue(text, this._nameElement.textContent.trim(), {caretPosition, getFunctionValueCompletions: this.getFunctionValueCompletions.bind(this)});
See CSSKeywordCompletions:51
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:70 > + const getFunctionValueCompletions = (functionName) => {
Can we declare this constant outside of compare expectations? Or, perhaps even better, could we make the additional `var` and `attr` completions optional parameters to `compareExpectations()`? In my opinion `compareExpectations` should do as little as possible to influence the outcome of what we are testing, and where it does it should do so with information from it's callers instead of inferring a non-empty set of these additional completions. Also, I understand why you used this exact name (note from CSSKeywordCompletions:51 not withstanding), but would it be clearer that this is being somewhat fakes for these tests to have `mock` in the name somewhere?
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:146 > + // attr(|
Nit: Can you put the value and caret inside backticks like the examples above, please. Ditto for below comments as well.
Razvan Caliman
Comment 13
2021-06-24 06:53:00 PDT
Created
attachment 432157
[details]
Patch 1.1 Address feedback; Patch based on top of the one from
bug 227097
Razvan Caliman
Comment 14
2021-06-24 07:07:54 PDT
Comment on
attachment 432065
[details]
Patch 1.0 View in context:
https://bugs.webkit.org/attachment.cgi?id=432065&action=review
>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:51 >> +WI.CSSKeywordCompletions.forPartialPropertyValue = function(text, propertyName, {caretPosition, getFunctionValueCompletions} = {}) > > Nit: We might want to tweak this variable name, as currently it makes it sound like it is the only provider of function value completions, instead of an additional source of those completions. `additionalFunctionValueCompletionsProvider`?
Yes, that's a good idea. Updated.
>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:50 >> + this._allCSSVariables = new Set; > > Nit: If I understand correctly, this isn't really `all` CSS variables, but the `available` (not convinced this is precisely the right prefix either) CSS variables.
The scope here is the styles for the selected DOM node. In that sense, these are all CSS variables applicable to it. I'm not sure if the `available` prefix would clarify it any more. The case for using `_all` in the context of this file is to distinguish that `_usedCSSVariables` will be a subset of `_allCSSVariables`. Still open to a better naming suggestion. I considered dropping `_all` in favor of just `_CSSVariables` but that just adds to the ambiguity.
>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:348 >> + return this._property.ownerStyle.node.attributes().map((attribute) => attribute.name); > > Any reason not to express this as a switch statement? I know with two cases that's a bit of a wash, but I could see us needing to add to this someday. I'm fine either way, just something to consider.
I tried a `switch` statement before, but it looked needlessly complex for the two cases currently handled. When more cases are added, perhaps it will then make more sense to switch to `switch` :)
>> LayoutTests/inspector/unit-tests/css-keyword-completions.html:70 >> + const getFunctionValueCompletions = (functionName) => { > > Can we declare this constant outside of compare expectations? Or, perhaps even better, could we make the additional `var` and `attr` completions optional parameters to `compareExpectations()`? In my opinion `compareExpectations` should do as little as possible to influence the outcome of what we are testing, and where it does it should do so with information from it's callers instead of inferring a non-empty set of these additional completions. > > Also, I understand why you used this exact name (note from CSSKeywordCompletions:51 not withstanding), but would it be clearer that this is being somewhat fakes for these tests to have `mock` in the name somewhere?
Extracted the mock provider and added a mock to its name. Additionally, I configured `compareExpectations()` to accept an argument for a function that provides the contextual function values – the mock. It did add verbosity to the test cases. I'm not convinced it was worthwhile given that this is a unit-test. But I don't have strong opinions either way.
Razvan Caliman
Comment 15
2021-06-24 07:16:44 PDT
> Or, perhaps even better, could we make the additional `var` and `attr` completions optional parameters to `compareExpectations()`?
On closer reading of your comment, I think I understand what you meant. Essentially, the test is checking that the provided `additionalFunctionValueCompletionsProvider()` gets called by `WI.CSSKeywordCompletions.forPartialPropertyValue` and its results are appended to the list of completions and sorted. Therefore, the mock itself doesn't have to contain any logic. The mock can just be a simple function that returns the list of additional completions. The test will check if these were appended, sorted and filtered accordingly. Ex: ``` // `attr(|` compareExpectations("attr(", "content", { expectedPrefix: "", expectedCompletions: ["class", "id", "var()"], additionalFunctionValueCompletionsProvider: () => { return ["class", "id"] }, }) ``` Do let me know if I misread your suggestion.
Razvan Caliman
Comment 16
2021-06-24 07:18:35 PDT
Created
attachment 432158
[details]
Patch 1.2 Update test to add inline mocks instead of global mock; Patch based on top of the one from
bug 227097
Devin Rousso
Comment 17
2021-06-24 11:24:07 PDT
Comment on
attachment 432158
[details]
Patch 1.2 View in context:
https://bugs.webkit.org/attachment.cgi?id=432158&action=review
> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:266 > + console.assert(Array.isArray(values), "Expected array of values for additional completion suggestions");
NIT: These kinds of comments are not useful IMO since this is basically redundant if one reads the code. For simple assertions like this the code itself is enough. NIT: For non-falsy checks please also include the argument in the assertion so that if a developer opens inspector^2 they can see what value was passed in ``` console.assert(Array.isArray(values), values); ```
> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:267 > + this._values = this._values.concat(values);
Please use the utility added in
r249301
:) ``` this._values.pushAll(values); ```
> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:122 > +
Style: unnecessary newline
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:344 > + if (functionName === "var")
Personally I prefer using `switch` even for a small number of `case` as it makes future patches cleaner and keeps the blame very straightforward. Up to you tho :)
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:68 > + additionalFunctionValueCompletionsProvider ??= () => {}
Style: missing semicolon
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:140 > + additionalFunctionValueCompletionsProvider: () => { return ["class", "id"] },
You can drop the `{ return` and `}` for these. You only have to have that for multi-line arrow functions (in accordance with our style) or when the returned value is an object literal.
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:148 > + additionalFunctionValueCompletionsProvider: () => { return ["class", "id"] },
ditto (:140)
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:155 > + additionalFunctionValueCompletionsProvider: () => { return ["--one", "--two"] },
ditto (:140)
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:162 > + additionalFunctionValueCompletionsProvider: () => { return ["--one", "--two"] },
ditto (:140)
> LayoutTests/inspector/unit-tests/css-keyword-completions.html:170 > + additionalFunctionValueCompletionsProvider: () => { return ["--one", "--two"] },
ditto (:140)
Razvan Caliman
Comment 18
2021-07-01 08:15:33 PDT
Comment on
attachment 432158
[details]
Patch 1.2 View in context:
https://bugs.webkit.org/attachment.cgi?id=432158&action=review
>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:267 >> + this._values = this._values.concat(values); > > Please use the utility added in
r249301
:) > ``` > this._values.pushAll(values); > ```
Neat!
>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:344 >> + if (functionName === "var") > > Personally I prefer using `switch` even for a small number of `case` as it makes future patches cleaner and keeps the blame very straightforward. Up to you tho :)
Yeah, the argument about a cleaner `blame` is a good one.
Razvan Caliman
Comment 19
2021-07-01 08:18:25 PDT
Created
attachment 432696
[details]
Patch 1.3 Rebased onto master after
Bug 227097
landed. Addressed code review feedback.
Devin Rousso
Comment 20
2021-07-01 11:23:14 PDT
Comment on
attachment 432696
[details]
Patch 1.3 View in context:
https://bugs.webkit.org/attachment.cgi?id=432696&action=review
r=me, nice work :)
> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:114 > + completions.addValues(contextualValueCompletions);
We should either not call this if `contextualValueCompletions` is empty or have a check inside `addValues` that bails if the provided array is empty so that we don't `sort()` unnecessarily.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:347 > + return Array.from(this._property.ownerStyle.nodeStyles.allCSSVariables); > + case "attr":
Style: missing newline between
Razvan Caliman
Comment 21
2021-07-02 06:27:40 PDT
Comment on
attachment 432696
[details]
Patch 1.3 View in context:
https://bugs.webkit.org/attachment.cgi?id=432696&action=review
>> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:114 >> + completions.addValues(contextualValueCompletions); > > We should either not call this if `contextualValueCompletions` is empty or have a check inside `addValues` that bails if the provided array is empty so that we don't `sort()` unnecessarily.
Good idea! I'll add the check to `WI.Completions.addValues()`
Razvan Caliman
Comment 22
2021-07-02 06:32:02 PDT
Created
attachment 432784
[details]
Patch 1.4 Carry over R+; Address last nits
EWS
Comment 23
2021-07-02 07:13:27 PDT
Committed
r279502
(
239354@main
): <
https://commits.webkit.org/239354@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 432784
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug