923940 - 'screenshot' command changes downloads dir

Status: VERIFIED FIXED

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

Description

[Gabriel Souza Franco [:gbs]]

The 'screenshot' command makes changes to the set path for downloads, solvable only by restarting the browser. As an example, running 'screenshot image.png' twice will cause it to try saving to the paths "C:\Users\<user>\Downloads\image.png" and "C:\Users\<user>\Downloads\image.png\image.png". I believe this is a regression introduced by Bug 875731.

Comment 1

[Gabriel Souza Franco [:gbs]]

Attachment: gcli-screenshot-fix.patch

Simple fix, assuming the |getPreferredDownloadsDirectory()| behavior is desired.
Also took the liberty to change the regexps tests to conform to https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style and to fix a escaping bug on the file extension test.

Comment 2

[Loic]

Regression range:
good=2013-09-25
bad=2013-09-26
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d279364a8b&tochange=e85b0372cece

Suspected bug: Bug 875731.

You need to ask someone to review your patch.

Comment 3

[:Paolo Amadini]

The solution here looks good.

Raymond, I'm wondering if we should just change the API to call "clone" ourselves, or even better return a path string in line with the other Download API methods (it seems to me that "OS.Path.join" would work as well in this particular use case).

We'll take care of further updating all callers later, if we implement either of those.

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to :Paolo Amadini from comment #3)
> The solution here looks good.
> 
> Raymond, I'm wondering if we should just change the API to call "clone"
> ourselves, or even better return a path string in line with the other
> Download API methods (it seems to me that "OS.Path.join" would work as well
> in this particular use case).
> 
> We'll take care of further updating all callers later, if we implement
> either of those.

Filed Bug 926736

Comment 5

[bhavana bajaj [:bajaj]]

IS this ready to land ?

Comment 6

[:Paolo Amadini]

(In reply to bhavana bajaj [:bajaj] from comment #5)
> IS this ready to land ?

This is being handled by bug 926736.

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to :Paolo Amadini from comment #6)
> (In reply to bhavana bajaj [:bajaj] from comment #5)
> > IS this ready to land ?
> 
> This is being handled by bug 926736.

Please verify that the patch for bug 926736 fixes this bug.

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to Gabriel Souza Franco from comment #0)
> The 'screenshot' command makes changes to the set path for downloads,
> solvable only by restarting the browser. As an example, running 'screenshot
> image.png' twice will cause it to try saving to the paths
> "C:\Users\<user>\Downloads\image.png" and
> "C:\Users\<user>\Downloads\image.png\image.png". I believe this is a
> regression introduced by Bug 875731.

Hi Gabriel, given bug 926736 has already landed, can you confirm this bug is fixed for you on our latest beta ?

Comment 9

[Gabriel Souza Franco [:gbs]]

Confirmed fix on Beta 27b2.
(Can't seem to mark bug FIXED myself.)

Comment 10

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0

Reproduced with Nightly from 2013-09-26.
Verified as fixed with Firefox 27 beta 4 (build ID: 20140106141415).