Originally posted by:nchevobbe see https://github.com/devtools-html/gecko-dev/issues/88 Displays a timestamp with the `HH:ii:ss,.SSS` before the log message when the `devtools.webconsole.timestampMessages`is set to TRUE Test to be rewritten: - [ ] browser_webconsole_expandable_timestamps.js

Comment on attachment 8810120 [details] Bug 1307871 - Add new console frontend support for 'Enable timestamps' pref. https://reviewboard.mozilla.org/r/92524/#review92664 Thanks Tim for working on this ! I made some comments, mostly for consitency sake. I think we should also have mocha tests to check the rendering of the different components (e.g. http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js ). ::: devtools/client/webconsole/new-console-output/actions/messages.js:94 (Diff revision 1) > id, > data > }; > } > > +function toggleTimestamps(enabled) { same here, invert the name and the verb, i.e. `timestampsToggle` ::: devtools/client/webconsole/new-console-output/components/console-output.js:94 (Diff revision 1) > + let className = "webconsole-output"; > + > + if (!timestampsEnabled) { > + className += " hideTimestamps"; > + } I'd rather have a `classList` array variable, and then joinng : ``` let classList = ["webconsole-output"]; if(!timestampsEnabled) { classList.push("hideTimestamps"); } return ( dom.div({ className: classList.join(" ") ... ``` It makes it easier to work with if we have other modifications to the className later ::: devtools/client/webconsole/new-console-output/components/message.js:93 (Diff revision 1) > frame, > stacktrace, > serviceContainer, > dispatch, > exceptionDocURL, > + timeStamp = Date.now() IIRC, there is a timestamp property on the message type. We should use this rather than calling Date.now since we can display cached messages (and thus the timestamp should output the time the message was logged, not the time it is shown) ::: devtools/client/webconsole/new-console-output/constants.js:15 (Diff revision 1) > MESSAGE_ADD: "MESSAGE_ADD", > MESSAGES_CLEAR: "MESSAGES_CLEAR", > MESSAGE_OPEN: "MESSAGE_OPEN", > MESSAGE_CLOSE: "MESSAGE_CLOSE", > MESSAGE_TABLE_RECEIVE: "MESSAGE_TABLE_RECEIVE", > + TOGGLE_TIMESTAMPS: "TOGGLE_TIMESTAMPS", nit: should be in the NAME_VERB form, like the others, i.e. `TIMESTAMPS_TOGGLE` ::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:117 (Diff revision 1) > > dispatchMessagesClear: function () { > store.dispatch(actions.messagesClear()); > }, > + > + toggleTimestamps: function (enabled) { We'd better keep the same syntax that the other dispatcher function (i.e. `dispatchTimestampsToggle` ) ::: devtools/client/webconsole/new-console-output/reducers/messages.js:25 (Diff revision 1) > // Map of the form {groupMessageId : groupArray}, > // where groupArray is the list of of all the parent groups' ids of the groupMessageId. > groupsById: Immutable.Map(), > // Message id of the current group (no corresponding console.groupEnd yet). > currentGroup: null, > + timestampsEnabled: false I think this isn't the right reducer to put this in. We have a UI reducer (http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/reducers/ui.js), which better represent what you are trying to do here. ::: devtools/client/webconsole/new-console-output/reducers/messages.js:34 (Diff revision 1) > const { > messagesById, > messagesUiById, > messagesTableDataById, > groupsById, > - currentGroup > + currentGroup, nit: remove the trailing comma ::: devtools/client/webconsole/new-console-output/reducers/messages.js:103 (Diff revision 1) > return state.deleteIn(["messagesUiById", index]); > case constants.MESSAGE_TABLE_RECEIVE: > const {id, data} = action; > return state.set("messagesTableDataById", messagesTableDataById.set(id, data)); > + case constants.TOGGLE_TIMESTAMPS: > + return state.set("timestampsEnabled", action.enabled); I think we could have a better name here. We don't really "enable" timestamps, we just show them :) ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_timestamps.js:11 (Diff revision 1) > const TEST_URI = "data:text/html;charset=utf-8,Web Console test for " + > - "bug 722267 - preference for toggling timestamps in messages"; > + "bug 1307871 - preference for toggling timestamps in messages"; > const PREF_MESSAGE_TIMESTAMP = "devtools.webconsole.timestampMessages"; > -var hud; > > add_task(function* () { > - yield loadTab(TEST_URI); > + let hud = yield openNewTabAndConsole(TEST_URI); I don't know if we want to copy those old tests directly, bgrins would know better

Comment on attachment 8810120 [details] Bug 1307871 - Add new console frontend support for 'Enable timestamps' pref. https://reviewboard.mozilla.org/r/92524/#review92664 > nit: should be in the NAME_VERB form, like the others, i.e. `TIMESTAMPS_TOGGLE` My bad, I misread this

Issues reported by Nicolas were fixed.

