Closed Bug 1307871 Opened 8 years ago Closed 8 years ago

Add new console frontend support for 'Enable timestamps' pref

Categories

(DevTools :: Console, enhancement, P2)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: linclark, Assigned: ntim)

References

Details

(Whiteboard: new-console)

Attachments

(1 file, 1 obsolete file)

Bug 1307871 - Add new console frontend support for 'Enable timestamps' pref.
58 bytes, text/x-review-board-request
bgrins
: review+
Details 
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
Priority: -- → P2
Whiteboard: new-console
Blocks: enable-new-console
Assignee: nobody → ntim.bugs
Summary: Add support for "activate timestamp" preference → Add new console frontend support for 'Enable timestamps' pref
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
Attachment #8810120 - Flags: review-
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
Attachment #8810120 - Flags: review?(bgrinstead)
Status: NEW → ASSIGNED
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
Attachment #8810120 - Flags: review?(bgrinstead)
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)
Attachment #8810120 - Flags: review?(bgrinstead) → review+
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
Attachment #8810936 - Attachment is obsolete: true
Attachment #8810936 - Flags: review?(jlong)
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
https://hg.mozilla.org/mozilla-central/rev/5d62e2bc7b59
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: