Issue 2492343002: Devtools: Pretty print fix for CSS coverage decorations.

Issue 2492343002: Devtools: Pretty print fix for CSS coverage decorations. (Closed)

Created:
4 years, 1 month ago by valih

Modified:
4 years, 1 month ago

Reviewers:
caseq, lushnikov

CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Devtools: The css coverage decorations are now getting adjusted after the source is formatted by the pretty print. BUG=664650 Committed: https://crrev.com/354cd6c5a2e6b2a8423816eecada2aeeb5a1a41c Cr-Commit-Position: refs/heads/master@{#433271}

Patch Set 1 #

Patch Set 2 : Pretty print fix for CSS coverage decorations. #

Total comments: 4

Patch Set 3 : Pretty print fix for CSS coverage decorations. #

Patch Set 4 : Pretty print fix for CSS coverage decorations. #

Total comments: 1

Patch Set 5 : Pretty print fix for CSS coverage decorations. #

Total comments: 2

Patch Set 6 : Pretty print fix for CSS coverage decorations. #

Total comments: 8

Patch Set 7 : Pretty print fix for CSS coverage decorations. #

Total comments: 4

Patch Set 8 : Pretty print fix for CSS coverage decorations. #

Patch Set 9 : Pretty print fix for CSS coverage decorations. #

Patch Set 10 : Pretty print fix for CSS coverage decorations. #

Patch Set 11 : Pretty print fix for CSS coverage decorations. #

Patch Set 12 : Pretty print fix for CSS coverage decorations. #

Total comments: 9

Patch Set 13 : Pretty print fix for CSS coverage decorations. #

Total comments: 1

Patch Set 14 : Pretty print fix for CSS coverage decorations. #

Total comments: 1

Patch Set 15 : Pretty print fix for CSS coverage decorations. #

Patch Set 16 : Pretty print fix for CSS coverage decorations. #

Patch Set 17 : Pretty print fix for CSS coverage decorations. #

Patch Set 18 : Pretty print fix for CSS coverage decorations. #

Total comments: 9

Patch Set 19 : Pretty print fix for CSS coverage decorations. #

Total comments: 4

Patch Set 20 : Pretty print fix for CSS coverage decorations. #

Total comments: 4

Patch Set 21 : Pretty print fix for CSS coverage decorations. #

Patch Set 22 : Pretty print fix for CSS coverage decorations. #

Total comments: 3

Patch Set 23 : Pretty print fix for CSS coverage decorations. #

Patch Set 24 : Pretty print fix for CSS coverage decorations. #

Total comments: 2

Patch Set 25 : Pretty print fix for CSS coverage decorations. #

Total comments: 1

Patch Set 26 : Pretty print fix for CSS coverage decorations. #

Created: 4 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+183 lines, -55 lines)			Patch
A	third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+77 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+7 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/inspector/tracing/resources/decorations-after-inplace-formatter.css	View	1 2 3 4	1 chunk	+19 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	3 chunks	+12 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	3 chunks	+7 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25	1 chunk	+25 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/timeline/TimelineController.js	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	3 chunks	+35 lines, -36 lines	0 comments	Download

Messages

