DevTools: introduce "Track CSS Usage" experiment

DevTools: introduce "Track CSS Usage" experiment

This patch introduces the "Track CSS Usage" experiment.

The experiment adds a new timeline setting "Track CSS".
When enabled, the timeline recording tracks the usage
of CSS rules and exposes this information through the
Sources panel.

BUG=663017
Committed: https://crrev.com/3e072f5d6d65ae7d9ee65b5937775eb158871a61
Cr-Commit-Position: refs/heads/master@{#431176}

[valih, 2016-11-04 20:30:06]

valih@google.com changed reviewers:
+ caseq@chromium.org

[caseq, 2016-11-04 21:10:02]

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/LineMarkingProfile.js
(right):

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components_lazy/LineMarkingProfile.js:7:
WebInspector.LineMarkingProfile = class {
CoverageProfile?

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

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:964:
'timelineMarkUnusedCSS',
WAT? Why has this changed?

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/Main.js:129:
Runtime.experiments.register('timelineRuleUsageRecording', 'Timeline tracing the
CSS rule usage');
Track CSS rules usage while recording Timeline.

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/main/Main.js:1070: 
revert this line.

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css
(right):

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css:17:
.CodeMirror-gutter-cssTracking {
I don't think that CodeMirror should be concerned with CSS rule usage tracking.

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js
(right):

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:587: if
(Runtime.experiments.isEnabled('timelineRuleUsageRecording'))
Also check the setting here.

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:789: for
(var i = 0; i < ruleUsageList.length; ++i) {
for (var rule of ruleUsageList)

https://codereview.chromium.org/2472213005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:790: if
(!ruleUsageList[i].wasUsed) {
if (!rule.wasUsed)
  continue;

[valih, 2016-11-07 19:05:53]

Description was changed from

==========
Unused CSS highlight in sources panel

BUG=
==========

to

==========
Unused CSS highlight in sources panel

BUG=663017
==========

[caseq, 2016-11-08 01:54:10]

caseq@chromium.org changed reviewers:
+ alph@chromium.org, lushnikov@chromium.org

[caseq, 2016-11-08 01:54:11]

+ lushnikov and alph on the gutter implementation

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js
(right):

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:782: if
(Runtime.experiments.isEnabled('timelineRuleUsageRecording') &&
this._markUnusedCSS.get()) {
Let's extract this into a separate function at least -- and it should be called
when we finish live recording rather than upon loadingComplete. Consider
TimelineController?

[alph, 2016-11-08 17:10:25]

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js
(right):

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:4:
/**
empty line plz

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:5:
* @unrestricted
Can you plz remove this.

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:36:

nit: here and below no need for empty lines

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:58:
_uiSourceCodeChanged(event) {
annotate

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:84:
for (var lineInfo of fileInfo[1]) {
... var line of ...

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:99:
* @unrestricted
drop it

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:127:
* @unrestricted
drop the line

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:146:
var element = createElementWithClass('div',
'text-editor-line-marker-cssTracking');
can we have a file type neutral name. We are going to reuse it for JS coverage.
E.g. s/cssTracking/coverage/

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:147:
element.style.backgroundColor = 'red';
As long as the color is static, you can encode it in css.

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css
(right):

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/text_editor/cmdevtools.css:283:
.CodeMirror .text-editor-line-marker-cssTracking::after{
nit: space before {

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:22:
if (Runtime.experiments.isEnabled('timelineRuleUsageRecording')) {
no need for {}

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:82:
if (!Runtime.experiments.isEnabled('timelineRuleUsageRecording') ||
!this._markUnusedCSS.get())
if an experiment is enabled after opening timeline, this._markUnusedCSS will be
undefined.

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:171:
if (!Runtime.experiments.isEnabled('timelineRuleUsageRecording') ||
!this._markUnusedCSS.get())
This looks bad. We want to suspend targets not involved in timeline recording.

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js
(right):

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:373:
WebInspector.UIString('Audits'), this._markUnusedCSS,
Audits is too generic. Maybe "CSS coverage"?

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:587: if
(Runtime.experiments.isEnabled('timelineRuleUsageRecording') &&
this._markUnusedCSS.get())
ditto for ths._markUnusedCSS being undefined. Just create the setting
unconditionally.

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:645: if
(Runtime.experiments.isEnabled('timelineRuleUsageRecording') &&
this._markUnusedCSS.get())
ditto

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:788:
function ruleListReceived(ruleUsageList) {
Annotate

https://codereview.chromium.org/2472213005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:789: if
(!ruleUsageList)
why can it be null?

[lushnikov, 2016-11-09 00:45:31]

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js
(right):

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:41:
this._files = new Map();
this is not a collection of "files". Let's come up with a better name? E.g.
"coverageForURL"

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:66:
uiSourceCode =>
uiSourceCode.removeAllLineDecorations(WebInspector.CoverageProfile.LineDecorator.type));
can we be smarter and avoid traversing the whole workspace?

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:70:
WebInspector.UISourceCode.Events.WorkingCopyChanged, this._uiSourceCodeChanged,
this));
The workignCopyChanged happens for every keypress in the editor. You don't want
to do anything
expensive here, e.g. traversing the whole workspace (it might contain thousands
of UISourceCodes...).

Why do you want to do this at all?

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:72:
for (var fileInfo of this._files) {
let's iterate keys here - this way you'll avoid unnecessary typecasts

for (var url of this._coverageForURL.keysArray()) {
   ...
}

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:79:
WebInspector.NetworkProject.targetForUISourceCode(uiSourceCode) ||
WebInspector.targetManager.mainTarget();
why do you need mainTarget?

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:80:
var debuggerModel = target ? WebInspector.DebuggerModel.fromTarget(target) :
null;
since you're dealing with CSS, you should not talk to DebuggerModel. @see
CSSModel

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:87:
var rawLocation = debuggerModel.createRawLocationByURL(url, line, 0);
this whole code will not work; let's add a test which would catch this

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:98:
WebInspector.CoverageProfile.Presentation = class {
probably you don't need this - unless it comes handy with SourceMaps

https://codereview.chromium.org/2472213005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:104:
WebInspector.debuggerWorkspaceBinding.createLiveLocation(rawLocation,
this.updateLocation.bind(this), locationPool);
Debugger has nothing to do with styles; use cssWorkspaceBinding if you need to.

[lushnikov, 2016-11-09 01:55:22]

We also need a test!

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js
(right):

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:29:
var fileInfo = this._coverageForURL.get(url);
fileInfo is a poor name as well

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:40:
/** @type {!Map<string, !Set<!WebInspector.TextRange>>} */
use Multimap

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:52:
}, 0);
let's have non-zero timeout, e.g. 100

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:56:
for (var url of this._coverageForURL.keysArray()) {
this appproach not gonna work in case of reset() method

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:62:
for (var fileInfo of this._coverageForURL) {
let's iterate over keys here to avoid typecasts

https://codereview.chromium.org/2472213005/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:69:
for (var line  = range.startLine; line <= range.endLine; ++line) {
multimple style issues here:
1. extra spaces before "="
2. indent should be 2 spaces
3. drop {} around single-line blocks

[caseq, 2016-11-09 18:25:30]

Mostly looks good. Let's revert stray changes and add a test.

It would also be nice to eventually refer to stylesheets by the stylesheet ids
rather than by URLs.

https://codereview.chromium.org/2472213005/diff/210001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js
(right):

https://codereview.chromium.org/2472213005/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js:106:
};
please revert this file.

https://codereview.chromium.org/2472213005/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js
(right):

https://codereview.chromium.org/2472213005/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js:120:
function ruleListReceived(ruleUsageList) {
please annotate ruleUsageList

https://codereview.chromium.org/2472213005/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js
(right):

https://codereview.chromium.org/2472213005/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:930: };
revert this one as well.

[caseq, 2016-11-09 23:33:55]

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source-expected.txt
(right):

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source-expected.txt:15:
1
This looks strange. Aren't those supposed to be line numbers?

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source.html
(right):

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source.html:21:
function printResults() {
{ => next line (this still uses old style)

Also, we prefer the declaration order that would match execution order, so

InspectorTest.startTimeline(timelineStarted)

function timelineStarted()
{
}
...
function printResults()
{
}

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source.html:23:
InspectorTest.showScriptSource("highlight-in-source.css", () =>
setTimeout(didShowScriptSource, 0)));
Passing the result of showScriptSource() to Promise.then() does not make sense.
Also, why do you need setTimeout()?

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source.html:25:
function didShowScriptSource(sourceFrame) {
{ => next line

https://codereview.chromium.org/2472213005/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/tracing/highlight-in-source.html:29:
InspectorTest.addResult("Number of lines: " + lines.length);
drop this, and just do InspectorTest.addResults(lines)

[caseq, 2016-11-10 00:58:01]

lgtm, but please also get one from lushnikov!

[lushnikov, 2016-11-10 01:32:21]

lgtm

[lushnikov, 2016-11-10 01:41:52]

Description was changed from

==========
Unused CSS highlight in sources panel

BUG=663017
==========

to

==========
DevTools: introduce "Track CSS Usage" experiment 

This patch introduces the "Track CSS Usage" experiment.

The experiment adds a new timeline setting "Track CSS".
When enabled, the timeline recording tracks the usage
of CSS rules and exposes this information through the
Sources panel.

BUG=663017
==========

[lushnikov, 2016-11-10 01:42:23]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2016-11-10 01:42:54]

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, 2016-11-10 03:59:42]

Description was changed from

==========
DevTools: introduce "Track CSS Usage" experiment 

This patch introduces the "Track CSS Usage" experiment.

The experiment adds a new timeline setting "Track CSS".
When enabled, the timeline recording tracks the usage
of CSS rules and exposes this information through the
Sources panel.

BUG=663017
==========

to

==========
DevTools: introduce "Track CSS Usage" experiment 

This patch introduces the "Track CSS Usage" experiment.

The experiment adds a new timeline setting "Track CSS".
When enabled, the timeline recording tracks the usage
of CSS rules and exposes this information through the
Sources panel.

BUG=663017
==========

[commit-bot: I haz the power, 2016-11-10 03:59:43]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-11-10 04:12:02]

Description was changed from

==========
DevTools: introduce "Track CSS Usage" experiment 

This patch introduces the "Track CSS Usage" experiment.

The experiment adds a new timeline setting "Track CSS".
When enabled, the timeline recording tracks the usage
of CSS rules and exposes this information through the
Sources panel.

BUG=663017
==========

to

==========
DevTools: introduce "Track CSS Usage" experiment

This patch introduces the "Track CSS Usage" experiment.

The experiment adds a new timeline setting "Track CSS".
When enabled, the timeline recording tracks the usage
of CSS rules and exposes this information through the
Sources panel.

BUG=663017

Committed: https://crrev.com/3e072f5d6d65ae7d9ee65b5937775eb158871a61
Cr-Commit-Position: refs/heads/master@{#431176}
==========

[commit-bot: I haz the power, 2016-11-10 04:12:04]

Patchset 17 (id:??) landed as
https://crrev.com/3e072f5d6d65ae7d9ee65b5937775eb158871a61
Cr-Commit-Position: refs/heads/master@{#431176}

[valih, 2016-12-17 03:54:30]

Patchset #19 (id:330001) has been deleted

[valih, 2016-12-17 03:54:37]

Patchset #18 (id:310002) has been deleted