Closed Bug 795264 Opened 12 years ago Closed 11 years ago

utils.saveScreenshot should be renamed because it saves dataURLs

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: xabolcs, Assigned: glennal.buford)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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]
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.
Attached patch patch (v1) (obsolete) — Splinter Review
Attachment #710004 - Flags: feedback?(hskupin)
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
Status: NEW → ASSIGNED
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-
Attached patch patch (v2)Splinter Review
D'oh.

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+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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 ;) ?
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.

Attachment

General

Creator:
Created:
Updated:
Size: