Closed Bug 1197611 Opened 4 years ago Closed 4 years ago

screenshot command saves only to the default directory

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P2)

40 Branch
defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: mark, Assigned: jryans)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150812163655

Steps to reproduce:

I use screenshot command to save a screenshot file. I do not want to use the default location. That's why I gave the full path to the file to be save. For example,
screenshot /Users/partypics/admin-list.png --selector #wpbody

This used to be working perfectly until Firefox 40.0.2


Actual results:

screenshot command saves the file to the default download location replacing "/" with "_". For example,
_Users_partypics_admin-list.png


Expected results:

It should have saved the file to the correct location instead of the default location.
I've managed to reproduce this on the latest release(42.0) and latest Nightly(45.0a1)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151029151421

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151126030226

On 39.0 release, screenshots are save to the specified path.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Considering this, I will mark this bug as New and assign the appropriate component.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Graphic Commandline and Toolbar
Ever confirmed: true
Product: Core → Firefox
Priority: -- → P2
Whiteboard: [btpp-fix-later]
The started happening because we changed to the "saveURL" function to get the standard download icon pulse, track in downloads manager, etc.

We may need a different mechanism to restore full path control.
To clarify, "saveURL" sanitizes the input path so that it can't leave the user's chosen Downloads folder.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Blocks: 1244079
In addition to this, we now have access to the final path the file is saved to,
so we can correct the image onclick handler to correctly reveal the new file.

Review commit: https://reviewboard.mozilla.org/r/36021/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36021/
Attachment #8722410 - Flags: review?(pbrosset)
Comment on attachment 8722410 [details]
MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere. r=pbro,jwalker

https://reviewboard.mozilla.org/r/36021/#review32659

I've had some problems on Windows:

If I enter `screenshot C:\Users\Patrick\Desktop\test.png`
Then it *looks* as if everything's fine, the download icon animates, and a preview of the image is shown next to the command line. 
But, the image is nowhere to be found and:
- the download button menu shows an entry for a file oddly named Desktopest.png with the message "Failed - Data Resource"
- clicking the image preview doesn't do anything
- the browser console has the following errors:

```
Win error 123 during operation remove on file C:\Users\Patrick\Desktop	est.png (The filename, directory name, or volume label syntax is incorrect.)
 DownloadCore.jsm:2520:0
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource:///modules/DownloadsCommon.jsm :: onDownloadChanged :: line 737"  data: no]
```

The command `screenshot` works perfectly though.
I haven't tried on Linux or Mac.

::: devtools/shared/gcli/commands/screenshot.js:436
(Diff revision 1)
> - * Save the screenshot data to disk, returning a promise which
> + * Progress listener that forwards calls to a transfer object.

The DownloadListener class is pretty cryptic for someone not used to this kind of code. Weird nsI things, and services, and Cr.NS_ERROR_WHATEVER, ...
I feel like we need a better comment that explains in a bit more details what this is, and why it is needed.

