DevTools: insert console message decorations in order

DevTools: insert console message decorations in order

Console messages can have decorations, which may be persistent, toggled via
settings, or dependent on nesting level or repeat count. Before, they inserted
themselves into the DOM, and only by change happened to appear in the order:

Nesting level markers > Level icon > Timestamp / Repeat count(grows)

Now, they will appear in deterministic order, as if they were columns:
Timestamp > Level icon > Nesting level markers > Repeat count(grows)

Please see comment #11 in the bug for screenshots.

BUG=679815

[luoe, 2017-01-13 18:25:18]

Description was changed from

==========
DevTools: option to show context labels for console messages

BUG=679815
==========

to

==========
DevTools: option to show context labels for console messages

Adds a gray label to indicate the execution context (top, iframe, extension,
worker, etc) for each console message, similar to timestamps.  A message's
context label is shown if it is different from the currently selected context.

BUG=679815
==========

[luoe, 2017-01-13 18:25:51]

luoe@chromium.org changed reviewers:
+ allada@chromium.org

[luoe, 2017-01-13 18:25:51]

ptal for early review.  test coming soon.

[luoe, 2017-01-18 03:46:41]

ready for review

[luoe, 2017-01-18 19:32:53]

luoe@chromium.org changed reviewers:
+ caseq@chromium.org

[luoe, 2017-01-18 19:32:54]

caseq@, are you available to take a look?

[luoe, 2017-01-18 20:30:36]

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

[luoe, 2017-01-19 01:33:00]

There's a new screenshot in the crbug comment #6.  Timestamps and context labels
take up less space, have tooltips, and ctrl-C/export still copy the full text. 
ptal

[allada, 2017-01-19 02:38:08]

