892889 - Provide api for assigning photos to contacts

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[Samuel Müller]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212




Expected results:

There is no unified api to easily assign a photo to a contact.
The implementation is currently under way as specified in https://bug691141.bugzilla.mozilla.org/attachment.cgi?id=567890 except that it contains no UI stuff. It provides optional callback functions for error, progress and success states.

Comment 1

[Samuel Müller]

Attachment: patch providing functionality for async downloading/copying/verifying/downscaling of contact photos.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 775333 [details] [diff] [review]
patch providing functionality for async downloading/copying/verifying/downscaling of contact photos.

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

Hey Samuel,

I really like the idea here. Making things more asynchronous is always good for the end-user experience (although it does tend to make the code a bit more complex in the end).

I also really think we should write tests for this new object - but as that would almost certainly be a long and drawn out process, I'd be willing to let this land with manual testing and to write tests after the fact (but it must be done!).

-Mike

::: mail/components/addrbook/content/abCommon.js
@@ +912,5 @@
>    } while (newFile.exists());
>    return newFile;
>  }
> +
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");

If these are only used within gImageDownloader, and you've gone so far as to not dirty the global scope with stuff within gImageDownloader, we should probably only import these things within gImageDownloader.

The common pattern is:

let gImageDownloader = (function() {
  let scope = {};
  Components.utils.import("resource://gre/modules/FileUtils.jsm", scope)
  Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm", scope);

  let FileUtils = scope.FileUtils;
  let PrivateBrowsingUtils = scope.PrivateBrowsingUtils;

  //... etc
})();

@@ +915,5 @@
> +
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +
> +// Public self-contained object for image transfers.

"Header" documentation should be in block form, so

/**
 * This is a block comment describing what this object does.
 */