::: devtools/shared/gcli/commands/screenshot.js:543
(Diff revision 1)
> +    } catch (ex) {
>        console.error(ex);
> -      reply.destinations.push(l10n.lookup("screenshotErrorSavingToFile") + " " + filename);
> +      reply.destinations.push(l10n.lookup("screenshotErrorSavingToFile") + " " +
> +                              reply.filename);

I assume that before your changes, the try/catch would catch errors from window.saveURL. Now that this is gone, do we still want to handle errors this way? Do you know what can potentially fail in the code in the try part? Maybe we could have a short try/catch only around the part(s) were we want to silence errors?
Attachment #8722410 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6)
> Comment on attachment 8722410 [details]
> MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere.
> r=pbro
> 
> https://reviewboard.mozilla.org/r/36021/#review32659
> 
> I've had some problems on Windows:
> 
> If I enter `screenshot C:\Users\Patrick\Desktop\test.png`
> Then it *looks* as if everything's fine, the download icon animates, and a
> preview of the image is shown next to the command line. 
> But, the image is nowhere to be found and:
> - the download button menu shows an entry for a file oddly named
> Desktopest.png with the message "Failed - Data Resource"
> - clicking the image preview doesn't do anything
> - the browser console has the following errors:
> 
> ```
> Win error 123 during operation remove on file C:\Users\Patrick\Desktop
> est.png (The filename, directory name, or volume label syntax is incorrect.)
>  DownloadCore.jsm:2520:0
> [Exception... "Component returned failure code: 0x80070057
> (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]" 
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> resource:///modules/DownloadsCommon.jsm :: onDownloadChanged :: line 737" 
> data: no]
> ```
> 
> The command `screenshot` works perfectly though.
> I haven't tried on Linux or Mac.

Took me a bit to realize what was happening, but GCLI is being too clever here...  It was trying to convert "\t" from "\test.png" into a tab character. :/

Joe, is there a real use case that expects this special character parsing[1]?  If there is, I could add an option so it can at least be ignored for file path args.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/source/lib/gcli/types/string.js#73-87
Flags: needinfo?(jwalker)
The 'right' answer is that what you have there is a 'file' [1] and not a 'string'.

Now that said, I'm not sure that the file type is working properly. I don't think I ever finished the back end, but it might be worth a quick test to see if removing anything to do with 'getPredictor' from [2] would make it work for you.

That way we could change the screenshot command [3] to look like this:

const filenameParam = {
  name: "filename",
  type: {
    name: "file",
    filetype: "file",
    existing: "no",
  },
  defaultValue: FILENAME_DEFAULT_VALUE,
  description: l10n.lookup("screenshotFilenameDesc"),
  manual: l10n.lookup("screenshotFilenameManual")
};

If that doesn't work, then lets hack the string type.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/source/lib/gcli/types/file.js
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/source/lib/gcli/util/fileparser.js
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/screenshot.js#36
Flags: needinfo?(jwalker)
(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #8)
> The 'right' answer is that what you have there is a 'file' [1] and not a
> 'string'.
> 
> Now that said, I'm not sure that the file type is working properly. I don't
> think I ever finished the back end, but it might be worth a quick test to
> see if removing anything to do with 'getPredictor' from [2] would make it
> work for you.

Great, this seems to work well as long as I disable the fileparser predictions.
https://reviewboard.mozilla.org/r/36021/#review32659

> The DownloadListener class is pretty cryptic for someone not used to this kind of code. Weird nsI things, and services, and Cr.NS_ERROR_WHATEVER, ...
> I feel like we need a better comment that explains in a bit more details what this is, and why it is needed.

Okay, I've tried to add some more comments.

> I assume that before your changes, the try/catch would catch errors from window.saveURL. Now that this is gone, do we still want to handle errors this way? Do you know what can potentially fail in the code in the try part? Maybe we could have a short try/catch only around the part(s) were we want to silence errors?

I am glad you asked!  In fact, none of this code appears to throw errors, even if you provide complete nonsense as a path.

I have added even more XPCOM fun so we can detect failure and show an error.
Attachment #8722410 - Attachment description: MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere. r=pbro → MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere. r=pbro,jwalker
Attachment #8722410 - Flags: review?(jwalker)
Comment on attachment 8722410 [details]
MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere. r=pbro,jwalker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36021/diff/1-2/
Attachment #8722410 - Flags: review?(pbrosset)
Comment on attachment 8722410 [details]
MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere. r=pbro,jwalker

https://reviewboard.mozilla.org/r/36021/#review33949

::: devtools/shared/gcli/commands/screenshot.js:468
(Diff revision 2)
> +      this[name] = makeClosure(name);

Is there a reason we didn't just do:

    this[name] = (...args) => transfer[name].apply(transfer, args);

::: devtools/shared/gcli/source/lib/gcli/util/fileparser.js:108
(Diff revision 2)
> -exports.supportsPredictions = true;
> +exports.supportsPredictions = false;

Woah! That was easy.
Attachment #8722410 - Flags: review?(jwalker) → review+
Comment on attachment 8722410 [details]
MozReview Request: Bug 1197611 - Allow GCLI screenshot to save anywhere. r=pbro,jwalker

https://reviewboard.mozilla.org/r/36021/#review34101

Works for me now on windows. Thanks.

::: devtools/shared/gcli/commands/screenshot.js:444
(Diff revision 2)
> + * nsIWebBrowserPersist object that does the actually saving to the nsITransfer

s/the actually/the actual

::: devtools/shared/gcli/commands/screenshot.js:518
(Diff revision 2)
>  function saveToFile(context, reply) {
>    return Task.spawn(function*() {

nit: Maybe make saveToFile a Task.async?
```
var saveToFile = Task.async(function*(context, reply) {

});
```
Attachment #8722410 - Flags: review?(pbrosset) → review+
https://reviewboard.mozilla.org/r/36021/#review33949

> Is there a reason we didn't just do:
> 
>     this[name] = (...args) => transfer[name].apply(transfer, args);

No reason, I just copied it from contentAreaUtils.js like this.  I'll update with your version.
https://reviewboard.mozilla.org/r/36021/#review34101

> s/the actually/the actual

Fixed.

> nit: Maybe make saveToFile a Task.async?
> ```
> var saveToFile = Task.async(function*(context, reply) {
> 
> });
> ```

Fixed.
https://hg.mozilla.org/mozilla-central/rev/415ead8adb8a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.