Gallery: Takes into account target canvas size when calculating view port.

Gallery: Takes into account target canvas size when calculating view port.

Gallery applies viewport both screen size cache, and full resolution image. To
handle the different size of two images, it used to set CSS 'width' and 'height'
style to the full resolution image so that the full resolution image can be
regarded as the same size of screen cache image.

But ImageView.Effect class, which is used for playing edit animation, does not
update the CSS properties. It causes wrong layout during the edit animation.

The CL removes the trick of CSS property, and let ImageView.Effect take into
account of the size of target canvas.

BUG=487129
TEST=Do and undo cropping and rotaiton.

Committed: https://crrev.com/171777f7e0ef8be56f2c60fe7e62dd2480bc7fd3
Cr-Commit-Position: refs/heads/master@{#329793}

[hirono, 2015-05-13 09:14:26]

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

[hirono, 2015-05-13 09:14:27]

PTAL, thanks!

[mtomasz, 2015-05-13 10:27:10]

I'll get a closer look tomorrow morning.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
File ui/file_manager/gallery/js/image_editor/viewport.js (right):

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:283:
assert(this.imageElementBoundsOnScreen_);
nit: This seems to be used only in this.getDeviceBounds. Shall we make private,
or even remove?

[mtomasz, 2015-05-14 00:32:32]

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
File ui/file_manager/gallery/js/image_editor/viewport.js (right):

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:196: *
ni: Please update jsdoc.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:479:
Viewport.prototype.getScreenRectTransformation = function(
nit: jsdoc missing

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:484: return [
optional: I think simply concatenating is easier to read, but it's up to you.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:490:
Viewport.prototype.getCroppingTransformation = function(
nit: jsdoc missing

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:535: * @param {number}
rotation90 Orientation of the rotation from the original
nit: Why the name is rotation90? It's hard for me to understand the meaning.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:536: *     image to the
rotated image. True is for clockwise and false is for
nit: Please update the comment, it's not bool anymore.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:547: * @private
nit: @jsdoc missing

[hirono, 2015-05-14 01:57:21]

Thanks!

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
File ui/file_manager/gallery/js/image_editor/viewport.js (right):

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:196: *
On 2015/05/14 00:32:32, mtomasz wrote:
> ni: Please update jsdoc.

Done.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:283:
assert(this.imageElementBoundsOnScreen_);
On 2015/05/13 10:27:10, mtomasz wrote:
> nit: This seems to be used only in this.getDeviceBounds. Shall we make
private,
> or even remove?

Done.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:479:
Viewport.prototype.getScreenRectTransformation = function(
On 2015/05/14 00:32:32, mtomasz wrote:
> nit: jsdoc missing

Done.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:484: return [
On 2015/05/14 00:32:32, mtomasz wrote:
> optional: I think simply concatenating is easier to read, but it's up to you.

I tries to create stringFormat helper function. Done.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:490:
Viewport.prototype.getCroppingTransformation = function(
On 2015/05/14 00:32:32, mtomasz wrote:
> nit: jsdoc missing

Done.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:535: * @param {number}
rotation90 Orientation of the rotation from the original
On 2015/05/14 00:32:32, mtomasz wrote:
> nit: Why the name is rotation90? It's hard for me to understand the meaning.

It was number of times for 90 degrees. e.g. 180 degree = 2 (x 90 degree). I
renamed it with rotation.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:536: *     image to the
rotated image. True is for clockwise and false is for
On 2015/05/14 00:32:32, mtomasz wrote:
> nit: Please update the comment, it's not bool anymore.

Done.

https://codereview.chromium.org/1138053003/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/viewport.js:547: * @private
On 2015/05/14 00:32:32, mtomasz wrote:
> nit: @jsdoc missing

Done.

[mtomasz, 2015-05-14 02:27:55]

lgtm with nits

https://codereview.chromium.org/1138053003/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/viewport.js (right):

https://codereview.chromium.org/1138053003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/viewport.js:603: var d = formatString(
nit: Per our style guide abbreviations are discouraged. How about
"transformation" or "result"?

https://codereview.chromium.org/1138053003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/viewport.js:609: console.log(d);
nit: Remove console.log?

[hirono, 2015-05-14 03:08:03]

Thank you!

https://codereview.chromium.org/1138053003/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/viewport.js (right):

https://codereview.chromium.org/1138053003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/viewport.js:603: var d = formatString(
On 2015/05/14 02:27:55, mtomasz wrote:
> nit: Per our style guide abbreviations are discouraged. How about
> "transformation" or "result"?

Yes, I removed console.log, and let it return the result directory from
formatString.

https://codereview.chromium.org/1138053003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/viewport.js:609: console.log(d);
On 2015/05/14 02:27:55, mtomasz wrote:
> nit: Remove console.log?

Done.

[hirono, 2015-05-14 03:08:07]

The CQ bit was checked by hirono@chromium.org

[hirono, 2015-05-14 03:08:07]

The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/1138053003/#ps40001
(title: "Fix.")

[commit-bot: I haz the power, 2015-05-14 03:08:19]

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

[commit-bot: I haz the power, 2015-05-14 03:35:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-14 03:36:36]

Patchset 3 (id:??) landed as
https://crrev.com/171777f7e0ef8be56f2c60fe7e62dd2480bc7fd3
Cr-Commit-Position: refs/heads/master@{#329793}