644412 - Export all data from Web Console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Rob Campbell [:rc] (:robcee)]

Currently, the only way to export data from the web console is to select all and copy the lines to the clipboard for pasting into an external editor. We should provide an export mechanism that saves all lines from the console and any headers and responses (bodies if checked) in the file.

Comment 1

[Patrick Hall]

The recent addition of persistent console log history makes this option more desirable to me: as the console takes over more of my development time I find myself wanting to be able to export a complex history more and more. This has shown its worth in Python's `readline.write_history_file` as well.

Comment 3

[Sebastian Zartner [:sebo]]

Note that there was an extension for Firebug called ConsoleExport[1], which offered this feature. Also the Chrome DevTools have an export option for quite some time[2].

Sebastian

[1] https://github.com/firebug/consoleexport
[2] https://twitter.com/addyosmani/status/590646206491156482

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

This sounds interesting. 
What chrome seems to do is to output every message in a file, with the location first, separated by a line break. i.e. :

```
script-a.js mylog from script-a
script-b.js mylog from script-b
```

I guess we could build this upon our "Copy message" context menu entry.
The action item could be in the context menu entry as well as in the future Console settings panel.


As a first step, we could do this strictly as "Copy message" works, and then take care of putting the location at the beginning of the message in a follow-up.

If anyone wants to work on this, I would gladly mentor them.

Comment 5

[Stefan Fleiter (:sfleiter)]

"Copy (Message)" from the context menu is not expanding logged objects which are represented by {…}.
These should (at least optionally) be expanded recursively for such an export.

This is really limiting the possibility to send web application developers your log output when seeing a problem happen...

Comment 6

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 7

[Jefry Lagrange]

Attachment: Screenshot from 2018-12-01 16-17-41.png

I have been looking at this issue and I have some questions regarding this.

1- How to get a l10 string for "Save as"?

2- I added two functions to DevToolsUtils
-- DevToolsUtils.saveFileStream
-- DevToolsUtils.showFilePicker

One shows the file picker and another one saves the file. Is that alright?

I found some code with this same functionality. The code to save a css from the stylesheet editor. It is tightly coupled. Thinking I should refactor to make use of this so that we have a central way of showing file picker and saving files. What do you think about this?

Idea: I want also to give the option to the user to save the output as a styled html keeping the same style as the console. Maybe new feature request for this?


3- What do you think about the picture? Is that the right place?

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

1. You can have a look at how we do that for other strings (e.g. https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/devtools/client/webconsole/components/CollapseButton.js#11-12 ). The string need to exists in webconsole.properties (https://searchfox.org/mozilla-central/source/devtools/client/locales/en-US/webconsole.properties), so you'd need to create a new entry if we don't already have something for that (I suspect we don't, as we might want to have a label that looks like "Export console data" or similar)

2. I think it's okay. I don't know how we do things in the style editor, but having global purposed function there make sense. 

Also, it might make sense to only have "Export all data to clipboard" for people who don't want to create new file but simply scan the output or share it in an email.

> Idea: I want also to give the option to the user to save the output as a styled html keeping the same style as the console. Maybe new feature request for this?

Yes, that's something we're thinking about too. The goal would be to have a "share" button in the console, that would give you an URL you can send to people to help you make sense of what you are seeing in the console. That's a broader project because there are _many_ things to discuss until we can ship such thing. I can cc you on bugs we'll file on the future so you xan see what we are thinking of, and can give feedback or ideas, and be able to work on some part of it :)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9029311 [details]
Screenshot from 2018-12-01 16-17-41.png

Looks like the right place to me. The wording might be more explicit I think, but we can discuss it later.

Comment 10

[Jefry Lagrange]

Attachment: preliminary patch

I have written a preliminary patch to see if I'm on the right track. 

I'm having problems trying to write a test that involves the file picker. I think I should mock the filepicker in a mochitest using "SpecialPowers.MockFilePicker". But I don't know how to go about writing it all up.

I left some TODOs inside the patch. Please have look at those and let me know what could be done in those cases.

There's also the issue of the showfilepicker, which can be used to refactor some code in the style editor. However, we also have the option to simplify this code to make it only useful saving files (less use cases than now). I kind of leaning towards this option.

Note: I haven't changed the name from "Save as" to "Export console data". Will do that in the next version of the patch

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Hello Jefry, thanks for working on this.
We may want to work on a "Export to clipboard" function first as: 
- it simpler to write things to the clipboard
- we don't have to deal with the file picker
- it will show what challenges we need to address

we can either use this bug or create a new one to work on that.

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9030161 [details] [diff] [review]
preliminary patch

Review of attachment 9030161 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing flag as we will probably on export to clipboard first

Comment 13

[Jefry Lagrange]

Attachment: export console output to clipboard

I have uploaded the patch to copy the output to the clipboard. Am I on the right path?

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9031736 [details] [diff] [review]
export console output to clipboard

