1470253 - console.timeStamp shouldn't output a message in the console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Daniel Lo Nigro (daniel15)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180605171542

Steps to reproduce:

Run console.timeStamp('Hello world')


Actual results:

"Hello world" is output to the console


Expected results:

"Hello world" should be added to performance profiler, and no console output.

See docs at https://developer.mozilla.org/en-US/docs/Web/API/Console/timeStamp:
> Adds a single marker to the browser's Performance or Waterfall tool. This lets you correlate a point in your code with the other events recorded in the timeline, such as layout and paint events.

This does not say anything about outputting to the console. Chrome does not output to the console on console.timeStamp calls.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I confirm the behavior from Comment 0.

Comment 2

[Saumya Balodi]

Hey, I am an outreachy applicant. Can I work on this issue if it is open?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Hello Saumya, of course you can work on this, thanks for offering help!

You can read http://docs.firefox-dev.tools/getting-started/ to setup the work environment. Make sure to choose Artifact builds when asked to, as it's much faster.

---

The fix itself will happen in devtools/client/webconsole/utils/messages.js and should be:

diff --git a/devtools/client/webconsole/utils/messages.js b/devtools/client/webconsole/utils/messages.js
--- a/devtools/client/webconsole/utils/messages.js
+++ b/devtools/client/webconsole/utils/messages.js
@@ -101,6 +101,9 @@ function transformConsoleAPICallPacket(p
         parameters = null;
       }
       break;
+    case "timeStamp":
+      type = MESSAGE_TYPE.NULL_MESSAGE;
+      break;
     case "time":
       parameters = null;
       if (timer && timer.error) {

Here we don't what "timeStamp" messages to be displayed at all, and we already have this case for other type of messages. What we need to do is set the type to MESSAGE_TYPE.NULL_MESSAGE, and they won't be displayed in the output.

The bulk of the work on this patch will be to add a test to make sure the message isn't displayed.
We might want to execute console.timeStamp("test") from the console, and make sure that the console output only contains 2 messages: :

-> console.timeStamp("test")
<- undefined

You can see how it differs from what we have currently

-> console.timeStamp("test")
   test
<- undefined

To add a test, we want to add a file with a descriptive name in devtools/client/webconsole/test/mochitest/, for example browser_webconsole_console_timeStamp, to be consistent with the other tests.
Then, we need to tell the test harness to run it. This is done by adding the test filename in devtools/client/webconsole/test/mochitest/browser.ini#293-294, between the 2 highlighted lines, because it should be alpha sorted.

Then, the test file should have this structure:

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

// Tests that a console.timeStamp() does not print anything in the console

"use strict";

const TEST_URI = "data:text/html,<meta charset=utf8>";

add_task(async function() {
  // We open the console and an empty tab, as we only want to evaluate something.
  const hud = await openNewTabAndConsole(TEST_URI);
  // We execute `console.timeStamp('test')` from the console input.
  await hud.jsterm.execute("console.timeStamp('test')");
  // TODO: We need to check that it did what we expect.
});

And after the // TODO part, we should check that we only have 2 messages, and that they have the expected content.

In order to do that, we can use the findMessages function, which is defined in devtools/client/webconsole/test/mochitest/head.js#250-267.

The first argument should be a reference to the webconsole (here, it will be hud), and the second one is a text we want the messages to match. Since we want all the messages, we can pass an empty string: findMessages(hud, "").

It will return an array of the HTML element messages.
So we need to :

• check the length of the array to be 2
• check that the first message is console.timeStamp("test") (we can use textContent to get the text of a HTML element. We also might need to call trim() on it, to remove any line ending character).
• check that the second message is undefined.

Feel free to ask any question, either here or on our Slack.

Comment 4

[Saumya Balodi]

Thank you for the detailed explanation, Nicolas. I have already set up the environment so I will start working on the issue straight away.

Comment 5

[Saumya Balodi]

Hey, I have fixed the bug and added the test file. On committing the changes it shows that the bug id is not valid. I wanted to confirm if the bug id is 1470253?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Hello Saumya 👋
Yes, this is the right bug number.
Is your commit like: Bug 1470253 - Don't show console.timeStamp() messages. r=nchevobbe.?
Phabricator expects this specific structure (Bug {bugId} - {description}. r={reviewer}).

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Also, I think I did not add the instruction to run the test, so if you did not find it yet, it would be ./mach test devtools/client/webconsole/test/mochitest/browser_webconsole_console_timeStamp.js

Comment 8

[Saumya Balodi]

I was adding the bug id in arc diff. But my commit message is not in this format so I will change that. I was able to find the command to run the test in the documentation and the result was successful. Thanks!

Comment 9

[Saumya Balodi]

Attachment: Bug 1470253 - Don't show console.timeStamp() messages. r=nchevobbe

Made changes to messages.js and added a test file.

Comment 10

[Saumya Balodi]

I am sorry I ended up making two commits by mistake. The first commit did not have a message in the format you specified so I made a new one with the correct format. Let me know if the second one works.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Yes, everything looked fine on Phabricator :) Thanks!

Comment 12

[Saumya Balodi]

Should I also add the changes suggested by reviewbot?

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Saumya Balodi from comment #12)

Should I also add the changes suggested by reviewbot?

yes, these are coding style that we enforce, and we can't land a patch if it has errors.
You can read about it in https://docs.firefox-dev.tools/contributing/eslint.html

Comment 14

[Saumya Balodi]

I have updated the commit with the required changes.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91686a9fe2bc
Don't show console.timeStamp() messages. r=nchevobbe

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/91686a9fe2bc