DevTools: Autocomplete CSS variables in StylesSidebar

DevTools: Autocomplete CSS variables in StylesSidebar

BUG=680693
Review-Url: https://codereview.chromium.org/2637593002
Cr-Commit-Position: refs/heads/master@{#448543}
Committed: https://chromium.googlesource.com/chromium/src/+/1fb1dc19092fca70ebee18705dab8463eed8c6dc

[einbinder, 2017-01-14 03:23:35]

einbinder@chromium.org changed reviewers:
+ dgozman@chromium.org

[einbinder, 2017-01-14 03:23:36]

ptal

[einbinder, 2017-01-14 03:23:59]

Description was changed from

==========
DevTools: Autocomplete CSS variables in StylesSidebar

BUG=none
==========

to

==========
DevTools: Autocomplete CSS variables in StylesSidebar

BUG=680693
==========

[einbinder, 2017-01-14 03:25:27]

The CQ bit was checked by einbinder@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 03:25:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-14 05:09:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-14 05:09:43]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-18 16:29:38]

https://codereview.chromium.org/2637593002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/2637593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3131:
var regexResult = text.match(/[^A-Za-z0-9\-]--[A-Za-z0-9\-]+/g) || [];
I see we now show defined variables in SSP unless it's in a rule which does not
inherit anything except for variables. Should we instead show those rules as
well? And also not dim the variable to indicate it's actually inherited.

Then you don't have to parse anything - just show suggestions from the variables
currently displayed in SSP, since those are exactly the ones which are available
for specific element. WDYT?

[einbinder, 2017-01-18 18:48:31]

On 2017/01/18 at 16:29:38, dgozman wrote:
>
https://codereview.chromium.org/2637593002/diff/1/third_party/WebKit/Source/d...
> File
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):
> 
>
https://codereview.chromium.org/2637593002/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3131:
var regexResult = text.match(/[^A-Za-z0-9\-]--[A-Za-z0-9\-]+/g) || [];
> I see we now show defined variables in SSP unless it's in a rule which does
not inherit anything except for variables. Should we instead show those rules as
well? And also not dim the variable to indicate it's actually inherited.
> 
> Then you don't have to parse anything - just show suggestions from the
variables currently displayed in SSP, since those are exactly the ones which are
available for specific element. WDYT?

We don't show variables in the cascade unless they are defined in that element.

[lushnikov, 2017-01-23 05:06:46]

lushnikov@chromium.org changed reviewers:
+ lushnikov@chromium.org

[lushnikov, 2017-01-23 05:06:47]

https://codereview.chromium.org/2637593002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/2637593002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3114:
Elements.StylesSidebarPane.findCSSVariables = function() {
fetching & parsing everything is a lot of work. can we rather pull the css
variables from the backend? It should be
available for us almost for free

[einbinder, 2017-01-23 22:22:16]

Now that CSS variables show in the cascade, I use the matched properties as
dgozman suggested.

[dgozman, 2017-01-23 23:37:41]

Andrey, do we have tests which exercise completions for full-stack, with getting
real data from backend? We could really use that for this patch.

[lushnikov, 2017-01-24 01:35:04]

Dmitry: I'm not aware of such a test - completions are defined in CSSMetadata,
so no one bothered doing full-stack test.

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2356:
var cssVariables = [];
nit: this makes sense as a method on MatchedStyles

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2911:
var editingName = this._isEditingName;
nit: i understand this is to avoid binding of filterCompletions, but it is a bit
confusing. Let's just use this._isEditingName

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2912:
var editingVariable = expression.trim().endsWith('var(');
nit: let's move this down to where it is actually used? Also, should it be

var editingVariable = !this._isEditingName &&
expression.trim().endsWith('var(');

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2918:
if (!editingVariable)
should it be:

if (editingVariable)
    this._cssVariables.forEach(filterCompletions);
else
    this._cssCompletions.forEach(filterCompletions);

[einbinder, 2017-02-03 01:51:54]

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js
(right):

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2356:
var cssVariables = [];
On 2017/01/24 at 01:35:03, lushnikov wrote:
> nit: this makes sense as a method on MatchedStyles

Done.

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2911:
var editingName = this._isEditingName;
On 2017/01/24 at 01:35:03, lushnikov wrote:
> nit: i understand this is to avoid binding of filterCompletions, but it is a
bit confusing. Let's just use this._isEditingName

Done.

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2912:
var editingVariable = expression.trim().endsWith('var(');
On 2017/01/24 at 01:35:03, lushnikov wrote:
> nit: let's move this down to where it is actually used? Also, should it be
> 
> var editingVariable = !this._isEditingName &&
expression.trim().endsWith('var(');

It is used in the next line.

https://codereview.chromium.org/2637593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2918:
if (!editingVariable)
On 2017/01/24 at 01:35:04, lushnikov wrote:
> should it be:
> 
> if (editingVariable)
>     this._cssVariables.forEach(filterCompletions);
> else
>     this._cssCompletions.forEach(filterCompletions);

When we are editing the name, we want to show both css completions and css
variables.

[einbinder, 2017-02-03 23:48:36]

The test isn't completely full stack, but it gets the variables from a real node
now.

[pfeldman, 2017-02-06 23:01:59]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2017-02-06 23:02:00]

lgtm

[einbinder, 2017-02-06 23:37:04]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2017-02-06 23:38:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 23:41:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 23:42:00]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[einbinder, 2017-02-07 01:22:46]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-02-07 01:22:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2637593002/#ps80001
(title: "merge")

[commit-bot: I haz the power, 2017-02-07 01:23:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 02:01:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 02:01:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[einbinder, 2017-02-07 02:55:22]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2017-02-07 02:55:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 04:34:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486436122205440,
"parent_rev": "f0de79a0e44c878b66df8012e77ff38896fe8faf", "commit_rev":
"1fb1dc19092fca70ebee18705dab8463eed8c6dc"}

[commit-bot: I haz the power, 2017-02-07 04:35:25]

Description was changed from

==========
DevTools: Autocomplete CSS variables in StylesSidebar

BUG=680693
==========

to

==========
DevTools: Autocomplete CSS variables in StylesSidebar

BUG=680693

Review-Url: https://codereview.chromium.org/2637593002
Cr-Commit-Position: refs/heads/master@{#448543}
Committed:
https://chromium.googlesource.com/chromium/src/+/1fb1dc19092fca70ebee18705dab...
==========

[commit-bot: I haz the power, 2017-02-07 04:35:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1fb1dc19092fca70ebee18705dab...