Review of attachment 9031736 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a nice start, thanks Jefry!
So we need:
- to have a localized string for "Export visible messages to clipboard"
- a test to ensure this works as expected

Comment 15

[Jefry Lagrange]

Attachment: Bug644412-clipboard.patch

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9033369 [details] [diff] [review]
Bug644412-clipboard.patch

Review of attachment 9033369 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot Jefry!
This looks almost good to land :)

There's only a few comments that need to be addressed, but after that, we should be good.

::: devtools/client/locales/en-US/webconsole.properties
@@ +191,5 @@
>  # opens the webconsole sidebar for the logged variable.
>  webconsole.menu.openInSidebar.label=Open in sidebar
>  webconsole.menu.openInSidebar.accesskey=V
>  
> +# LOCALIZATION NOTE (webconsole.menu.saveToFile.label)

Here it should be `webconsole.menu.exportClipboard.label`

@@ +193,5 @@
>  webconsole.menu.openInSidebar.accesskey=V
>  
> +# LOCALIZATION NOTE (webconsole.menu.saveToFile.label)
> +# Label used for a context-menu item displayed for object/variable logs. Clicking on it
> +# opens a file picker in order to save the full output of the console to a file.

this is not really what it does since we want to copy things in the clipboard :)

::: devtools/client/webconsole/test/mochitest/browser_webconsole_context_menu_export_console_output_clipboard.js
@@ +10,5 @@
> +
> +const TEST_URI = `data:text/html;charset=utf-8,<script>
> +  window.logStuff = function () {
> +    console.log("simple text message");
> +    console.log("second message test");

they are indeed simple :) 
Could we try to log multiple objects in the second message to have a more "real world" example (e.g. console.log("object", {a: 1}, "array", ["b", "c"])).

@@ +27,5 @@
> +    content.wrappedJSObject.logStuff();
> +  });
> +
> +  info("Test export to clipboard ");
> +  const message = await waitFor(() => findMessage(hud, "simple text message"));

I'd say let's wait until we have all the messages in the output (so wait until we have 2 messages)

@@ +32,5 @@
> +  const clipboardText = await copyMessageContent(hud, message);
> +  ok(true, "Clipboard text was found and saved");
> +
> +  info("Check if all messages where copied to clipboard");
> +  ok(clipboardText.includes(TEST_TEXT_MESSAGE),

I think we should test exactly what it gets us, to make sure we do have line breaks and spaces at the right places.

Comment 17

[Jefry Lagrange]

Attachment: Bug644412-clipboard.patch

Thanks for the review. I have introduced some improvements.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9034366 [details] [diff] [review]
Bug644412-clipboard.patch

Review of attachment 9034366 [details] [diff] [review]:
-----------------------------------------------------------------

tested it and it works well!

Let's fix the comments and push this to TRY to ensure it works fine everywhere.

::: devtools/client/locales/en-US/webconsole.properties
@@ +192,5 @@
>  webconsole.menu.openInSidebar.label=Open in sidebar
>  webconsole.menu.openInSidebar.accesskey=V
>  
> +# LOCALIZATION NOTE (webconsole.menu.exportClipboard.label)
> +# Label used for a context-menu item displayed for object/variable logs. Clicking on it

erf, sorry I missed something yesterday: 

> Label used for a context-menu item displayed for object/variable logs

should be 

"Label used for a context-menu item displayed on the output."

::: devtools/client/webconsole/test/mochitest/browser_webconsole_context_menu_export_console_output_clipboard.js
@@ +4,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const TEST_URI =

Let's change it to the following:

data:text/html,<meta charset=utf8><script>console.log("hello")</script>

Because it would be nice to test a message with a location as well.

Then, first, we should wait for the "hello" message to be displayed, and then do the other logs.

For testing this message, it should then be like:

```js
is(clipboardLines[0].trim(), `hello ${TEST_URI}:1`);
```

@@ +6,5 @@
> +"use strict";
> +
> +const TEST_URI =
> +"data:text/html,Test the export visible messages to clipboard of the webconsole";
> +const TEST_DATA = {

that was good thinking here, but I think we could directly inline it in the ContentTask.spawn as the messages are pretty simple.

@@ +29,5 @@
> +                                        "array:", testData.array);
> +  });
> +
> +  info("Test export to clipboard ");
> +  const message = await waitFor(() => findMessage(hud, TEST_DATA.msg));

we should wait until all messages are available. For example we could do `waitFor(() => findMessages(hud, "").length === 3)` (or 4, if we add the "hello" message)

Comment 19

[Jefry Lagrange]

>that was good thinking here, but I think we could directly inline it in the ContentTask.spawn as the messages are pretty >simple.

I don't understand very well. Are you referring to the TEST_DATA variable? Do you mean moving this var to the function that includes ContentTask.spawn? I use the messages in other places besides ContentTask.spawn for checking.

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

> I don't understand very well. Are you referring to the TEST_DATA variable? Do you mean moving this var to the function that includes ContentTask.spawn? I use the messages in other places besides ContentTask.spawn for checking.

I meant completely removing TEST_DATA, and using object, array and string literals when using content.console.log in ContentTask.spawn.

I understand it was used for checking the message content later too, but since we're only testing 2, relatively simple, strings, it might not be worth it.
If you feel like it's cleaner, we can keep it with the strings, but remove the object and array, which we don't use for checking.

Comment 21

[Jefry Lagrange]

Attachment: Bug644412-clipboard.patch

I have removed the obj and array from TEST_DATA but left the strings to facilitate changing the strings in future.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9034387 [details] [diff] [review]
Bug644412-clipboard.patch

Review of attachment 9034387 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry to find new things on each round, I should have done a better job from the start.
This time, I think we should be good after the changes that I ask for.

::: devtools/client/webconsole/test/mochitest/browser_webconsole_context_menu_export_console_output_clipboard.js
@@ +43,5 @@
> +    "found second text message in clipboard");
> +  is(clipboardLines[2].trim(), 'object: Object { a: 1 } array: Array [ "b", "c" ]',
> +    "found object and array in clipboard");
> +  const CLEAN_URI = TEST_URI.replace("text/html;charset=utf-8,", "");
> +  is(clipboardLines[3].trim(), `hello ${CLEAN_URI}:1:32`,

cool!

@@ +51,5 @@
> +/**
> + * Simple helper method to open the context menu on a given message, and click on the
> + * export visible messages to clipboard.
> + */
> +async function copyMessageContent(hud, message) {

missed that too previously: it should be name `exportAllToClipboard` or something similar.
Also, `openContextMenu` second argument is an element, not necessarily a message.
So here, we could remove `message`, and instead pass `hud.outputNode` to openContextMenu.

Then we can replace lines 34 and 35 by: 

```
const clipboardText = await copyMessageContent(hud);
```

Comment 23

[Jefry Lagrange]

Attachment: Bug644412-clipboard.patch

I have renamed the function but not passed hud.outputNode because it gives timeout.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Looks good! Let's see what TRY says: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd1fd69032290c24338b1e0aae95e9606ac868c

Comment 25

[Jefry Lagrange]

Attachment: Bug644412-clipboard-mochitest.patch

This fixes the failing test introduced by the patch.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9034429 [details] [diff] [review]
Bug644412-clipboard-mochitest.patch

Review of attachment 9034429 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Jefry
Do you think you could put these changes into the other patch?
I think one rule is that a single commit should not make the tests fail, so here it would be nice to put it in the same commit that changes the context menu.

::: devtools/client/webconsole/test/mochitest/browser_console_context_menu_entries.js
@@ +43,5 @@
>      "#console-menu-store (S) [disabled]",
>      "#console-menu-copy (C)",
>      "#console-menu-copy-object (o) [disabled]",
>      "#console-menu-select (A)",
> +  ], ["#console-menu-export-clipboard ()"]);

That feels a bit weird.
Do you think we could change this a bit?

For example, we could rename `addPrefBasedEntries` into `handlePrefBasedEntries`, and pref based entry would be represented as object instead of simple strings: 

```
   let expectedContextMenu = addPrefBasedEntries([
     "#console-menu-copy-url (a)",
     "#console-menu-open-url (T)",
     "#console-menu-store (S) [disabled]",
     "#console-menu-copy (C)",
     "#console-menu-copy-object (o) [disabled]",
     "#console-menu-select (A)",
     {
       entry: "#console-menu-open-sidebar (V) [disabled]", 
       pref: "devtools.webconsole.sidebarToggle"
     },
     "#console-menu-export-clipboard ()"
   ]);