Total messages: 49 (9 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

caseq

https://codereview.chromium.org/2492343002/diff/20001/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js File third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js (right): https://codereview.chromium.org/2492343002/diff/20001/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js#newcode114 third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js:114: var markers = uiSourceCode._typeDecorations.get('coverage'); We treat names starting with ...

4 years, 1 month ago (2016-11-11 23:23:40 UTC) #5

lushnikov

https://codereview.chromium.org/2492343002/diff/20001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js (right): https://codereview.chromium.org/2492343002/diff/20001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js#newcode33 third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:33: if (range.startColumn) why is this? https://codereview.chromium.org/2492343002/diff/20001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js (right): ...

4 years, 1 month ago (2016-11-11 23:28:01 UTC) #6

valih

https://codereview.chromium.org/2492343002/diff/60001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html File third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html (right): https://codereview.chromium.org/2492343002/diff/60001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html#newcode1 third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html:1: <html> Not finished!

4 years, 1 month ago (2016-11-14 19:40:01 UTC) #7

einbinder

https://codereview.chromium.org/2492343002/diff/80001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter-expected.txt File third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter-expected.txt (right): https://codereview.chromium.org/2492343002/diff/80001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter-expected.txt#newcode11 third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter-expected.txt:11: 5,6,7,9,10,11,12,18,19,20,22,23,24 Can you make this easier to read? https://codereview.chromium.org/2492343002/diff/80001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html ...

4 years, 1 month ago (2016-11-14 22:18:53 UTC) #10

lushnikov

https://codereview.chromium.org/2492343002/diff/90008/third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js (right): https://codereview.chromium.org/2492343002/diff/90008/third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js#newcode957 third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:957: Workspace.FormatterMappingPayload; This doesn't belong here as well https://codereview.chromium.org/2492343002/diff/90008/third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js#newcode959 third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:959: ...

4 years, 1 month ago (2016-11-14 22:38:52 UTC) #12

caseq

https://codereview.chromium.org/2492343002/diff/110001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js (right): https://codereview.chromium.org/2492343002/diff/110001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js#newcode12 third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:12: Workspace.workspace.uiSourceCodes().forEach(uiSourceCode => uiSourceCode.addEventListener( When does this happen? What about ...

4 years, 1 month ago (2016-11-14 23:33:18 UTC) #14

lushnikov

This looks legit! Meanwhile Joel convinced me that your proposal to generalize UISourceCode.addLineDecoration(lineNumber, decoration) to ...

4 years, 1 month ago (2016-11-14 23:34:52 UTC) #15

valih

https://codereview.chromium.org/2492343002/diff/210001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js (right): https://codereview.chromium.org/2492343002/diff/210001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js#newcode32 third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:32: range.startColumn is the first position after the opening brace ...

4 years, 1 month ago (2016-11-15 18:56:24 UTC) #20

valih

https://codereview.chromium.org/2492343002/diff/230001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js (right): https://codereview.chromium.org/2492343002/diff/230001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js#newcode32 third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:32: range.startColumn is the first position after the opening brace ...

4 years, 1 month ago (2016-11-15 19:20:51 UTC) #21

valih

https://codereview.chromium.org/2492343002/diff/250001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js (right): https://codereview.chromium.org/2492343002/diff/250001/third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js#newcode32 third_party/WebKit/Source/devtools/front_end/components_lazy/CoverageProfile.js:32: range.startColumn is the first position after the opening brace ...

4 years, 1 month ago (2016-11-15 19:44:00 UTC) #22

caseq

https://codereview.chromium.org/2492343002/diff/210001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html File third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html (right): https://codereview.chromium.org/2492343002/diff/210001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html#newcode12 third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html:12: {setTimeout(InspectorTest.completeTest, 3000); nuke this https://codereview.chromium.org/2492343002/diff/210001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html#newcode22 third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html:22: InspectorTest.startTimeline(timelineStarted); You should ...

4 years, 1 month ago (2016-11-15 22:07:05 UTC) #23

lushnikov

https://codereview.chromium.org/2492343002/diff/330001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js (left): https://codereview.chromium.org/2492343002/diff/330001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js#oldcode112 third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js:112: if (this._uiLocation) { why this change? https://codereview.chromium.org/2492343002/diff/330001/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js File third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js ...

4 years, 1 month ago (2016-11-16 22:53:55 UTC) #28

valih

https://codereview.chromium.org/2492343002/diff/350001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js (left): https://codereview.chromium.org/2492343002/diff/350001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js#oldcode112 third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js:112: if (this._uiLocation) { it is ok to not remove ...

4 years, 1 month ago (2016-11-17 01:26:37 UTC) #30

lushnikov

4 years, 1 month ago (2016-11-17 17:55:52 UTC) #31

https://codereview.chromium.org/2492343002/diff/350001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js
(left):

https://codereview.chromium.org/2492343002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js:112:
if (this._uiLocation) {
On 2016/11/17 01:26:37, valih wrote:
> it is ok to not remove decorations from the unformatted file and keep them in
> both (this is not an inplace formatting)

Doesn't this mean that whenever the formatted file gets closed, the decorations
for the original file will be double-added?

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

https://codereview.chromium.org/2492343002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js:119:
uiSourceCode.allDecorations().forEach(decoration =>
decorationList.push(decoration));
let's do the following:

_formatDecorations(uiSourceCode, sourceMapping) {
    var decorations = uiSourceCode.allDecorations();
    if (!decorations.length)
        return;
    uiSourceCode.removeAllDecorations();
    for (var decoration of decorations) {
        ....
    }

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

https://codereview.chromium.org/2492343002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:645:
this.dispatchEventToListeners(Workspace.UISourceCode.Events.LineDecorationRemoved,
marker));
event sending should be the last thing you do. Otherwise, a listener to the
event might add a new decoration to the UISourceCode which
you will accidentally delete in the next line.

caseq

https://codereview.chromium.org/2492343002/diff/370001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html File third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html (right): https://codereview.chromium.org/2492343002/diff/370001/third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html#newcode44 third_party/WebKit/LayoutTests/inspector/tracing/decorations-after-inplace-formatter.html:44: InspectorTest.addSniffer(Components.CoverageProfile.LineDecorator.prototype, "decorate", formatSource); So when "decorate" is invoked? Looks ...

4 years, 1 month ago (2016-11-17 22:13:19 UTC) #33

lushnikov

https://codereview.chromium.org/2492343002/diff/350002/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js File third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js (right): https://codereview.chromium.org/2492343002/diff/350002/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js#newcode121 third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js:121: var startLocation = new Array(2); drop these https://codereview.chromium.org/2492343002/diff/350002/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js#newcode124 third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js:124: ...

4 years, 1 month ago (2016-11-17 22:50:43 UTC) #36

lushnikov

https://codereview.chromium.org/2492343002/diff/440001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js File third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js (right): https://codereview.chromium.org/2492343002/diff/440001/third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js#newcode113 third_party/WebKit/Source/devtools/front_end/components_lazy/LineLevelProfile.js:113: this._uiLocation.uiSourceCode.removeDecorationsForType(Components.LineLevelProfile.LineDecorator.type); why do you want to remove all the ...

4 years, 1 month ago (2016-11-18 00:43:15 UTC) #38

caseq

lgtm https://codereview.chromium.org/2492343002/diff/460001/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js File third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js (right): https://codereview.chromium.org/2492343002/diff/460001/third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js#newcode126 third_party/WebKit/Source/devtools/front_end/sources/InplaceFormatterEditorAction.js:126: endLocation[0], endLocation[1]), nit: this can also be written ...

4 years, 1 month ago (2016-11-18 02:09:55 UTC) #41

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-18 18:01:01 UTC) #45

commit-bot: I haz the power

4 years, 1 month ago (2016-11-18 20:01:07 UTC) #49

Message was sent while issue was closed.

Patchset 26 (id:??) landed as
https://crrev.com/354cd6c5a2e6b2a8423816eecada2aeeb5a1a41c
Cr-Commit-Position: refs/heads/master@{#433271}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages