This change is ready for review.
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
17 comments:
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
Multimap.has() should clean up properly if the Set is empty. Other areas would break if .has() does not work.
Patch Set #2, Line 693: viewMessage
viewMessage may not exist any more.
Patch Set #2, Line 693: lastMessage
lastMessage will always exist now.
Patch Set #2, Line 718: this._consoleMessages[i].setInSimilarGroup(false);
Can we move this into a listener when the setting changes? This appears to be a hot path.
Patch Set #2, Line 745: x => this._shouldMessageBeVisible(x)
nit: this._shouldMessageBeVisible.bind(this)
Patch Set #2, Line 786: // TODO: fix this.
Who is the TODO for? and why should we fix it?
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:
Patch Set #2, Line 1032: this._similarGroupMarker = this._element.createChild('div', 'nesting-level-marker');
Can we cleanly not have it create this marker in two different places?
Patch Set #2, Line 1340: _tokenizeMessageText
This function is going to get ran quite a bit, can we shortcut it to 1357 if we have the types and regexs already built?
Patch Set #2, Line 1343: \/[\w\.-]*
Simplified would be: /\/[\w\.-\/]*:\d+/
Patch Set #2, Line 1343: var pathLineRegex = /(?:\/[\w\.-]*)+\:[\d]+/;
I don't understand what 'pathLine' is or why it needs to end with ':\d+'? Maybe we should remove this?
Patch Set #2, Line 1344: ([\d]+)
/took (?:\d+)ms/ ... We try not to capture.
Patch Set #2, Line 1345: (\w+)
(?:\w+) ... We try not to capture.
It might be a good idea to consider ensuring there is a spacelike character on each side of this, otherwise we are going to get crazy matching on things like base64.
Patch Set #2, Line 1346: [6-7]
Do we really want to force ourselves to keep updating this every year? This means within 2 years we'll need to change this to [6-8].
Patch Set #2, Line 1377: var result = tokens.reduce((acc, token) => {
Lets extract this named types into an enum. Then we can do something like:
result = tokens.reduce((acc, token) => acc + (myEnum[token.type] || token.text));
<N> for consistency?
what is this?
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
2 comments:
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
Patch Set #2, Line 745: if (!viewMessagesInGroupArray.find(x => this._shouldMessageBeVisible(x)))
this can be cached per group; otherwise, if all messages fall into the same group, it will be N^2?
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:
Patch Set #2, Line 151: if (firstActiveIndex <= i && i - firstActiveIndex < this._renderedItems.length && i <= lastActiveIndex)
why is this needed? renderedItems have exactly lastActiveIndex - firstActiveIndex + 1 elements:
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 2:Code-Review +1
1 comment:
since this is to avoid nasty exception, let's put a TODO here so that future readers won't be confused.
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
Played with this:
2 comments:
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:
Patch Set #2, Line 1375: groupTitle() {
Should we cache this?
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:
Patch Set #2, Line 151: if (firstActiveIndex <= i && i - firstActiveIndex < this._renderedItems.length && i <= lastActiveIndex)
Hmm... Are you breaking the invariant now? This worked previously. I am concerned something else might break.
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
More feedback:
Un-checking and re-checking 'Group similar' box sometimes gives me large blank gaps in console viewport :(
5 comments:
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
Patch Set #2, Line 761: viewMessagesInGroupArray.peekLast() === viewMessageInGroup
When a group contains N repeats, they are collapsed into the first occurring viewMessage. Really, we want `lastVisibleViewMessageInGroup === viewMessageInGroup`, which we don't have.
Repro: on about:blank, evaluate `document.write()` many times, expand the group, the 'group-closed' isn't applied to the visible child.
Might be fixable if ConsoleViewMessage L944 adds the similar-group-marker to the message's ._nestingLevelMarkers. Or maybe we could expose updateNestingLevel() ? That would also allow us to rely on our EndGroup to add the 'group-closed' for us.
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:
nit: /** @type {!Map<!RegExp, string>} */ (new Map())
Patch Set #2, Line 1352: eventRegex
Could we also add a handlerRegex? I think it'd be nice in a demo to group these:
'DOMContentLoaded' handler took 955ms
'setTimeout' handler took 63ms
as
Event handler took Nms
If we set the string to `Common.UIString('the future')`, we can get more messages to group together:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp?l=742
https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_client.cc?l=1696
what is this?
We use `%o` as a substitution parameter from the backend. Perhaps the regex can be `/ [%]o/g` to ensure we don't match on a '%o' that happens to be inside a user's URL.
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
24 comments:
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
Multimap.has() should clean up properly if the Set is empty. Other areas would break if . […]
Nice!
Patch Set #2, Line 693: lastMessage
lastMessage will always exist now.
I'll make viewMessage non-null.
Patch Set #2, Line 693: viewMessage
viewMessage may not exist any more.
Same as above?
Patch Set #2, Line 718: this._consoleMessages[i].setInSimilarGroup(false);
Can we move this into a listener when the setting changes? This appears to be a hot path.
It is not too hot - one need to expand the group and have element in the tree for serious overhead.
Patch Set #2, Line 745: if (!viewMessagesInGroupArray.find(x => this._shouldMessageBeVisible(x)))
this can be cached per group; otherwise, if all messages fall into the same group, it will be N^2?
If all the messages are in the same group, we only get here once.
Patch Set #2, Line 745: x => this._shouldMessageBeVisible(x)
nit: this._shouldMessageBeVisible. […]
I now use arrows when I can - it reads better.
Patch Set #2, Line 761: viewMessagesInGroupArray.peekLast() === viewMessageInGroup
When a group contains N repeats, they are collapsed into the first occurring viewMessage. […]
:( I see. This is getting too complicated, I'll need to follow up.
Patch Set #2, Line 786: // TODO: fix this.
Who is the TODO for? and why should we fix it?
Well, it is testing based on the classes it should not know about! Just look at the groupMessage.parentElement.message below. Basically, all the code below this line should not exist.
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:
Patch Set #2, Line 1032: this._similarGroupMarker = this._element.createChild('div', 'nesting-level-marker');
Can we cleanly not have it create this marker in two different places?
Why not?
Patch Set #2, Line 1340: _tokenizeMessageText
This function is going to get ran quite a bit, can we shortcut it to 1357 if we have the types and r […]
Done (it was taking 2 microseconds)
Patch Set #2, Line 1343: \/[\w\.-]*
Simplified would be: /\/[\w\. […]
This has not changed.
Patch Set #2, Line 1343: var pathLineRegex = /(?:\/[\w\.-]*)+\:[\d]+/;
I don't understand what 'pathLine' is or why it needs to end with ':\d+'? Maybe we should remove thi […]
This has not changed.
Patch Set #2, Line 1344: ([\d]+)
/took (?:\d+)ms/ ... We try not to capture.
Done
Patch Set #2, Line 1345: (\w+)
(?:\w+) ... We try not to capture.
Done
Patch Set #2, Line 1346: [6-7]
Do we really want to force ourselves to keep updating this every year? This means within 2 years we' […]
Yey! We'll celebrate Chrome 80 with updating it in a couple of years!
It might be a good idea to consider ensuring there is a spacelike character on each side of this, ot […]
Done
nit: /** @type {!Map<!RegExp, string>} */ (new Map())
Ack
Patch Set #2, Line 1352: eventRegex
Could we also add a handlerRegex? I think it'd be nice in a demo to group these: […]
Let's follow up. I haven't seen them trigger, so did not spot them.
Patch Set #2, Line 1375: groupTitle() {
Should we cache this?
We cache the console view message itself.
Patch Set #2, Line 1377: var result = tokens.reduce((acc, token) => {
Lets extract this named types into an enum. Then we can do something like: […]
That's too generic to my taste.
<N> for consistency?
Done
If we set the string to `Common. […]
Ack
We use `%o` as a substitution parameter from the backend. […]
Ack
File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:
Patch Set #2, Line 151: if (firstActiveIndex <= i && i - firstActiveIndex < this._renderedItems.length && i <= lastActiveIndex)
Hmm... Are you breaking the invariant now? This worked previously. […]
I'm not breaking the invariant. This code was broken:
invalidate() is doing
1. this._itemCount = this._provider.itemCount()
2. this._rebuildCumulativeHeights
(2) fails because itemCount is out of sync with this._renderedItems.
It never worked...
To view, visit change 731969. To unsubscribe, or for help writing mail filters, visit settings.
I know, but I don't want to ungroup repeat count - things might explode!
I can't reproduce this.
It does for me.
That's the deep text content I mentioned at the meeting.
Patch set 3:Commit-Queue +2
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"review comments addressed" https://chromium-review.googlesource.com/c/731969/3
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/731969/3
Bot data: {"action": "start", "triggered_at": "2017-10-23T07:03:07.0Z", "cq_cfg_revision": "573b6c39d3de239a70e8fa672647b63bf0bd1f89", "revision": "63303d27747d9ba3affe8c28905fc22e1f20e4e9"}
Commit Bot merged this change.
DevTools: automatic console message grouping changes:
- New message matching logic
- Setting in the toolbar
- Nesting marker
- Bugfixes
Bug: 774694
Change-Id: I1898b113e2c7e69873ddb31156c593a8eb655661
Reviewed-on: https://chromium-review.googlesource.com/731969
Commit-Queue: Pavel Feldman <pfel...@chromium.org>
Reviewed-by: Andrey Lushnikov <lush...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510737}
---
M third_party/WebKit/LayoutTests/http/tests/devtools/console-xhr-logging.js
M third_party/WebKit/LayoutTests/http/tests/devtools/console/console-timestamp.js
M third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
M third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js
M third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js
M third_party/WebKit/Source/devtools/front_end/console/consoleView.css
M third_party/WebKit/Source/devtools/front_end/console/module.json
M third_party/WebKit/Source/devtools/front_end/console_model/ConsoleModel.js
M third_party/WebKit/Source/devtools/front_end/main/Main.js
9 files changed, 183 insertions(+), 64 deletions(-)