Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;
        });
    });
};
share|improve this question
up vote 1 down vote accepted

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.

share|improve this answer

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);
    });
};
share|improve this answer
    
Personally, I prefer the original version, since I read it as, "Capture the Visible tab, then do stuff," which your proposal loses. I suppose I could have the best of both worlds by wrapping captureVisibleTab in a promise, but that strikes me as overkill. – Brian Jan 10 '14 at 20:26

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.