Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651843003:
DevTools: update console timestamp style (Closed)
|
Created:
3 years, 11 months ago by luoe Modified:
3 years, 10 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: update console timestamp style
A few bugs with console timestamps are addressed:
- All messages get timestamps; previously, commands/evaluations did not.
- Timestamps should not wrap
- Turning on timestamps should remove the repeated-message class
- New setting to choose [none, full timestamp, short timestamp (no yy:mm:dd)]
With the addition of a new setting, users who previously had timestamps enabled
will be migrated to the short format by default.
BUG=651927, 678585, 682049
Review-Url: https://codereview.chromium.org/2651843003
Cr-Commit-Position: refs/heads/master@{#447065}
Committed: https://chromium.googlesource.com/chromium/src/+/d674aa2e6ff1a72c7a427ef033cc75e48cc61876
Patch Set 1 #
Total comments: 6
Patch Set 2 : ac #
Total comments: 13
Patch Set 3 : ac #Patch Set 4 : nochange #
Total comments: 1
Messages
Total messages: 45 (27 generated)
Description was changed from ========== DevTools: update console timestamp style BUG=651927, 678585, 682049 ========== to ========== DevTools: update console timestamp style A few bugs with console timestamps are addressed: - All messages get timestamps; previously, commands/evaluations did not. - Timestamps should not wrap - Turning on timestamps should remove the repeated-message class - New setting to choose [none, full timestamp, short timestamp (no yy:mm:dd)] With the addition of a new setting, users who previously had timestamps enabled will be migrated to the short format by default. BUG=651927, 678585, 682049 ==========
luoe@chromium.org changed reviewers: + chenwilliam@chromium.org, dgozman@chromium.org, lushnikov@chromium.org
Please take a look.
https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:827: var timestamp = (new Date(this._message.timestamp)).toConsoleTime(); Instead of extending the Date prototype... you can create a function: formatTimestampToYearMonthDay() https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/platform/utilities.js (right): https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:400: function leadZero(x) { Not related to your change, but why is this called differently than leadZero2 below? https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:410: Date.prototype.toConsoleTime = function() { I would just remove this method and create a utility method in the Console class since this method isn't used outside of Console. If there's a use case for re-using in another module and want to keep it in here, I think it's better to split it up into two functions (e.g. Date.prototype.toHourMinuteSecond() and ...toYearMonthDay()). AFAICT, there's nothing specific to Console.
https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:827: var timestamp = (new Date(this._message.timestamp)).toConsoleTime(); On 2017/01/24 03:32:13, chenwilliam wrote: > Instead of extending the Date prototype... you can create a function: > formatTimestampToYearMonthDay() Done. https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/platform/utilities.js (right): https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:400: function leadZero(x) { Doesn't seem to have a good reason. I left this one alone and wrote a new leadZero() in ConsoleViewMessage. https://codereview.chromium.org/2651843003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:410: Date.prototype.toConsoleTime = function() { No other need right now. Moved inline into ConsoleViewMessage.
Nice patch! https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:821: updateTimestamp(format) { Instead of passing format, grab it from the setting right here. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:827: if (!this.timestampElement) _timestampElement https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/module.json (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/module.json:109: "title": "Timestamps", Timestamps: https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/module.json:121: "text": "Full (yyyy-MM-dd HH:mm:ss.fff)", I'd make text "Full", while showing the format in title. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/module.json:126: "text": "Short (HH:mm:ss.fff)", Same here. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:829: 'consoleTimestampFormat', Remove.
ptal https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:821: updateTimestamp(format) { On 2017/01/25 00:51:46, dgozman wrote: > Instead of passing format, grab it from the setting right here. Done. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:827: if (!this.timestampElement) On 2017/01/25 00:51:46, dgozman wrote: > _timestampElement Done. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/module.json (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/module.json:109: "title": "Timestamps", On 2017/01/25 00:51:46, dgozman wrote: > Timestamps: Done. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/module.json:121: "text": "Full (yyyy-MM-dd HH:mm:ss.fff)", On 2017/01/25 00:51:46, dgozman wrote: > I'd make text "Full", while showing the format in title. Done. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/module.json:126: "text": "Short (HH:mm:ss.fff)", On 2017/01/25 00:51:46, dgozman wrote: > Same here. Done. https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:829: 'consoleTimestampFormat', Done. I assumed this was used for all properties, but I guess it's just a snapshot then?
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js (right): https://codereview.chromium.org/2651843003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:829: 'consoleTimestampFormat', On 2017/01/25 19:43:21, luoe wrote: > Done. I assumed this was used for all properties, but I guess it's just a > snapshot then? Correct.
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1260
error: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
patch does not apply
Patch: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
Index: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
diff --git a/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
b/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
index
87c0470fd53622c3f91b73d47378b209e67cb58a..34289021d98a7a0bdeb0e8d84c7550c4c94c511c
100644
--- a/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
+++ b/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
@@ -145,7 +145,7 @@ Console.ConsoleView = class extends UI.VBox {
this._consoleHistoryAutocompleteChanged();
this._updateFilterStatus();
-
Common.moduleSetting('consoleTimestampsEnabled').addChangeListener(this._consoleTimestampsSettingChanged,
this);
+
Common.moduleSetting('consoleTimestampFormat').addChangeListener(this._consoleTimestampsSettingChanged,
this);
this._registerWithMessageSink();
SDK.targetManager.observeTargets(this);
@@ -319,15 +319,9 @@ Console.ConsoleView = class extends UI.VBox {
this._addConsoleMessage(consoleMessage);
}
- /**
- * @param {!Common.Event} event
- */
- _consoleTimestampsSettingChanged(event) {
- var enabled = /** @type {boolean} */ (event.data);
+ _consoleTimestampsSettingChanged() {
this._updateMessageList();
- this._consoleMessages.forEach(function(viewMessage) {
- viewMessage.updateTimestamp(enabled);
- });
+ this._consoleMessages.forEach(viewMessage =>
viewMessage.updateTimestamp());
}
_executionContextChanged() {
@@ -681,8 +675,9 @@ Console.ConsoleView = class extends UI.VBox {
* @return {boolean}
*/
_tryToCollapseMessages(lastMessage, viewMessage) {
- if (!Common.moduleSetting('consoleTimestampsEnabled').get() && viewMessage
&&
- !lastMessage.consoleMessage().isGroupMessage() &&
+ var timestampFormat = Common.moduleSetting('consoleTimestampFormat').get();
+ var timestampsShown = timestampFormat !==
Console.ConsoleViewMessage.TimestampFormat.None;
+ if (!timestampsShown && viewMessage &&
!lastMessage.consoleMessage().isGroupMessage() &&
lastMessage.consoleMessage().isEqual(viewMessage.consoleMessage())) {
viewMessage.incrementRepeatCount();
return true;
@@ -1220,6 +1215,8 @@ Console.ConsoleCommand = class extends
Console.ConsoleViewMessage {
} else {
this._updateSearch();
}
+
+ this.updateTimestamp();
}
return this._contentElement;
}
@@ -1260,7 +1257,6 @@ Console.ConsoleCommandResult = class extends
Console.ConsoleViewMessage {
var icon = UI.Icon.create('smallicon-command-result',
'command-result-icon');
element.insertBefore(icon, element.firstChild);
}
- this.updateTimestamp(false);
return element;
}
};
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1260
error: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
patch does not apply
Patch: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
Index: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
diff --git a/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
b/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
index
87c0470fd53622c3f91b73d47378b209e67cb58a..34289021d98a7a0bdeb0e8d84c7550c4c94c511c
100644
--- a/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
+++ b/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
@@ -145,7 +145,7 @@ Console.ConsoleView = class extends UI.VBox {
this._consoleHistoryAutocompleteChanged();
this._updateFilterStatus();
-
Common.moduleSetting('consoleTimestampsEnabled').addChangeListener(this._consoleTimestampsSettingChanged,
this);
+
Common.moduleSetting('consoleTimestampFormat').addChangeListener(this._consoleTimestampsSettingChanged,
this);
this._registerWithMessageSink();
SDK.targetManager.observeTargets(this);
@@ -319,15 +319,9 @@ Console.ConsoleView = class extends UI.VBox {
this._addConsoleMessage(consoleMessage);
}
- /**
- * @param {!Common.Event} event
- */
- _consoleTimestampsSettingChanged(event) {
- var enabled = /** @type {boolean} */ (event.data);
+ _consoleTimestampsSettingChanged() {
this._updateMessageList();
- this._consoleMessages.forEach(function(viewMessage) {
- viewMessage.updateTimestamp(enabled);
- });
+ this._consoleMessages.forEach(viewMessage =>
viewMessage.updateTimestamp());
}
_executionContextChanged() {
@@ -681,8 +675,9 @@ Console.ConsoleView = class extends UI.VBox {
* @return {boolean}
*/
_tryToCollapseMessages(lastMessage, viewMessage) {
- if (!Common.moduleSetting('consoleTimestampsEnabled').get() && viewMessage
&&
- !lastMessage.consoleMessage().isGroupMessage() &&
+ var timestampFormat = Common.moduleSetting('consoleTimestampFormat').get();
+ var timestampsShown = timestampFormat !==
Console.ConsoleViewMessage.TimestampFormat.None;
+ if (!timestampsShown && viewMessage &&
!lastMessage.consoleMessage().isGroupMessage() &&
lastMessage.consoleMessage().isEqual(viewMessage.consoleMessage())) {
viewMessage.incrementRepeatCount();
return true;
@@ -1220,6 +1215,8 @@ Console.ConsoleCommand = class extends
Console.ConsoleViewMessage {
} else {
this._updateSearch();
}
+
+ this.updateTimestamp();
}
return this._contentElement;
}
@@ -1260,7 +1257,6 @@ Console.ConsoleCommandResult = class extends
Console.ConsoleViewMessage {
var icon = UI.Icon.create('smallicon-command-result',
'command-result-icon');
element.insertBefore(icon, element.firstChild);
}
- this.updateTimestamp(false);
return element;
}
};
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:1260
error: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:
patch does not apply
Patch: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
Index: third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
diff --git a/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
b/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
index
87c0470fd53622c3f91b73d47378b209e67cb58a..34289021d98a7a0bdeb0e8d84c7550c4c94c511c
100644
--- a/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
+++ b/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
@@ -145,7 +145,7 @@ Console.ConsoleView = class extends UI.VBox {
this._consoleHistoryAutocompleteChanged();
this._updateFilterStatus();
-
Common.moduleSetting('consoleTimestampsEnabled').addChangeListener(this._consoleTimestampsSettingChanged,
this);
+
Common.moduleSetting('consoleTimestampFormat').addChangeListener(this._consoleTimestampsSettingChanged,
this);
this._registerWithMessageSink();
SDK.targetManager.observeTargets(this);
@@ -319,15 +319,9 @@ Console.ConsoleView = class extends UI.VBox {
this._addConsoleMessage(consoleMessage);
}
- /**
- * @param {!Common.Event} event
- */
- _consoleTimestampsSettingChanged(event) {
- var enabled = /** @type {boolean} */ (event.data);
+ _consoleTimestampsSettingChanged() {
this._updateMessageList();
- this._consoleMessages.forEach(function(viewMessage) {
- viewMessage.updateTimestamp(enabled);
- });
+ this._consoleMessages.forEach(viewMessage =>
viewMessage.updateTimestamp());
}
_executionContextChanged() {
@@ -681,8 +675,9 @@ Console.ConsoleView = class extends UI.VBox {
* @return {boolean}
*/
_tryToCollapseMessages(lastMessage, viewMessage) {
- if (!Common.moduleSetting('consoleTimestampsEnabled').get() && viewMessage
&&
- !lastMessage.consoleMessage().isGroupMessage() &&
+ var timestampFormat = Common.moduleSetting('consoleTimestampFormat').get();
+ var timestampsShown = timestampFormat !==
Console.ConsoleViewMessage.TimestampFormat.None;
+ if (!timestampsShown && viewMessage &&
!lastMessage.consoleMessage().isGroupMessage() &&
lastMessage.consoleMessage().isEqual(viewMessage.consoleMessage())) {
viewMessage.incrementRepeatCount();
return true;
@@ -1220,6 +1215,8 @@ Console.ConsoleCommand = class extends
Console.ConsoleViewMessage {
} else {
this._updateSearch();
}
+
+ this.updateTimestamp();
}
return this._contentElement;
}
@@ -1260,7 +1257,6 @@ Console.ConsoleCommandResult = class extends
Console.ConsoleViewMessage {
var icon = UI.Icon.create('smallicon-command-result',
'command-result-icon');
element.insertBefore(icon, element.firstChild);
}
- this.updateTimestamp(false);
return element;
}
};
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2651843003/#ps60001 (title: "nochange")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485803463477810,
"parent_rev": "1b358ff908aa511571cb758016a50fa340d0b6e7", "commit_rev":
"d674aa2e6ff1a72c7a427ef033cc75e48cc61876"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: update console timestamp style A few bugs with console timestamps are addressed: - All messages get timestamps; previously, commands/evaluations did not. - Timestamps should not wrap - Turning on timestamps should remove the repeated-message class - New setting to choose [none, full timestamp, short timestamp (no yy:mm:dd)] With the addition of a new setting, users who previously had timestamps enabled will be migrated to the short format by default. BUG=651927, 678585, 682049 ========== to ========== DevTools: update console timestamp style A few bugs with console timestamps are addressed: - All messages get timestamps; previously, commands/evaluations did not. - Timestamps should not wrap - Turning on timestamps should remove the repeated-message class - New setting to choose [none, full timestamp, short timestamp (no yy:mm:dd)] With the addition of a new setting, users who previously had timestamps enabled will be migrated to the short format by default. BUG=651927, 678585, 682049 Review-Url: https://codereview.chromium.org/2651843003 Cr-Commit-Position: refs/heads/master@{#447065} Committed: https://chromium.googlesource.com/chromium/src/+/d674aa2e6ff1a72c7a427ef033cc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d674aa2e6ff1a72c7a427ef033cc...
Message was sent while issue was closed.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2651843003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Settings.js (right): https://codereview.chromium.org/2651843003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Settings.js:712: _updateVersionFrom22To23() { Lets not switch to the short/long, this adds another unnecessary option to the list.
Message was sent while issue was closed.
For reference, a revert CL is tracked here: https://codereview.chromium.org/2683713004/ |
