0
\$\begingroup\$

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;
        });
    });
};
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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);
    });
};
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$
    – Brian
    Jan 10 '14 at 20:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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