@@ +918,5 @@
> +
> +// Public self-contained object for image transfers.
> +// Responsible for file transfer, validating the image and downscaling.
> +// Attention: It is the responsibility of the caller to remove the old photo and update the card!
> +var gImageDownloader = (function() {

let, not var. This applies to every usage of var in this code.

@@ +924,5 @@
> +
> +  var downloadInProgress = false;
> +
> +  // nsIWebBrowserPersist is used here to be able to obtain progress reports for file transfers.
> +  const WebBrowserPersist = Components.Constructor("@mozilla.org/embedding/browser/nsWebBrowserPersist;1",

> 80 chars - please find a way of nicely breaking it at or before the 80 char limit.

@@ +934,5 @@
> +  // Temporary nsIFile used for download.
> +  var tempFile;
> +
> +  // Images are downsized to this size while keeping the aspect ratio.
> +  const maxSize = 300;

Most consts (with the exception of things like the WebBrowserPersist constructor) follows the convention that their variable names start with k, or that they're all uppercase.

I prefer the k method, so this should be kMaxSize.

@@ +942,5 @@
> +  var callbackError;
> +  var callbackProgress;
> +
> +  // Start at 4% to show a slight progress initially
> +  const initProgress = 4;

kInitProgress.

@@ +945,5 @@
> +  // Start at 4% to show a slight progress initially
> +  const initProgress = 4;
> +
> +  // Constants indicating error and file transfer status
> +  const STATE_TRANSFERRING = 0;

So now we have inconsistent usage of constants. Either use the k prefix, or all uppercase. Try not to mix the two.

@@ +973,5 @@
> +   *   progress report. An error code (see above) and the progress percentage (0-100) is passed in.
> +   *   State transitions: STATE_TRANSFERRING -> STATE_RESIZING -> STATE_OK (100%)
> +   */
> +  function savePhoto(aURI, cbSuccess, cbError, cbProgress) {
> +    callbackSuccess = (typeof cbSuccess == "function")? cbSuccess: null;

Instead of setting these to null, maybe just set them to empty functions. That way, you don't have to keep checking for their existence.

@@ +977,5 @@
> +    callbackSuccess = (typeof cbSuccess == "function")? cbSuccess: null;
> +    callbackError = (typeof cbError == "function")? cbError: null;
> +    callbackProgress = (typeof cbProgress == "function")? cbProgress: null;
> +
> +    //Application.console.log('savePhoto: '+aURI);

Please remove debugging code.

@@ +988,5 @@
> +      callbackProgress(STATE_TRANSFERRING, initProgress);
> +    }
> +
> +    downloader = WebBrowserPersist();
> +    downloader.persistFlags = Ci.nsIWebBrowserPersist.PERSIST_FLAGS_BYPASS_CACHE

Lots of 80+ char violations below that should be cleaned up to improve readability.

@@ +995,5 @@
> +    downloader.progressListener = {
> +      onProgressChange: function(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress) {
> +        if (aMaxTotalProgress > -1 && callbackProgress) {
> +          // Download progress is 0-90%, 90-100% is verifying and scaling the image.
> +          var percent = Math.round(initProgress + (aCurTotalProgress / aMaxTotalProgress) * (90-initProgress));

spaces on either side of -.

Also, the "90" is a magic number, and should be replaced with a const like kInitProgress.

@@ +1013,5 @@
> +                callbackError(ERROR_UNAVAILABLE);
> +              }
> +            }
> +          } catch (err) {
> +            // The nsIHttpChannel interface is not available - just proceed

Should we not log this error or put it in the console instead of eating it silently?

@@ +1037,5 @@
> +    try {
> +      // Obtain the privacy context of the browser window that the URL
> +      // we are downloading comes from. If, and only if, the URL is not
> +      // related to a window, null should be used instead.
> +      var privacy = PrivateBrowsingUtils.privacyContextFromWindow(window);

Thunderbird definitely doesn't make use of private browsing. I don't think this is necessary.

@@ +1061,5 @@
> +      }
> +    }
> +    img.onload = function() {
> +      if (callbackProgress) {
> +        callbackProgress(STATE_RESIZING, 95);

Another magic number that should be assigned to a const.

@@ +1067,5 @@
> +
> +      // Images are scaled down in two steps to improve quality. Resizing ratios larger than 2 use
> +      // a different interpolation algorithm than small ratios. Resize three times (instead of just
> +      // two steps) to improve the final quality.
> +      var canvas = downscale(img, 3.8*maxSize);

3.8 and 1.9 are magic numbers.

@@ +1100,5 @@
> +    var w = graphicsObject.width;
> +    var h = graphicsObject.height;
> +
> +    if (w > h && w > maxDimension) {
> +      h = Math.round(maxDimension*h/w);

Spaces on either side of the *

@@ +1103,5 @@
> +    if (w > h && w > maxDimension) {
> +      h = Math.round(maxDimension*h/w);
> +      w = maxDimension;
> +    } else if (h > maxDimension) {
> +      w = Math.round(maxDimension*w/h);

Spaces on either side of the * - and use brackets to isolate w/h.

Example:

maxDimension * (w / h)

@@ +1165,5 @@
> +                            | Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES
> +                            | Ci.nsIWebBrowserPersist.PERSIST_FLAGS_CLEANUP_ON_FAILURE;
> +    downloader.progressListener = {
> +      onStateChange: function(aWebProgress, aRequest, aFlag, aStatus) {
> +        if (aFlag & Ci.nsIWebProgressListener.STATE_STOP) {

Is there a chance of something going wrong with the transfer? Out of memory, or anything like that? I think we need some error handling in here.

@@ +1176,5 @@
> +
> +    // Obtain the privacy context of the browser window that the URL
> +    // we are downloading comes from. If, and only if, the URL is not
> +    // related to a window, null should be used instead.
> +    var privacy = PrivateBrowsingUtils.privacyContextFromWindow(window);

Again, I'm not 100% convinced this is necessary because (unless this has changed recently and I'm unaware of it), TB makes no use of private browsing whatsoever.

Comment 3

[Samuel Müller]

Attachment: v2 addressing the comments on v1

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Thanks for the patches, Samuel! Just came off holidays, and I will have a review for you soon.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8542521 [details] [diff] [review]
v2 addressing the comments on v1

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

First off - humongous apologies for the delay in getting to this. :(

So, this is a pretty cool API, Samuel. I've got some questions though - see below.

::: mail/components/addrbook/content/abCommon.js
@@ +921,5 @@
> +// Attention: It is the responsibility of the caller to remove the old photo and update the card!
> +var gImageDownloader = (function() {
> +  const Ci = Components.interfaces;
> +
> +  var downloadInProgress = false;

let, not var - goes for the entire set of changes.

@@ +973,5 @@
> +   *   progress report. An error code (see above) and the progress percentage (0-100) is passed in.
> +   *   State transitions: STATE_TRANSFERRING -> STATE_RESIZING -> STATE_OK (100%)
> +   */
> +  function savePhoto(aURI, cbSuccess, cbError, cbProgress) {
> +    callbackSuccess = (typeof cbSuccess == "function")? cbSuccess: null;

Space before ? - same for lines 978 and 979.

@@ +977,5 @@
> +    callbackSuccess = (typeof cbSuccess == "function")? cbSuccess: null;
> +    callbackError = (typeof cbError == "function")? cbError: null;
> +    callbackProgress = (typeof cbProgress == "function")? cbProgress: null;
> +
> +    //Application.console.log('savePhoto: '+aURI);

Remove dead code.

@@ +987,5 @@
> +    if (callbackProgress) {
> +      callbackProgress(STATE_TRANSFERRING, initProgress);
> +    }
> +
> +    downloader = WebBrowserPersist();

What's the advantage of using WebBrowserPersist over, say, XHR?

@@ +1037,5 @@
> +    try {
> +      // Obtain the privacy context of the browser window that the URL
> +      // we are downloading comes from. If, and only if, the URL is not
> +      // related to a window, null should be used instead.
> +      var privacy = PrivateBrowsingUtils.privacyContextFromWindow(window);

Thunderbird does not use the privacy contexts for windows. I think this is superfluous.

@@ +1067,5 @@
> +
> +      // Images are scaled down in two steps to improve quality. Resizing ratios larger than 2 use
> +      // a different interpolation algorithm than small ratios. Resize three times (instead of just
> +      // two steps) to improve the final quality.
> +      var canvas = downscale(img, 3.8*maxSize);

Spaces on either side of the *'s

@@ +1100,5 @@
> +    var w = graphicsObject.width;
> +    var h = graphicsObject.height;
> +
> +    if (w > h && w > maxDimension) {
> +      h = Math.round(maxDimension*h/w);

Spaces on either side of the operators

Comment 6

[:aceman]

Attachment: 892889.patch v3

I have fixed the review comments from mconley and rewrapped some long comments.
To make this actually work right, I had to add some more status flag chekcing in the progress listeners, otherwise the code was run multiple times (there were multiple notifications with STATE_STOP) and bad things happened.

Samuel, thanks for the great code. I'm sorry it took so long to get back to this (unfortunately mconley left TB). If you are still around you could check my changes (you can compare the diffs using bugzilla).

This can be landed together with bug 691141 in TB 60 beta.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7aa7b9e29d85
Provide API for assigning photos to contacts in abCommon.js. r=aceman

Comment 8

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 5):
https://hg.mozilla.org/releases/comm-beta/rev/19e815a73cb363490dc7ea803a44872754a076c6