Add loadAsDataUrl to ThumbnailLoader.

Add loadAsDataUrl to ThumbnailLoader.

loadAsDataUrl loads thumbnail as data url instead of attaching it to a box. While loadAsDataUrl does not fit image to the box, it returns dataUrl of thumbnail which transform is applied when it is necessary.

BUG=438050
TEST=out/Release --gtest_filter=FileManagerJsTest.ThumbnailLoader

Committed: https://crrev.com/921c21162c175fe9e1bd6a46afff1d541d8d88a7
Cr-Commit-Position: refs/heads/master@{#314525}

[yawano, 2015-02-04 06:39:50]

yawano@chromium.org changed reviewers:
+ hirono@chromium.org

[yawano, 2015-02-04 06:39:50]

PTAL. Thank you!

[hirono, 2015-02-04 07:25:13]

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:244: * Load
thumbnail as data url. If data url of thumbnail can be fetched from
nit: Loads

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:251: * @param
{!Entry} entry An entry.
The entry is not shown in the argument list.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:256: return new
Promise(function(resolve) {
It looks opt_metadata has not been used here.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:282: * Apply
transform to data url.
nit: Applies

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:289: * @return
{!Promise} A promise which is resolved with dataUrl and its width and
nit: Could you annotate this as !Promise<{data:string, width:number,
height:number}>?

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:322:
context.translate(scaleX === -1 ? width : 0, scaleY === -1 ? height : 0);
We should either reject scaleX and scaleY that are negative but not -1, or
handle all negative values here.

[yawano, 2015-02-04 07:50:43]

Thank you for the review. PTAL.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:244: * Load
thumbnail as data url. If data url of thumbnail can be fetched from
On 2015/02/04 07:25:13, hirono wrote:
> nit: Loads

Done.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:251: * @param
{!Entry} entry An entry.
On 2015/02/04 07:25:13, hirono wrote:
> The entry is not shown in the argument list.

Done.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:256: return new
Promise(function(resolve) {
On 2015/02/04 07:25:13, hirono wrote:
> It looks opt_metadata has not been used here.

Done.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:282: * Apply
transform to data url.
On 2015/02/04 07:25:13, hirono wrote:
> nit: Applies

Done.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:289: * @return
{!Promise} A promise which is resolved with dataUrl and its width and
On 2015/02/04 07:25:13, hirono wrote:
> nit: Could you annotate this as !Promise<{data:string, width:number,
> height:number}>?

Done.

https://codereview.chromium.org/902513002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:322:
context.translate(scaleX === -1 ? width : 0, scaleY === -1 ? height : 0);
On 2015/02/04 07:25:13, hirono wrote:
> We should either reject scaleX and scaleY that are negative but not -1, or
> handle all negative values here.

Done. Added assertions for scaleX, scaleY and also for rotate90.

[hirono, 2015-02-04 08:00:33]

Thanks! lgtm with a nit.

https://codereview.chromium.org/902513002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):

https://codereview.chromium.org/902513002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:311:
assert(scaleX === 1 || scaleX === -1);
I prefer to put these assertions at the outside of Promise.
The difference is immediate exception vs rejected promise.
Immediate exception more remarkable and useful for finding programming errors.

[yawano, 2015-02-04 08:05:07]

New patchsets have been uploaded after l-g-t-m from hirono@chromium.org

[yawano, 2015-02-04 08:06:10]

Thank you!

https://codereview.chromium.org/902513002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/thumbnail_loader.js (right):

https://codereview.chromium.org/902513002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/thumbnail_loader.js:311:
assert(scaleX === 1 || scaleX === -1);
On 2015/02/04 08:00:33, hirono wrote:
> I prefer to put these assertions at the outside of Promise.
> The difference is immediate exception vs rejected promise.
> Immediate exception more remarkable and useful for finding programming errors.

Done.

[yawano, 2015-02-04 08:06:16]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-04 08:07:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/902513002/80001

[commit-bot: I haz the power, 2015-02-04 08:54:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-04 08:56:58]

Patchset 5 (id:??) landed as
https://crrev.com/921c21162c175fe9e1bd6a46afff1d541d8d88a7
Cr-Commit-Position: refs/heads/master@{#314525}