Export all data from Web Console

RESOLVED FIXED in Firefox 66

Status

defect
P3
normal
RESOLVED FIXED
8 years ago
3 months ago

People

(Reporter: rcampbell, Assigned: jefry.reyes, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-needed, parity-chrome})

unspecified
Firefox 66
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [console])

Attachments

(2 attachments, 8 obsolete attachments)

Reporter

Description

8 years ago
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.
Reporter

Updated

7 years ago
Component: Developer Tools → Developer Tools: Console
Reporter

Updated

6 years ago
Priority: -- → P3

Comment 1

4 years ago
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.
Duplicate of this bug: 1157200
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
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.
Mentor: nchevobbe
"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...
Whiteboard: [console] → [console][parity-chrome]
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [console][parity-chrome] → [console]

Updated

Last year
Product: Firefox → DevTools
Assignee

Updated

7 months ago
Assignee: nobody → jefry.reyes
Assignee

Comment 7

7 months ago
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?
Flags: needinfo?(nchevobbe)
Attachment #9029311 - Flags: feedback?(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 :)
Flags: needinfo?(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.
Attachment #9029311 - Flags: feedback?(nchevobbe) → feedback+
Assignee

Comment 10

6 months ago
Posted patch preliminary patch (obsolete) — Splinter Review
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
Attachment #9030161 - Flags: feedback?(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 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
Attachment #9030161 - Flags: feedback?(nchevobbe)
Assignee

Comment 13

6 months ago
I have uploaded the patch to copy the output to the clipboard. Am I on the right path?
Attachment #9030161 - Attachment is obsolete: true
Attachment #9031736 - Flags: feedback?(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
Attachment #9031736 - Flags: feedback?(nchevobbe) → feedback+
Assignee

Comment 15

6 months ago
Posted patch Bug644412-clipboard.patch (obsolete) — Splinter Review
Attachment #9031736 - Attachment is obsolete: true
Attachment #9033369 - Flags: review?(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.
Attachment #9033369 - Flags: review?(nchevobbe)
Assignee

Comment 17

5 months ago
Posted patch Bug644412-clipboard.patch (obsolete) — Splinter Review
Thanks for the review. I have introduced some improvements.
Attachment #9033369 - Attachment is obsolete: true
Attachment #9034366 - Flags: review?(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)
Attachment #9034366 - Flags: review?(nchevobbe)
Assignee

Comment 19

5 months ago
>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.
Flags: needinfo?(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.
Flags: needinfo?(nchevobbe)
Assignee

Comment 21

5 months ago
Posted patch Bug644412-clipboard.patch (obsolete) — Splinter Review
I have removed the obj and array from TEST_DATA but left the strings to facilitate changing the strings in future.
Attachment #9034366 - Attachment is obsolete: true
Attachment #9034387 - Flags: review?(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);
```
Attachment #9034387 - Flags: review?(nchevobbe)
Assignee

Comment 23

5 months ago
Posted patch Bug644412-clipboard.patch (obsolete) — Splinter Review
I have renamed the function but not passed hud.outputNode because it gives timeout.
Attachment #9034387 - Attachment is obsolete: true
Attachment #9034395 - Flags: review?(nchevobbe)
Attachment #9034395 - Flags: review?(nchevobbe) → review+
Assignee

Comment 25

5 months ago
This fixes the failing test introduced by the patch.
Attachment #9034429 - Flags: review?(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?
Attachment #9034429 - Flags: review?(nchevobbe)
Assignee

Comment 27

5 months ago
Posted patch Bug644412-clipboard.patch (obsolete) — Splinter Review

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.

Attachment #9034395 - Attachment is obsolete: true
Attachment #9034429 - Attachment is obsolete: true
Attachment #9036023 - Flags: review?(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.
Attachment #9036023 - Flags: review?(nchevobbe) → review+

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

Attachment #9036023 - Attachment is obsolete: true
Comment on attachment 9036308 [details] [diff] [review]
export-all.patch

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

_(carrying r+ from the previous patch)__
Attachment #9036308 - Flags: review+

Comment 32

5 months ago

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc9822da7de
Export all data from Web Console to clipboard. r=nchevobbe

Keywords: checkin-needed

Comment 33

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

This landed 🎉, thanks a lot Jefry!

Assignee

Comment 36

3 months ago

Thanks Nicolas!

You need to log in before you can comment on or make changes to this bug.