Comment on attachment 8810120 [details] Bug 1307871 - Add new console frontend support for 'Enable timestamps' pref. https://reviewboard.mozilla.org/r/92524/#review92748 ::: devtools/client/webconsole/new-console-output/actions/messages.js:20 (Diff revision 3) > MESSAGE_ADD, > MESSAGES_CLEAR, > MESSAGE_OPEN, > MESSAGE_CLOSE, > MESSAGE_TYPE, > - MESSAGE_TABLE_RECEIVE, > + MESSAGE_TABLE_RECEIVE , shouldn't be removed ::: devtools/client/webconsole/new-console-output/actions/messages.js:98 (Diff revision 3) > module.exports = { > messageAdd, > messagesClear, > messageOpen, > messageClose, > - messageTableDataGet, > + messageTableDataGet , shouldn't be removed ::: devtools/client/webconsole/new-console-output/actions/ui.js:15 (Diff revision 3) > const Services = require("Services"); > > const { > FILTER_BAR_TOGGLE, > PREFS, > + TIMESTAMPS_TOGGLE Add , at end of line ::: devtools/client/webconsole/new-console-output/components/message.js:93 (Diff revision 3) > frame, > stacktrace, > serviceContainer, > dispatch, > exceptionDocURL, > + timeStamp = Date.now() Add , at end of line ::: devtools/client/webconsole/new-console-output/components/message.js:93 (Diff revision 3) > frame, > stacktrace, > serviceContainer, > dispatch, > exceptionDocURL, > + timeStamp = Date.now() Do we really want Date.now()? Shouldn't this be based on the timeStamp property from the message? ::: devtools/client/webconsole/new-console-output/reducers/messages.js:24 (Diff revision 3) > messagesTableDataById: Immutable.Map(), > // Map of the form {groupMessageId : groupArray}, > // where groupArray is the list of of all the parent groups' ids of the groupMessageId. > groupsById: Immutable.Map(), > // Message id of the current group (no corresponding console.groupEnd yet). > - currentGroup: null, > + currentGroup: null Add , at end of line ::: devtools/client/webconsole/new-console-output/reducers/ui.js:11 (Diff revision 3) > "use strict"; > > const { > FILTER_BAR_TOGGLE, > MESSAGE_ADD, > + TIMESTAMPS_TOGGLE Add , at end of line ::: devtools/client/webconsole/new-console-output/reducers/ui.js:19 (Diff revision 3) > > const UiState = Immutable.Record({ > filterBarVisible: false, > filteredMessageVisible: false, > autoscroll: true, > + timestampsVisible: true Add , at end of line ::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:97 (Diff revision 3) > + > + it("renders a timestamp", () => { > + const message = stubPreparedMessages.get("console.log('foobar', 'test')"); > + const wrapper = render(ConsoleApiCall({ message, serviceContainer })); > + > + expect(wrapper.find(".timestamp").text().length).toBe(12); Once we are using the actual timestamp instead of Date.now we could do a more specific assertion here (and in other tests) ::: devtools/client/webconsole/new-console-output/utils/messages.js:143 (Diff revision 3) > return new ConsoleMessage({ > source: MESSAGE_SOURCE.CONSOLE_API, > type: MESSAGE_TYPE.LOG, > level: MESSAGE_LEVEL.LOG, > messageText: "Navigated to " + message.url, > + timeStamp: message.timeStamp For the rest of this file (and the patch), please add a hanging , at the end of object literals ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_timestamps.js:23 (Diff revision 3) > > - hud = yield openConsole(); > + testPrefDefaults(outputEl); > - let panel = yield consoleOpened(); > > - yield onOptionsPanelSelected(panel); > - onPrefChanged(); > + let toolbox = gDevTools.getToolbox(hud.target); > + let optionsPanel = yield toolbox.selectTool("options"); Having to open up the options panel is unfortunate, and a result of using gDevTools.on('pref-changed') in webconsole.js. This'll be fixed by bug 1316630, but in the mean time we may as well use this I guess

Comment on attachment 8810120 [details] Bug 1307871 - Add new console frontend support for 'Enable timestamps' pref. https://reviewboard.mozilla.org/r/92524/#review92852 ::: devtools/client/themes/webconsole.css:177 (Diff revision 4) > padding-inline-start: 0; > margin-inline-start: 7px; > width: calc(100% - 7px); > } > > -#output-container.hideTimestamps > .message > .timestamp { > +.hideTimestamps > .message > .timestamp { I'd rather have a new rule in this file down by other new console styles for .webconsole-output.hideTimestamps { display: none; } to make it easier to remove this code in the future ::: devtools/client/webconsole/new-console-output/test/components/evaluation-result.test.js:91 (Diff revision 4) > + const message = stubPreparedMessages.get("new Date(0)"); > + const wrapper = render(EvaluationResult({ message })); > + const L10n = require("devtools/client/webconsole/new-console-output/test/fixtures/L10n"); > + const { timestampString } = new L10n(); > + > + console.log(message.timeStamp); Remove this console statement ::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser.ini:14 (Diff revision 4) > test-network-event.html > test-tempfile.css > test-tempfile.js > > [browser_webconsole_update_stubs_console_api.js] > -skip-if=true # This is only used to update stubs. It is not an actual test. > +# skip-if=true # This is only used to update stubs. It is not an actual test. These should still skip, right? ::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/head.js:55 (Diff revision 4) > from: existingPacket.from > }); > > // Clean root timestamp. > if(res.timestamp) { > - res.timestamp = existingPacket.timestamp; > + //res.timestamp = existingPacket.timestamp; These lines should be un commented out

Comment on attachment 8810120 [details] Bug 1307871 - Add new console frontend support for 'Enable timestamps' pref. https://reviewboard.mozilla.org/r/92524/#review93144 Thanks! ::: devtools/client/webconsole/new-console-output/components/console-output.js:130 (Diff revision 5) > messages: getAllMessages(state), > messagesUi: getAllMessagesUiById(state), > messagesTableData: getAllMessagesTableDataById(state), > autoscroll: getScrollSetting(state), > groups: getAllGroupsById(state), > + timestampsVisible: state.ui.timestampsVisible Nit: , at end. (yes, we need eslint for this)

Comment on attachment 8810936 [details] Bug 1307871 - Allow null frames object to prevent throwing when there aren't sources on the page; Wrong bug ID for review request

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5d62e2bc7b59 Add new console frontend support for 'Enable timestamps' pref. r=bgrins