utils.saveScreenshot should be renamed because it saves dataURLs



6 years ago
2 years ago


(Reporter: xabolcs, Assigned: glennal.buford)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: [mentor=whimboo][lang=js][good first bug])


(1 attachment, 1 obsolete attachment)

3.84 KB, patch
: review+
Details | Diff | Splinter Review


6 years ago
As in a comment on GitHub [1], utils.saveScreenshot and the related wordings should have a rename.

xabolcs wrote on Thu, 13 Sep 2012 12:06:59 +0200:
> There was a talk in pull #88 [2], to update utils.js:saveScreenshot()'s
> description.
> I would like to propose (in a follow up bug) renaming of |saveScreenshot| 
> to |saveDataURL| or |saveDataURLToFile|.

After |utils.takeScreenshot| got splitted into |takeScreenshot| and 
|saveScreenshot| (Bug 761423), |saveScreenshot| has to have corresponding name to it's specification:

> /**
> * Takes a dataURL as input and saves it to the file name.jpg in the persisted screenshot path (or temporary directory)
> * Returns the filepath of the saved file
> */
> function saveScreenshot(aDataURL, aFilename, aCallback) {

[1] https://github.com/mozilla/mozmill/pull/94#discussion_r1595891
[2] https://github.com/mozilla/mozmill/pull/88
Whiteboard: [good first bug][lang=js] → [mentor=whimboo][lang=js][good first bug]

Comment 1

6 years ago
I'd like to take this bug. Which method name is preferred saveDataURL or saveDataURLToFile?
saveDataURL sounds fine to me. Also thanks for your interest in fixing it. If you need further information please join us on IRC in #automation.

Comment 3

6 years ago
Created attachment 710004 [details] [diff] [review]
patch (v1)
Attachment #710004 - Flags: feedback?(hskupin)

Comment 4

6 years ago
You have to rename the usages of |utils.saveScreenshot| too!

A quick search gives me the following:
- mozmill/mozmill/extension/resource/driver/controller.js
- mozmill/mozmill/extension/resource/stdlib/utils.js (utils.saveScreenshot itself)
- mutt/mutt/tests/js/utils/savescreenshot.js
- mutt/mutt/tests/js/utils/tests.ini

The last one included due to file naming, but there is no need to rename file savescreenshot.js, IMHO.
What do you think whimboo?
Assignee: nobody → glennal.buford
Flags: needinfo?(hskupin)
Correct. The test file should keep it's name because it really tests the screenshot feature.
Flags: needinfo?(hskupin)
Comment on attachment 710004 [details] [diff] [review]
patch (v1)

Review of attachment 710004 [details] [diff] [review]:

As what has already been mentioned we also have to update the consumers of that method.
Attachment #710004 - Flags: feedback?(hskupin) → feedback-

Comment 7

6 years ago
Created attachment 710241 [details] [diff] [review]
patch (v2)


Commit message: Bug 795264 - Rename saveScreenshot method to saveDataURL
Attachment #710004 - Attachment is obsolete: true
Attachment #710241 - Flags: feedback?(hskupin)
Comment on attachment 710241 [details] [diff] [review]
patch (v2)

Review of attachment 710241 [details] [diff] [review]:

Looks fine now. Thank you a lot, Glenna!
Attachment #710241 - Flags: feedback?(hskupin) → review+
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 9

6 years ago
Glenna, you haven't provided author information in your patches.

What kind of userinfo should Henrik use for the commit?
The Bugzilla one (Glenna <xxxxxxx@xxxx.com>) 
or the GitHub one (glenna <xxxxx@xxxx.net>) 
or an other one ;) ?

Comment 10

6 years ago
Please use the bugzilla one. Thanks :)
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.