795243 - utils.saveScreenshot should be synchronous

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Szabolcs Hubai (:xabolcs)]

Regarding to the Bug 783223 related talk below, utils.js:saveScreenshot()
should handle the NetUtil callbacks and should be synchronous.

[ 9:57 ] Szabolcs: to clarify:
[ 9:57 ] Szabolcs: should I change the two expect.ok at L34 and L36?
[ 9:57 ] Szabolcs: and change back the assert.ok(file.exists(),...) to expect.ok() at L41?
[ 9:57 ] Szabolcs: https://github.com/mozilla/mozmill/compare/997a3cf6e71d#L1R34
[ 10:03 ] Henrik: when reading the code again i think 
[ 10:03 ] Henrik: we should make utils.saveScreenshot synchronous 
[ 10:03 ] Henrik: by moving the callback handing inside it. 
[ 10:03 ] Henrik: In that case your assert would be ok 
[ 10:03 ] Henrik: and we can remove the other two. 
[ 10:03 ] Henrik: i wouldn't mind to do it in a follow-up bug
[ 10:03 ] Henrik: this async stuff is adding a lot of complexity


Example handling code can be found in |MozMillController.prototype.screenshot| [1].


[1] https://github.com/mozilla/mozmill/blob/750a288ec1d7/mozmill/mozmill/extension/resource/driver/controller.js#L405

Comment 1

[Szabolcs Hubai (:xabolcs)]

Customers of utils.saveScreenshot need two information:
 - a failure bit: was the save succesful?
 - a filename: where is the file on the filesystem?


"filename" is returned as the result of utils.saveScreenshot, "failure" is transmitted with the sync() callback.

I propose that utils.saveScreenshot should return not just "filename", but also "failure". Storing this failure bit could be done in the |screenshot|.

And the calling of saveScreenshot would be:

> ...
>
> // Save screenshot to disk
> [ screenshot.filename, screenshot.failure ] = utils.saveScreenshot(screenshot.dataURL, name);
> if (screenshot.failure) {
>   broker.log({'function': 'controller.screenshot()',
>               'message': 'Error writing to file: ' + screenshot.filename});
>  } else {
> ...


Any toughts? 
Defining a new property to screenshot is allowed?
Returning more than one value is allowed?

Comment 2

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v1 - returning multiple values

Comment 3

[Szabolcs Hubai (:xabolcs)]

Comment on attachment 680890 [details] [diff] [review]
Patch v1 - returning multiple values

>diff --git a/mozmill/mozmill/extension/resource/stdlib/utils.js b/mozmill/mozmill/extension/resource/stdlib/utils.js
>--- a/mozmill/mozmill/extension/resource/stdlib/utils.js
>+++ b/mozmill/mozmill/extension/resource/stdlib/utils.js
>@@ -391,57 +391,68 @@ function takeScreenshot(node, highlights
>     }
>   }
> 
>   return canvas.toDataURL("image/jpeg", 0.5);
> }
> 
> /**
>  * Takes a canvas as input and saves it to the file tempdir/name.png
>- * Returns the filepath of the saved file
>+ * Returns the filepath of the saved file and the failure flag

Could somebody help me to provide a correct description here?


>  */
>-function saveScreenshot(aDataURL, aFilename, aCallback) {
>+function saveScreenshot(aDataURL, aFilename) {
>   const FILE_PERMISSIONS = parseInt("0644", 8);
> 
>   let file = Cc["@mozilla.org/file/directory_service;1"]
>              .getService(Ci.nsIProperties).get("TmpD", Ci.nsILocalFile);
>   file.append("mozmill_screenshots");
>   file.append(aFilename + ".jpg");
>   file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FILE_PERMISSIONS);
> 
>   // Create an output stream to write to file
>-  var foStream = Cc["@mozilla.org/network/file-output-stream;1"]
>+  let foStream = Cc["@mozilla.org/network/file-output-stream;1"]
>                  .createInstance(Ci.nsIFileOutputStream);
>   foStream.init(file, 0x02 | 0x08 | 0x10, FILE_PERMISSIONS, foStream.DEFER_OPEN);
> 
>-  var dataURI = NetUtil.newURI(aDataURL, "UTF8", null);
>+  let dataURI = NetUtil.newURI(aDataURL, "UTF8", null);
>   if (!dataURI.schemeIs("data")) {
>     throw TypeError("aDataURL parameter has to have 'data'" +
>                     " scheme instead of '" + dataURI.scheme + "'");
>   }
> 
>   // Write asynchronously to buffer;
>   // Input and output streams are closed after write
>+

Extra newline is a NIT?


>+  let ready = false;
>+  let failure = false;
>+
>+  function sync(status) {
>+    if (!Components.isSuccessCode(status)) {
>+      failure = true;
>+    }
>+    ready = true;
>+  }
>+
>   NetUtil.asyncFetch(dataURI, function (aInputStream, aAsyncFetchResult) {
>     if (!Components.isSuccessCode(aAsyncFetchResult)) {
>         // An error occurred!
>-        if (typeof(aCallback) === "function") {
>-          aCallback(aAsyncFetchResult);
>-        }
>+        sync(aAsyncFetchResult);
>     } else {
>       // Consume the input stream.
>       NetUtil.asyncCopy(aInputStream, foStream, function (aAsyncCopyResult) {
>-        if (typeof(aCallback) === "function") {
>-          aCallback(aAsyncCopyResult);
>-        }
>+        sync(aAsyncCopyResult);
>       });
>     }
>   });
> 
>-  return file.path;
>+  waitFor(function () {
>+    return ready;
>+  }, "Screenshot '" + file.path + "' has been saved.");
>+
>+  return [file.path, failure];
> }
> 
> /**
>  * Some very brain-dead timer functions useful for performance optimizations
>  * This is only enabled in debug mode
>  *
>  **/
> var gutility_mzmltimer = 0;

