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)
Testing Graveyard
Mozmill
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)
3.84 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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
Updated•12 years ago
|
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?
Comment 2•11 years ago
|
||
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.
Attachment #710004 -
Flags: feedback?(hskupin)
Reporter | ||
Comment 4•11 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
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
Comment 5•11 years ago
|
||
Correct. The test file should keep it's name because it really tests the screenshot feature.
Flags: needinfo?(hskupin)
Comment 6•11 years ago
|
||
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-
D'oh. Commit message: Bug 795264 - Rename saveScreenshot method to saveDataURL
Attachment #710004 -
Attachment is obsolete: true
Attachment #710241 -
Flags: feedback?(hskupin)
Comment 8•11 years ago
|
||
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+
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 9•11 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 ;) ?
Assignee | ||
Comment 10•11 years ago
|
||
Please use the bugzilla one. Thanks :)
Reporter | ||
Comment 11•11 years ago
|
||
FTR: https://github.com/mozilla/mozmill/commit/f5d7d0422c0a3a8d988acbcd34d4e0bbb48f3805
Updated•8 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•