DevTools: support live coverage

DevTools: support live coverage

- support polling in CoverageModel and CoverageView
- support incremental updates in CoverageListView
- revise the way decorators are shown, make a dummy decorator and then iterate over all lines in UISourceCode, map them to original file locations and check coverage
- remove remains of mixed coverage decorators
- assume whitespace is always unused

BUG=719113
Review-Url: https://codereview.chromium.org/2865573003
Cr-Commit-Position: refs/heads/master@{#469856}
Committed: https://chromium.googlesource.com/chromium/src/+/4c7e58d58127b87916dfbaf423e63bf6276d80e8

[caseq, 2017-05-04 23:42:38]

Patchset #1 (id:1) has been deleted

[caseq, 2017-05-04 23:50:28]

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

[alph, 2017-05-05 19:18:09]

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js
(right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:57:
var startLocationsByProvider = new Map(startLocations.map(location =>
[location.contentProvider, location]));
can it have several locations with he same provider?

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:59:
for (var end of endLocations) {
nit: var used = endLocations.some(...);
;-)

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:67:
while (startOffset < endOffset && /\s/.test(textValue[startOffset]))
<=

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:67:
while (startOffset < endOffset && /\s/.test(textValue[startOffset]))
Have you considered using substr and match with /^\s*/ ? I believe it creates a
sliced string with copy-on-write.

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:69:
while (startOffset < endOffset && /\s/.test(textValue[endOffset]))
ditto

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:83:
* @return !Promise
I'm ok with using the clean syntax, but I know couple people that can be against
it. Please make sure there's a consensus.

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:89:
if (!this._textByProvider.has(entry.contentProvider)) {
if (...) continue;

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:100:
* @param {!Common.ContentProvider} contentProvider
@return

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js
(right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js:193:
/** @type {number} */
|undefined

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js:201: *
param {number} maxSize
@param

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js:202:
*/
@return

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js
(right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:287: *
@param {number} columnOffset
@return

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:362: var
index = this._segments.upperBound(start, (time, segment) => time - segment.end);
time?

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:367:
return index < this._segments.length && this._segments[index].count;
can't this segment be beyond the end boundary?

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:313: return
this._agent.stopRuleUsageTracking((error, ruleUsage) => {});
drop the callback

[dgozman, 2017-05-05 19:21:20]

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js
(right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:1:
// Copyright (c) 2017 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:16:
for (var project of Workspace.workspace.projects()) {
Use Workspace.uiSourceCodes().

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:25:
for (var uiSourceCode of project.uiSourceCodes())
ditto

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:116:
return location && location.script() ?
html file could have both css and js locations.

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js
(right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:64:
return [].concat(...updates);
What is this?

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/coverage/CoverageView.js
(right):

https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageView.js:114:
this._pollTimer = setTimeout(() => this._poll(), 700);
Would throttler help here?

[caseq, 2017-05-05 21:52:02]

On 2017/05/05 19:18:09, alph wrote:
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js
> (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:57:
> var startLocationsByProvider = new Map(startLocations.map(location =>
> [location.contentProvider, location]));
> can it have several locations with he same provider?

Good point, rewritten to take care of it.
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:59:
> for (var end of endLocations) {
> nit: var used = endLocations.some(...);
> ;-)

No.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:67:
> while (startOffset < endOffset && /\s/.test(textValue[startOffset]))
> <=

Done.

> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:67:
> while (startOffset < endOffset && /\s/.test(textValue[startOffset]))
> Have you considered using substr and match with /^\s*/ ? I believe it creates
a
> sliced string with copy-on-write.

Decided against this based on worse readability. There does not seem to be a
performance problem here.
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:69:
> while (startOffset < endOffset && /\s/.test(textValue[endOffset]))
> ditto

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:83:
> * @return !Promise
> I'm ok with using the clean syntax, but I know couple people that can be
against
> it. Please make sure there's a consensus.

Fixed.

> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:89:
> if (!this._textByProvider.has(entry.contentProvider)) {
> if (...) continue;

Done.

> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:100:
> * @param {!Common.ContentProvider} contentProvider
> @return

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js
> (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js:193:
> /** @type {number} */
> |undefined

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js:201:
*
> param {number} maxSize
> @param

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageListView.js:202:
> */
> @return

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js
> (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:287: *
> @param {number} columnOffset
> @return

Done.

> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:362:
var
> index = this._segments.upperBound(start, (time, segment) => time -
segment.end);
> time?

Fixed.

> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:367:
> return index < this._segments.length && this._segments[index].count;
> can't this segment be beyond the end boundary?

It will -- and this means the end boundary belongs to this segment :-)

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:313: return
> this._agent.stopRuleUsageTracking((error, ruleUsage) => {});
> drop the callback

Done.

[caseq, 2017-05-05 21:55:34]

On 2017/05/05 19:21:20, dgozman wrote:
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js
> (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:1:
> // Copyright (c) 2017 The Chromium Authors. All rights reserved.
> no (c)

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:16:
> for (var project of Workspace.workspace.projects()) {
> Use Workspace.uiSourceCodes().

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:25:
> for (var uiSourceCode of project.uiSourceCodes())
> ditto

Done.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:116:
> return location && location.script() ?
> html file could have both css and js locations.

Well, I planned to take care of HTML later, because it doesn't really work for
other reasons, but fixed this part now.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js
> (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:64:
> return [].concat(...updates);
> What is this?

Thought we just decided that using spreads with built-ins that accept varargs is
ok. Rewritten.

>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/coverage/CoverageView.js
> (right):
> 
>
https://codereview.chromium.org/2865573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/coverage/CoverageView.js:114:
> this._pollTimer = setTimeout(() => this._poll(), 700);
> Would throttler help here?

I considered it but I don't see how it makes things simpler here.

[alph, 2017-05-05 22:50:37]

https://codereview.chromium.org/2865573003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js
(right):

https://codereview.chromium.org/2865573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:65:
Coverage.CoverageDecorationManager._compareLocations(start,
endLocations[endIndex]) >= 0)
Shouldn't be ">" instead?
start === endLocations[endIndex] seems to be a legit range.

https://codereview.chromium.org/2865573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:67:
if (endIndex >= endLocations.length || endLocations[endIndex].id !== start.id)
hmm. How come "endLocations[endIndex].id !== start.id)"? Should it be an assert?

https://codereview.chromium.org/2865573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageDecorationManager.js:158:
* @param {!Coverage.RawLocation} b
@return

https://codereview.chromium.org/2865573003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js
(right):

https://codereview.chromium.org/2865573003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/coverage/CoverageModel.js:368:
return index < this._segments.length && this._segments[index].count;
I'll repeat my question: can't this segment be beyond the end boundary? If so,
why do you take it into account?

[alph, 2017-05-05 22:57:04]

lgtm

[caseq, 2017-05-05 23:22:07]

The CQ bit was checked by caseq@chromium.org

[caseq, 2017-05-05 23:22:07]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2865573003/#ps120001
(title: "minor polish on corner cases")

[commit-bot: I haz the power, 2017-05-05 23:23:05]

CQ is trying da patch.

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

[caseq, 2017-05-06 00:23:26]

Description was changed from

==========
DevTools: support live coverage
==========

to

==========
DevTools: support live coverage

BUG=719113
==========

[caseq, 2017-05-06 00:26:05]

Description was changed from

==========
DevTools: support live coverage

BUG=719113
==========

to

==========
DevTools: support live coverage

- support polling in CoverageModel and CoverageView
- support incremental updates in CoverageListView
- revise the way decorators are shown, make a dummy decorator and then iterate
over all lines in UISourceCode, map them to original file locations and check
coverage
- remove remains of mixed coverage decorators
- assume whitespace is always unused

BUG=719113
==========

[commit-bot: I haz the power, 2017-05-06 18:57:57]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494026527247070,
"parent_rev": "2eddc334c95ba670f38a4a656f25ad85a7a8f565", "commit_rev":
"4c7e58d58127b87916dfbaf423e63bf6276d80e8"}

[commit-bot: I haz the power, 2017-05-06 18:58:07]

Description was changed from

==========
DevTools: support live coverage

- support polling in CoverageModel and CoverageView
- support incremental updates in CoverageListView
- revise the way decorators are shown, make a dummy decorator and then iterate
over all lines in UISourceCode, map them to original file locations and check
coverage
- remove remains of mixed coverage decorators
- assume whitespace is always unused

BUG=719113
==========

to

==========
DevTools: support live coverage

- support polling in CoverageModel and CoverageView
- support incremental updates in CoverageListView
- revise the way decorators are shown, make a dummy decorator and then iterate
over all lines in UISourceCode, map them to original file locations and check
coverage
- remove remains of mixed coverage decorators
- assume whitespace is always unused

BUG=719113

Review-Url: https://codereview.chromium.org/2865573003
Cr-Commit-Position: refs/heads/master@{#469856}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c7e58d58127b87916dfbaf423e6...
==========

[commit-bot: I haz the power, 2017-05-06 18:58:09]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/4c7e58d58127b87916dfbaf423e6...