Comment 4

[Szabolcs Hubai (:xabolcs)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #2)
> Created attachment 680890 [details] [diff] [review]
> Patch v1 - returning multiple values

Forget to note the development branch:
https://github.com/xabolcs/mozmill/compare/branch-bug-810627-radiobutton-instantapply

Comment 5

[Szabolcs Hubai (:xabolcs)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #4)
> (In reply to Szabolcs Hubai (:xabolcs) from comment #2)
> > Created attachment 680890 [details] [diff] [review]
> > Patch v1 - returning multiple values
> 
> Forget to note the development branch:
> https://github.com/xabolcs/mozmill/compare/branch-bug-810627-radiobutton-
> instantapply

Ooops!

This is the real one:
https://github.com/xabolcs/mozmill/compare/branch-bug-795243-savescreenshot-sync

Comment 6

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

Comment on attachment 680890 [details] [diff] [review]
Patch v1 - returning multiple values

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

This patch looks fine. I only have some things to comment on which we should agree on and get fixed.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +395,5 @@
>  }
>  
>  /**
>   * Takes a canvas as input and saves it to the file tempdir/name.png
> + * Returns the filepath of the saved file and the failure flag

While touching the comment please make it a real jsdoc one.

@@ +423,5 @@
> +
> +  let ready = false;
> +  let failure = false;
> +
> +  function sync(status) {

nit: aStatus please.

@@ +444,5 @@
>    });
>  
> +  waitFor(function () {
> +    return ready;
> +  }, "Screenshot '" + file.path + "' has been saved.");

This message is misleading if we encountered a failure.

@@ +446,5 @@
> +  waitFor(function () {
> +    return ready;
> +  }, "Screenshot '" + file.path + "' has been saved.");
> +
> +  return [file.path, failure];

I wouldn't make this an array but a hash.

::: mutt/mutt/tests/js/utils/savescreenshot.js
@@ +11,5 @@
>   */
>  var testScreenshotSaveCorruption = function() {
>    const name = "smile5x5";
>  
>    const smile5x5DataURL = "data:image/jpeg;base64," + 

nit: please fix the trailing white-space.

Comment 7

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 680890 [details] [diff] [review]
> Patch v1 - returning multiple values
> 
> Review of attachment 680890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks fine. I only have some things to comment on which we should
> agree on and get fixed.

Thank you for the f+.
Please read my questions inline!


> 
> ::: mozmill/mozmill/extension/resource/stdlib/utils.js
> @@ +395,5 @@
> >  }
> >  
> >  /**
> >   * Takes a canvas as input and saves it to the file tempdir/name.png
> > + * Returns the filepath of the saved file and the failure flag
> 
> While touching the comment please make it a real jsdoc one.
> 


For example:
>  /**
>   * Saves a dataURL to the file tempdir/name.jpg
>   *
>   * @param {string} aDataURL
>   *        The dataURL to save
>   * @param {string} aFilename
>   *        A desired filename, without extension part
>   *
>   * @returns {sorry, what?} The hash containing the path of saved file, and the faillure bit
>   */

Can I ask for help about wording this result = path + failure bit stuff?
Thank you!


> @@ +423,5 @@
> > +
> > +  let ready = false;
> > +  let failure = false;
> > +
> > +  function sync(status) {
> 
> nit: aStatus please.
> 
> @@ +444,5 @@
> >    });
> >  
> > +  waitFor(function () {
> > +    return ready;
> > +  }, "Screenshot '" + file.path + "' has been saved.");
> 
> This message is misleading if we encountered a failure.
> 

E.g.: "Saving dataURL to '/path/to/tmp/something.jpg' has been failed."?


> @@ +446,5 @@
> > +  waitFor(function () {
> > +    return ready;
> > +  }, "Screenshot '" + file.path + "' has been saved.");
> > +
> > +  return [file.path, failure];
> 
> I wouldn't make this an array but a hash.
> 

o.O
How do you define hash in javascript?
Searching for hash on MDN didn't showed me any js-related page. :(
Are you talking about simple objects?
> let result = {path: ...., failure: false};

Or should I use Dict.jsm[1]? That introduced in Gecko 5.

Sorry, I don't know what hash means (in js context of course).



https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Dict.jsm

> ::: mutt/mutt/tests/js/utils/savescreenshot.js
> @@ +11,5 @@
> >   */
> >  var testScreenshotSaveCorruption = function() {
> >    const name = "smile5x5";
> >  
> >    const smile5x5DataURL = "data:image/jpeg;base64," + 
> 
> nit: please fix the trailing white-space.

Comment 8

[Szabolcs Hubai (:xabolcs)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #7)
> (In reply to Henrik Skupin (:whimboo) from comment #6)
> > Comment on attachment 680890 [details] [diff] [review]
> > @@ +446,5 @@
> > > +  waitFor(function () {
> > > +    return ready;
> > > +  }, "Screenshot '" + file.path + "' has been saved.");
> > > +
> > > +  return [file.path, failure];
> > 
> > I wouldn't make this an array but a hash.
> > 
> 
> o.O
> How do you define hash in javascript?
> Searching for hash on MDN didn't showed me any js-related page. :(
> Are you talking about simple objects?
> > let result = {path: ...., failure: false};
> 
> Or should I use Dict.jsm[1]? That introduced in Gecko 5.
> 
> Sorry, I don't know what hash means (in js context of course).
> 
> 
> 
> https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Dict.jsm
> 

MXR helped me, and looks like a hash is a simple object:
> let result = {};
> ...
> result["path"] = "/tmp/randomfilename.jpg";
> ...
> result["failure"] = false;
> ...
> return result;

Comment 9

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

(In reply to Szabolcs Hubai (:xabolcs) from comment #7)
> >   * Saves a dataURL to the file tempdir/name.jpg

You can use something like 'Save the dataURL content to the specified file. It will be stored in the temporary folder.

> >   * @param {string} aFilename
> >   *        A desired filename, without extension part

Target file name without extension

> >   * @returns {sorry, what?} The hash containing the path of saved file, and the faillure bit

In this case it's an object we return.

> > > +  waitFor(function () {
> > > +    return ready;
> > > +  }, "Screenshot '" + file.path + "' has been saved.");
> > 
> > This message is misleading if we encountered a failure.
> 
> E.g.: "Saving dataURL to '/path/to/tmp/something.jpg' has been failed."?

No, because we are waiting for the ready flag here, which will happen in case of success and failure. The case we do not cover is ready==true and failure==true. Probably we should enclose it in a try/catch and re-throw with better information? 

> > @@ +446,5 @@
> > > +  waitFor(function () {
> > > +    return ready;
> > > +  }, "Screenshot '" + file.path + "' has been saved.");
> > > +
> > > +  return [file.path, failure];
> > 
> > I wouldn't make this an array but a hash.
> > 
> 
> o.O
> How do you define hash in javascript?
> Searching for hash on MDN didn't showed me any js-related page. :(
> Are you talking about simple objects?
> > let result = {path: ...., failure: false};

Yes, exactly that. And you could directly return without creating a temporary variable.

> Or should I use Dict.jsm[1]? That introduced in Gecko 5.
> https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Dict.jsm

Nice!  I have never heard of it. It seems to be kinda useful. I will remember it. Thanks!

Comment 10

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v2 - returning hash

Addressed comments:
- jsdoced saveScreenshot()
- saveScreenshot returns hash
- removed white space from test dataURL
- use aStatus in sync() helper function
- use not too misleading message in waitFor()


Partial testrun:
> Running JS Tests
> TEST-START | waitfor.js | setupModule
> TEST-START | waitfor.js | testWaitForCallback
> TEST-PASS | waitfor.js | testWaitForCallback
> TEST-START | waitfor.js | testWaitForFalseAfterTrue
> TEST-PASS | waitfor.js | testWaitForFalseAfterTrue
> TEST-START | waitfor.js | testWaitForCallbackCounter
> TEST-PASS | waitfor.js | testWaitForCallbackCounter
> TEST-START | waitfor.js | testWaitForTimeoutAccuracy
> TEST-PASS | waitfor.js | testWaitForTimeoutAccuracy
> TEST-END | waitfor.js | finished in 3219ms
> TEST-START | pageload.js | setupModule
> TEST-START | pageload.js | testWaitForPageLoad
> TEST-PASS | pageload.js | testWaitForPageLoad
> TEST-END | pageload.js | finished in 8114ms
> TEST-START | savescreenshot.js | testScreenshotSaveCorruption
> TEST-PASS | savescreenshot.js | testScreenshotSaveCorruption
> TEST-END | savescreenshot.js | finished in 105ms
> All tests were successful.  Ship it!
> 


Henrik!
If you find this patch r+ then please append commit message with " r=..."!
From aadced721cc9 [1] it was left out unfortunately.


[1]: https://github.com/mozilla/mozmill/commit/aadced721cc9

Comment 11

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

Comment on attachment 685829 [details] [diff] [review]
Patch v2 - returning hash

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

First sorry, that this review was so long in the queue. It totally came under the wire. :( Everything in here looks great now. Thanks a lot Szabolcs for your work. I will make sure to get this pushed to master now.

Comment 12

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

https://github.com/mozilla/mozmill/commit/de522004cadc2ec79c67e4510a58fefc3f9df77a