Closed Bug 1472673 Opened 5 years ago Closed 5 years ago

Refactor screenshot to use modern download and clipboard actions

Categories

(DevTools :: Console, enhancement, P3)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: yulia, Assigned: yulia)

References

Details

Attachments

(2 files)

Bug 1472673 - use simpler download and clipboard methods;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1472673 - fix windows screenshot file test, add more console screenshot command tests;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
The screenshot implemented in Bug 1464461 uses a deprecated api to interact with the Download component. The clipboard action is also a little strange and can be improved. More tests should also be added
Priority: -- → P3
Assignee: nobody → ystartsev
Comment on attachment 8990363 [details]
Bug 1472673 - use simpler download and clipboard methods;

https://reviewboard.mozilla.org/r/255424/#review262458

Whaa that's a drastic simplication!
Works great here on linux, thanks for cleaning this up.

::: devtools/shared/webconsole/screenshot-helper.js:180
(Diff revision 1)
>   *
>   * @param object window
>   *        The Debugger Client window.
>   *
>   * @param string data
>   *        The image data encoded in base64 that was sent from the server.

variable names and doc could be slightly tweaked.
`data` is a "base64 data URI" and not just raw base64 data.
So `data` may be better named `url` while `newData` can be named `data`, `b64Data` or `base64Data`.

::: devtools/shared/webconsole/screenshot-helper.js:245
(Diff revision 1)
> -                              null,
> -                              targetFileURI,
> -                              isPrivate);
> -
>    try {
>      // Await successful completion of the save via the listener

Do we still wait for full completion of the download or just wait for it to be started and correctly handled by the download manager?
If we only wait for download start, we should update the comment here and may be acknowledge that in the test. 
We are only testing destination file creation in the test, so may be we are fine here. createDownload may at least create the file. But I can see things going wrong when we try to remove the file while the download is still in process.
Attachment #8990363 - Flags: review?(poirot.alex) → review+
(In reply to Yulia Startsev [:yulia] from comment #0)
> More tests should also be added

Do you still plan to add a test?
Would this patch fix try failure on windows?
Comment on attachment 8990363 [details]
Bug 1472673 - use simpler download and clipboard methods;

https://reviewboard.mozilla.org/r/255424/#review262458

> Do we still wait for full completion of the download or just wait for it to be started and correctly handled by the download manager?
> If we only wait for download start, we should update the comment here and may be acknowledge that in the test. 
> We are only testing destination file creation in the test, so may be we are fine here. createDownload may at least create the file. But I can see things going wrong when we try to remove the file while the download is still in process.

I changed it so that we wait for it to finish, thanks for catching that
here is a try run with windows included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1e5cd08a5997938c5b844a3010144cf17a9d6fa

one extra test is still coming, most of the others were taken care of
Windows test now works; plus the test requested by nchevobbe is now there. These were the two unfinished tasks in 1464461

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b68a0fa9988d0a93dc1f48223444bde4b6a65278
Comment on attachment 8990955 [details]
Bug 1472673 - fix windows screenshot file test, add more console screenshot command tests;

https://reviewboard.mozilla.org/r/255946/#review262810

Thanks a lot yulia !
Everything looks good to me, except the test doesn't pass as is, but only because of a typo in the test url.

::: commit-message-66b86:1
(Diff revision 1)
> +Bug 1472673 - fix windows test, add more tests; r=nchevobbe

nit: we could specify that this is about console screenshot command tests

::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_user.js:8
(Diff revision 1)
> +
> +// Test that screenshot command works properly
> +
> +"use strict";
> +
> +const TEST_URI = ":text/html,<meta charset=utf8>" +

the test fails if I don't append \`data\` in front the \`:text/html\`

I find it nice to use ` for those data url so it's a bit more readable (and no need to escape quotes) like: 

```js
const TEST_URI = `data:text/html,<meta charset=utf8><script>
  function screenshot() {
    console.log("content defined screenshot")
  }</script>`;
```

::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_user.js:25
(Diff revision 1)
> +
> +async function testCommand(hud) {
> +  const command = `:screenshot --clipboard`;
> +  const onMessage = waitForMessage(hud, "Screenshot copied to clipboard.");
> +  hud.jsterm.execute(command);
> +  await onMessage;

nit: maybe we can add a simple descriptive ok at the end to indicate everything went fine,

```js
ok(true, ":screenshot was executed as expected");
```

::: devtools/client/webconsole/test/mochitest/browser_jsterm_screenshot_command_user.js:34
(Diff revision 1)
> +// command should not overwrite the screenshot function
> +async function testUserScreenshotFunction(hud) {
> +  const command = `screenshot()`;
> +  const onMessage = waitForMessage(hud, "screen");
> +  hud.jsterm.execute(command);
> +  await onMessage;

nit: maybe we can add a simple descriptive ok at the end to indicate everything went fine,

```js
ok(true, "content screenshot function is not overidden and was executed as expected");
```
Attachment #8990955 - Flags: review?(nchevobbe) → review+
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6a72be09112
use simpler download and clipboard methods; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/639574c8a863
fix windows screenshot file test, add more console screenshot command tests; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/b6a72be09112
https://hg.mozilla.org/mozilla-central/rev/639574c8a863
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.