795264 - utils.saveScreenshot should be renamed because it saves dataURLs

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Szabolcs Hubai (:xabolcs)]

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

Comment 1

[Glenna]

I'd like to take this bug. Which method name is preferred saveDataURL or saveDataURLToFile?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

[Glenna]

Attachment: patch (v1)

Comment 4

[Szabolcs Hubai (:xabolcs)]

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?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Correct. The test file should keep it's name because it really tests the screenshot feature.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 7

[Glenna]

Attachment: patch (v2)

D'oh.

Commit message: Bug 795264 - Rename saveScreenshot method to saveDataURL

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 710241 [details] [diff] [review]
patch (v2)

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

Looks fine now. Thank you a lot, Glenna!

Comment 9

[Szabolcs Hubai (:xabolcs)]

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

What kind of userinfo should Henrik use for the commit?
The Bugzilla one (Glenna <[e-mail]>) 
or the GitHub one (glenna <[e-mail]>) 
or an other one ;) ?

Comment 10

[Glenna]

Please use the bugzilla one. Thanks :)

Comment 11

[Szabolcs Hubai (:xabolcs)]

FTR:
https://github.com/mozilla/mozmill/commit/f5d7d0422c0a3a8d988acbcd34d4e0bbb48f3805