```

Then in handlePrefBasedEntries, we would iterate over the array, and if an item is an object, check the associated preference, and if enabled, replace the object with entry.

Does that sound good?

Comment 27

[Jefry Lagrange]

Attachment: Bug644412-clipboard.patch

I managed to change the ordering so that for fixing the mochitest the only thing needed is to add the entry of our new option, no need to modify functions. It's the simplest solution, I think.

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9036023 [details] [diff] [review]
Bug644412-clipboard.patch

Review of attachment 9036023 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Jefry!
The patch looks good to me. I'll push it to TRY, and if everything is fine we can land it.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

obsolete TRY push

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Attachment: export-all.patch

Here's a patch that fixes the ESLint failures.
I pushed it to TRY , and if everything looks good, we'll push this.

Comment 31

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9036308 [details] [diff] [review]
export-all.patch

Review of attachment 9036308 [details] [diff] [review]:
-----------------------------------------------------------------

_(carrying r+ from the previous patch)__

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc9822da7de
Export all data from Web Console to clipboard. r=nchevobbe

Comment 33

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9cc9822da7de

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

This landed 🎉, thanks a lot Jefry!

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

Hey Jefry, people seem to like it 🙂 https://twitter.com/FirefoxDevTools/status/1111268359927144448

Comment 36

[Jefry Lagrange]

Thanks Nicolas!