Screenshot-scaling code

Question

Please review for readability.

/*
Takes a screenshot of the current tab, then scales it to width and height.
Returns null if the tab is smaller than width and height.
Crops the screenshot to maintain aspect ratio.
Also crops the screenshot to avoid scaling down by more maximumScaleFactor.
*/
var GetCurrentScreenshot = function (width, height, maximumScaleFactor) {
    return new RSVP.Promise(function (resolve) {
        chrome.tabs.captureVisibleTab(null, { format: 'png' }, function (dataURL) {
            var sourceImage = new Image();
            sourceImage.onload = function () {
                var canvas = document.createElement("canvas");
                var cropWidth = sourceImage.width;
                var cropHeight = sourceImage.height;
                if (cropWidth < width || cropHeight < height) {
                    resolve(null);
                    return;
                }
                var outputRatio = width / height;
                var inputRatio = cropWidth / cropHeight;
                if (outputRatio > inputRatio) {
                    cropHeight = cropWidth / outputRatio;
                } else {
                    cropWidth = cropHeight * outputRatio;
                }

                //This should be impossible
                if (cropWidth > sourceImage.width) cropWidth = sourceImage.width;
                if (cropHeight > sourceImage.height) cropHeight = sourceImage.height;

                if (cropWidth > width * maximumScaleFactor) {
                    cropHeight = cropHeight / (cropWidth / (width * maximumScaleFactor));
                    cropWidth = width * maximumScaleFactor;
                }

                canvas.width = width;
                canvas.height = height;
                canvas.getContext("2d").drawImage(sourceImage, 0, 0, cropWidth, 
                    cropHeight, 0, 0, width, height);
                resolve(canvas.toDataURL());
            };
            sourceImage.src = dataURL;
        });
    });
};

Answer 1

Breaking out the callback function

I would break out that function as well, though I would give it a better name than func, mayhaps drawScaledScreenshot?

Functionality

• CR is not the best place to discuss functionality, but why would you not draw the image if the tab size is too small, it does not seem to make sense.

• Also, why would you only check for width going over the maximumScaleFactor ? I would check height as well.

Scope

I feel the following vars could have been declared outside of the drawScaledScreenshot function, since they only depend on width and height.

var outputRatio      = width / height;
var maximumCropWidth = width * maximumScaleFactor; //You calc this several times

Paranoia

If you truly think it is impossible, then it should not be in the code

//This should be impossible
if (cropWidth > sourceImage.width) cropWidth = sourceImage.width;
if (cropHeight > sourceImage.height) cropHeight = sourceImage.height;

If you think it is possible, then I would suggest you convert this to the more readble

//This should not be needed
cropWidth  = Math.min( cropWidth,  sourceImage.width);
cropHeight = Math.min( cropHeight, sourceImage.height);

Notice that if width * maximumScaleFactor is exceeded, you can still get a cropHeight > sourceImage.height..

Furtermore, I would be more paranoid that the caller forgets to provide maximumScaleFactor and would add a

maximumScaleFactor = maximumScaleFactor || 10; //Or whatever is reasonable

Finally, since you are dividing by height, I would verify that height is not zero.

Answer 2

I'd break out that inner function - easier to see where it starts and ends.

/*
Takes a screenshot of the current tab, then scales it to width and height.
Returns null if the tab is smaller than width and height.
Crops the screenshot to maintain aspect ratio.
Also crops the screenshot to avoid scaling down by more maximumScaleFactor.
*/
var GetCurrentScreenshot = function (width, height, maximumScaleFactor) {
    return new RSVP.Promise(function (resolve) {

        var func = function (dataURL) {
            var sourceImage = new Image();
            sourceImage.onload = function () {
                var canvas = document.createElement("canvas");
                var cropWidth = sourceImage.width;
                var cropHeight = sourceImage.height;
                if (cropWidth < width || cropHeight < height) {
                    resolve(null);
                    return;
                }
                var outputRatio = width / height;
                var inputRatio = cropWidth / cropHeight;
                if (outputRatio > inputRatio) {
                    cropHeight = cropWidth / outputRatio;
                } else {
                    cropWidth = cropHeight * outputRatio;
                }

                //This should be impossible
                if (cropWidth > sourceImage.width) cropWidth = sourceImage.width;
                if (cropHeight > sourceImage.height) cropHeight = sourceImage.height;

                if (cropWidth > width * maximumScaleFactor) {
                    cropHeight = cropHeight / (cropWidth / (width * maximumScaleFactor));
                    cropWidth = width * maximumScaleFactor;
                }

                canvas.width = width;
                canvas.height = height;
                canvas.getContext("2d").drawImage(sourceImage, 0, 0, cropWidth,
                    cropHeight, 0, 0, width, height);
                resolve(canvas.toDataURL());
            };
            sourceImage.src = dataURL;
        };
        chrome.tabs.captureVisibleTab(null, {
            format: 'png'
        }, func);
    });
};