looks good!

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:6:
<script>
nit: can we merge these two script tags?

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:12:
var worker = new Worker("resources/worker-logger.js");
There's no guarantee that this worker will have executed any of it's code by the
time the test() is ran. Lets do a sniffer or something in the test() to ensure
that we only run our test when we know our test is in a good state.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:22:
var values = Array.prototype.map.call(options, option => option.value);
lets sort it just to protect against flakiness, but just dump out to make sure
main frame is first before you sort.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:23:
InspectorTest.addResult('Context options: ' + values);
Lets remove this line since it's doing it below.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(left):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:46:
result = '\u00a0\u00a0\u00a0\u00a0' + (executionContext.label() ||
executionContext.origin);
Looks like this unicode stuff was used for indenting for drop down context. Lets
move the indenting aspect to the dropdown specific code and let it choose when
to indent it or not.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:14:
static titleFor(executionContext, showStatus) {
I feel like this isn't very descriptive, I would suggest something like:
"titleForContext" or something.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:16:
if (executionContext.isDefault) {
I suggest something more like:

if (executionContext.isDefault && executionContext.frameId) {
  var resourceTreeModel =
SDK.ResourceTreeModel.fromTarget(executionContext.target());
  var frame = resourceTreeModel &&
resourceTreeModel.frameForId(executionContext.frameId);
  result = frame ? frame.displayName() : executionContext.label(showStatus);
} else if(executionContext.isDefault) {
  result =
executionContext.target().decorateLabel(executionContext.label(showStatus));
} else {
  result = executionContext.label(showStatus) || executionContext.origin;
}

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:28:
var maxLength = 50;
nit: inline or change this to const.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:337:
this._consoleMessages.map(message => {
nit: Remove curlies and put on same line.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:337:
this._consoleMessages.map(message => {
Don't use map, use .forEach()

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:850:
this._contextLabelElement.textContent = '';
Lets move these to the end of the function and store the data that will be in
them in local variables.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:461:
this._versionId;
nit: lets make these empty strings when undefined.

[luoe, 2017-01-19 23:23:00]

ptal

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:6:
<script>
On 2017/01/19 02:38:07, allada wrote:
> nit: can we merge these two script tags?

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:12:
var worker = new Worker("resources/worker-logger.js");
On 2017/01/19 02:38:07, allada wrote:
> There's no guarantee that this worker will have executed any of it's code by
the
> time the test() is ran. Lets do a sniffer or something in the test() to ensure
> that we only run our test when we know our test is in a good state.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:22:
var values = Array.prototype.map.call(options, option => option.value);
On 2017/01/19 02:38:07, allada wrote:
> lets sort it just to protect against flakiness, but just dump out to make sure
> main frame is first before you sort.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-multiple-execution-contexts.html:23:
InspectorTest.addResult('Context options: ' + values);
On 2017/01/19 02:38:07, allada wrote:
> Lets remove this line since it's doing it below.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:14:
static titleFor(executionContext, showStatus) {
On 2017/01/19 02:38:08, allada wrote:
> I feel like this isn't very descriptive, I would suggest something like:
> "titleForContext" or something.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:28:
var maxLength = 50;
On 2017/01/19 02:38:08, allada wrote:
> nit: inline or change this to const.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:337:
this._consoleMessages.map(message => {
On 2017/01/19 02:38:08, allada wrote:
> Don't use map, use .forEach()

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:337:
this._consoleMessages.map(message => {
On 2017/01/19 02:38:08, allada wrote:
> nit: Remove curlies and put on same line.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:850:
this._contextLabelElement.textContent = '';
On 2017/01/19 02:38:08, allada wrote:
> Lets move these to the end of the function and store the data that will be in
> them in local variables.

Done.

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/2623143002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:461:
this._versionId;
On 2017/01/19 02:38:08, allada wrote:
> nit: lets make these empty strings when undefined.

Done.

https://codereview.chromium.org/2623143002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2623143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:21:
result = executionContext.target().decorateLabel(result);
It turns out that browser extensions can load dedicated workers, in which case,
the execution context is not default, but we still want the decoration (gear
icon) consistent with other workers.

https://codereview.chromium.org/2623143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:27:
}
Since the presentation logic for titles in the selector dropdown is in multiple
places, it was more convenient to use the flag passed in to decide whether to
apply this.

[dgozman, 2017-01-20 00:01:32]

https://codereview.chromium.org/2623143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/2623143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/RuntimeModel.js:461:
this._versionId = '';
We cannot have versionId/versionStatus in ExecutionContext.

Instead, we can either:
- set two labels (short + long);
- use external entity to label contexts, which will consult with service workers
department.

[luoe, 2017-01-28 00:59:36]

Description was changed from

==========
DevTools: option to show context labels for console messages

Adds a gray label to indicate the execution context (top, iframe, extension,
worker, etc) for each console message, similar to timestamps.  A message's
context label is shown if it is different from the currently selected context.

BUG=679815
==========

to

==========
DevTools: option to show context labels for console messages

Introduces a setting (off by default) that adds icons next to console messages
to indicate context for workers (gear), extensions (puzzle piece), and iframes
(box). Messages from the top context or the main page do not get an icon.

BUG=679815
==========

[luoe, 2017-01-28 01:01:03]

Description was changed from

==========
DevTools: option to show context labels for console messages

Introduces a setting (off by default) that adds icons next to console messages
to indicate context for workers (gear), extensions (puzzle piece), and iframes
(box). Messages from the top context or the main page do not get an icon.

BUG=679815
==========

to

==========
DevTools: option to show context labels for console messages

Introduces a setting (off by default) that adds icons next to console messages
to indicate context for workers (gear), extensions (puzzle piece), and iframes
(box). Messages from the top context or the main page do not get an icon.
Icons will never overlap with other elements.

BUG=679815
==========

[luoe, 2017-01-28 01:01:22]

I've reworked this CL to instead show icons for different contexts. Icons show
up on the left, before error/warning icons.

ConsoleViewMessage.updateContextLabel() has a lot of logic about getting target
name/info. I was not sure if it's big enough to be moved.

Please take a look

https://codereview.chromium.org/2623143002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:54:
this._timestampElement;
Drive by

https://codereview.chromium.org/2623143002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:834:
}
Drive by change

[dgozman, 2017-01-30 22:19:27]

Let's explore removing error and warning icons entirely?

[luoe, 2017-02-01 18:05:15]

Description was changed from

==========
DevTools: option to show context labels for console messages

Introduces a setting (off by default) that adds icons next to console messages
to indicate context for workers (gear), extensions (puzzle piece), and iframes
(box). Messages from the top context or the main page do not get an icon.
Icons will never overlap with other elements.

BUG=679815
==========

to

==========
DevTools: option to show context labels for console messages

Introduces a setting (off by default) that adds icons next to console messages
to indicate context for workers (gear), extensions (puzzle piece), and iframes
(box). Messages from the top context or the main page do not get an icon.

Message decorations are now added in an explicit order as follows:
1. Timestamp
2. Context Icon
3. Message Level Icon
4. Group Nesting Level markers
5. Repeat count

When the context icon setting is set to Single, Message Level Icon will not be
shown, and the Command/Result icons will be added into the Context Icon.

BUG=679815
==========

[luoe, 2017-02-02 02:15:30]

Description was changed from

==========
DevTools: option to show context labels for console messages

Introduces a setting (off by default) that adds icons next to console messages
to indicate context for workers (gear), extensions (puzzle piece), and iframes
(box). Messages from the top context or the main page do not get an icon.

Message decorations are now added in an explicit order as follows:
1. Timestamp
2. Context Icon
3. Message Level Icon
4. Group Nesting Level markers
5. Repeat count

When the context icon setting is set to Single, Message Level Icon will not be
shown, and the Command/Result icons will be added into the Context Icon.

BUG=679815
==========

to

==========
DevTools: insert console message decorations in order

Console messages can have decorations, which may be persistent, toggled via
settings, or dependent on nesting level and repeat count. Before, they inserted
themselves into the DOM, and only by change happened to appear in the order:
Nesting level markers > Level icon > Timestamp / Repeat count(grows)

Now, they will appear in deterministic order, as if they were columns:
Timestamp > Level icon > Nesting level markers > Repeat count(grows)

BUG=679815
==========

[luoe, 2017-02-02 02:15:52]

Description was changed from

==========
DevTools: insert console message decorations in order

Console messages can have decorations, which may be persistent, toggled via
settings, or dependent on nesting level and repeat count. Before, they inserted
themselves into the DOM, and only by change happened to appear in the order:
Nesting level markers > Level icon > Timestamp / Repeat count(grows)

Now, they will appear in deterministic order, as if they were columns:
Timestamp > Level icon > Nesting level markers > Repeat count(grows)

BUG=679815
==========

to

==========
DevTools: insert console message decorations in order

Console messages can have decorations, which may be persistent, toggled via
settings, or dependent on nesting level or repeat count. Before, they inserted
themselves into the DOM, and only by change happened to appear in the order:

Nesting level markers > Level icon > Timestamp / Repeat count(grows)

Now, they will appear in deterministic order, as if they were columns:
Timestamp > Level icon > Nesting level markers > Repeat count(grows)

BUG=679815
==========

[luoe, 2017-02-02 07:59:11]

As discussed offline, we will have more than one setting for the context icons. 
The CL was growing quickly, so I will put the context changes into a future CL
and have this one be a preparation patch that just introduces deterministic
ordering.

Please take a look

[luoe, 2017-02-02 07:59:34]

Description was changed from

==========
DevTools: insert console message decorations in order

Console messages can have decorations, which may be persistent, toggled via
settings, or dependent on nesting level or repeat count. Before, they inserted
themselves into the DOM, and only by change happened to appear in the order:

Nesting level markers > Level icon > Timestamp / Repeat count(grows)

Now, they will appear in deterministic order, as if they were columns:
Timestamp > Level icon > Nesting level markers > Repeat count(grows)

BUG=679815
==========

to

==========
DevTools: insert console message decorations in order

Console messages can have decorations, which may be persistent, toggled via
settings, or dependent on nesting level or repeat count. Before, they inserted
themselves into the DOM, and only by change happened to appear in the order:

Nesting level markers > Level icon > Timestamp / Repeat count(grows)

Now, they will appear in deterministic order, as if they were columns:
Timestamp > Level icon > Nesting level markers > Repeat count(grows)

Please see comment #11 in the bug for screenshots.

BUG=679815
==========

[dgozman, 2017-02-03 21:16:41]

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

[dgozman, 2017-02-03 21:16:43]

Overall looks good, but we now create all the decorations even if we haven't
asked for the element yet. This means we create unnecessary nodes for possibly
zillion console messages without a reason. Let's fix that.

Adding Andrey for thorough review, as he knows this code best.

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1027:
updateMessageLevelIcon() {
Make it private?

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/consoleView.css
(right):

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/consoleView.css:180:
height: calc(100% + 1px);
Oh my. Can we avoid this?

[luoe, 2017-02-03 22:45:05]

We only need decorations when there is content to decorate, so each update*()
checks for _contentElement.

There will be more messageLevelIcons, however.  Before, level icons used
position:absolute to reside in space made by the message's padding.  In order to
make a column of 'empty space' after timestamps but before the nesting markers,
there will be empty messageLevelIcons where there used to be nothing.  While
this means there will be more elements generated than before, I don't have a way
around it.

ptal

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1027:
updateMessageLevelIcon() {
On 2017/02/03 21:16:42, dgozman (OOO till Feb 13) wrote:
> Make it private?

Done.

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/consoleView.css
(right):

https://codereview.chromium.org/2623143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/consoleView.css:180:
height: calc(100% + 1px);
On 2017/02/03 21:16:43, dgozman (OOO till Feb 13) wrote:
> Oh my. Can we avoid this?

Done.

[pfeldman, 2017-02-04 01:55:18]

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

[pfeldman, 2017-02-04 01:55:20]

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:901:
if (!this._nestingLevelMarkersElement && this._nestingLevel > 0) {
The second time _updateNestingLevelMarkers is called, we already have
this._nestingLevelMarkersElement. And we add more and children into it, so it
grows indefinitely?

The way you cache things here is error-prone. I'd rather rebuild the element
upon update as it was previously.

Also, can nesting level change? It is different from other decorations in that
regard.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1004:
this._updateMessageLevelIcon();
'create' these instead of 'updating'.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1014:
addElementIfNeededBound(this._repeatCountElement);
you can do 

[a, b, c].forEach(addElementIfNeeded.bind(this));

also, you are prepending, so you could name the method as such.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1024:
if (!elementToAdd)
Why are these nullable? We want inset to be the same at all time, so it seems
like they should present at least as the slots that maintain consistent
geometry.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/consoleView.css
(right):

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/consoleView.css:107:
.message-level-icon {
let's name these consistently.

[luoe, 2017-02-06 19:51:55]

As pfeldman mentioned, using, inserting, and deleting cached decorations is
error-prone.

Instead of inserting elements whenever decorations are updated, there are now
'slots' for each decoration that are always on every message.  The repeat
counter slot has an internal element that is cached since repeat counter can
update often.

Please take another look

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:901:
if (!this._nestingLevelMarkersElement && this._nestingLevel > 0) {
Nesting level doesn't change and messages don't grow indefinitely due to
grouping.

I've updated the semantics for almost all update()'s to clear its decoration
completely and rebuild contents as needed.

One exception is the repeat counter, which thrashes a ton unless the element is
reused.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1004:
this._updateMessageLevelIcon();
Timestamps and repeat counters can disappear after calling 'create', which is
confusing.  How about 'rebuild'?

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1014:
addElementIfNeededBound(this._repeatCountElement);
On 2017/02/04 01:55:19, pfeldman wrote:
> you can do 
> 
> [a, b, c].forEach(addElementIfNeeded.bind(this));
> 
> also, you are prepending, so you could name the method as such.

Acknowledged.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1024:
if (!elementToAdd)
On 2017/02/04 01:55:19, pfeldman wrote:
> Why are these nullable? We want inset to be the same at all time, so it seems
> like they should present at least as the slots that maintain consistent
> geometry.

Acknowledged.

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/consoleView.css
(right):

https://codereview.chromium.org/2623143002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/consoleView.css:107:
.message-level-icon {
On 2017/02/04 01:55:19, pfeldman wrote:
> let's name these consistently.

Done.

[pfeldman, 2017-02-06 22:30:52]

if we are viewported, we could actually create all columns and populate them
with data at all times. and then toggle them using css. that might help reducing
the code complexity.

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:51:
/** @type {!Element|undefined} */
You could assign these to null for clarity.

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:838:
if (!this._timestampSlot)
This should never happen.

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:896:
_rebuildNestingLevelMarkers() {
You only call it once, call it _buildNestingLevelMarkers

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:897:
if (!this._nestingLevelMarkersSlot)
ditto

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1009:
if (!this._levelSlot)
ditto, also don't see that you call it more than once.

[luoe, 2017-02-07 00:50:33]

Before, toggling timestamps used to build timestamps for all messages, even
those offscreen.  Now, it just relies on the viewport to call wasShown for just
the rendered messages.

Please take another look

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:51:
/** @type {!Element|undefined} */
On 2017/02/06 22:30:52, pfeldman wrote:
> You could assign these to null for clarity.

Done.

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:838:
if (!this._timestampSlot)
On 2017/02/06 22:30:52, pfeldman wrote:
> This should never happen.

Done.

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:896:
_rebuildNestingLevelMarkers() {
After refactoring, it was small enough that I inlined it at callsite,
updateMessageElement()

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:897:
if (!this._nestingLevelMarkersSlot)
On 2017/02/06 22:30:52, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/2623143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1009:
if (!this._levelSlot)
On 2017/02/06 22:30:52, pfeldman wrote:
> ditto, also don't see that you call it more than once.

Done.

[pfeldman, 2017-02-07 01:11:35]

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:82:
this._rebuildTimestamp();
Why would we rebuild these slots over and over as user scrolls?

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:838:
_rebuildTimestamp() {
I don't see how timestamp becomes visible upon toggling the setting now.

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:849:
this._timestampElement.hidden = !show;
.classList.toggle('hidden', !show)

[luoe, 2017-02-07 17:33:05]

I've addressed the comments with what we discussed offline.  I think this CL is
wrapping up, but if you forsee further changes, please let me know if I should
make a new clean CL with fewer patch sets!

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:82:
this._rebuildTimestamp();
As discussed offline, it should not be too expensive to destroy decorations and
build new ones when they are shown.  This allows us to keep a very simple model
without caching.

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:838:
_rebuildTimestamp() {
On 2017/02/07 01:11:35, pfeldman wrote:
> I don't see how timestamp becomes visible upon toggling the setting now.

Done.

https://codereview.chromium.org/2623143002/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:849:
this._timestampElement.hidden = !show;
On 2017/02/07 01:11:35, pfeldman wrote:
> .classList.toggle('hidden', !show)

Done.

[pfeldman, 2017-02-07 18:50:29]

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:97:
_buildDecorations() {
rebuildDecorations

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:122:
this._nestingLevelMarkers = [];
You are now calling it on wasShown, I don't think _nestingLevelMarkers should be
mutated just because something was shown.

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1023:
_rebuildRepeatCountElement() {
That is now _updateRepeatCountElement, could we also inline it in the same
decorations method? Then we just call updateMessage on the one that has repeat
changed.

[pfeldman, 2017-02-07 18:51:14]

(i don't think your patch applies to ToT)

[luoe, 2017-02-07 19:46:46]

Rebased, it should apply without conflicts now.  ptal

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:97:
_buildDecorations() {
On 2017/02/07 18:50:29, pfeldman wrote:
> rebuildDecorations

Done.

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:122:
this._nestingLevelMarkers = [];
Yes, moved to updateMessageElement().

https://codereview.chromium.org/2623143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1023:
_rebuildRepeatCountElement() {
Renamed.

I don't think we can safely inline it, the callers resetIncrementRepeatCount()
and incrementRepeatCount() cannot safely call anything that removes and adds
things to the DOM.

What we can do is move the switch logic into updateMessageElement(), since it
doesn't change when the counter increments.

[pfeldman, 2017-02-07 23:02:52]

lgtm, but let's follow-up to remove separate repeat count update.

[luoe, 2017-02-07 23:04:40]

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

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

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-02-08 00:42:38]

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

[commit-bot: I haz the power, 2017-02-08 00:42:39]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-02-18 02:53:27]

luoe@chromium.org changed reviewers:
- caseq@chromium.org

[luoe, 2017-02-18 02:53:30]

Now that the other wasShown() CL has landed, I've made a small number of
changes:
- rebuildDecorations() is public, called by updateMessageList and wasShown
- Empty textNode fix to keep selection working

To pfeldman@'s suggestion, I've separated out the repeat count followup into
another CL:
https://codereview.chromium.org/2702913002/

Please take a look, or let me know if I should migrate to a clean new CL.

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:710:
this._visibleViewMessages.forEach(message => message.rebuildDecorations());
Now that wasShown() is called only on things that were not shown before, we can
call rebuildDecorations in wasShown() and here for all visible messages.

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:104:
this._decorationWrapper.createTextChild('');
In addition to making rebuildDecorations() public, this change is required so
that console-viewport-selection.html works.

[luoe, 2017-02-21 19:23:07]

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

[commit-bot: I haz the power, 2017-02-21 19:23:58]

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

[pfeldman, 2017-02-21 19:26:04]

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:104:
this._decorationWrapper.createTextChild('');
Decorations should be a private details of the message, why are we making it
public? We need to figure out a language console view talks to console view
message so that it is easy to comprehend and maintain.

[luoe, 2017-02-21 19:27:08]

The CQ bit was unchecked by luoe@chromium.org

[luoe, 2017-02-21 23:56:42]

ptal

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:104:
this._decorationWrapper.createTextChild('');
Back to private.  updateMessageList() now calls updateMessageElement(),
_rebuildDecorations() is only called in wasShown

[pfeldman, 2017-02-22 00:10:46]

We probably should take smaller steps in fixing it, maybe draft the use cases
and a rough plan?

https://codereview.chromium.org/2623143002/diff/640001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-log-linkify-stack-in-errors-expected.txt
(right):

https://codereview.chromium.org/2623143002/diff/640001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-log-linkify-stack-in-errors-expected.txt:66:
at foob.js:8 message-decoration-wrapper > message-level-icon >
message-repeat-count hidden > info > console-message > source-code >
console-message-anchor > devtools-link > console-message-text > devtools-link >
devtools-link
What are we dumping here exactly? It does not look like all these messages are
nested.

https://codereview.chromium.org/2623143002/diff/640001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2623143002/diff/640001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:710:
this._visibleViewMessages.forEach(message => message.updateMessageElement());
This still does not make much sense to me! Why do we invalidate and also rebuild
all the messages in one call?

https://codereview.chromium.org/2623143002/diff/640001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
(right):

https://codereview.chromium.org/2623143002/diff/640001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:1008:
case SDK.ConsoleMessage.MessageLevel.Warning:
How come it is in a different location from updating the decorations?

[luoe, 2017-05-10 21:09:19]

luoe@chromium.org changed reviewers:
- allada@chromium.org, dgozman@chromium.org, lushnikov@chromium.org,
pfeldman@chromium.org