Gallery.app: Move geometry calculation for the slide image to Viewport class.

Gallery.app: Move geometry calculation for the slide image to Viewport class.

This is a preparation for implementing the zoom feature in Gallery.app.

BUG=245926
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282553

[hirono, 2014-07-10 06:58:48]

PTAL the CL? Thank you!

[mtomasz, 2014-07-10 07:26:52]

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/image_editor/image_view.js (right):

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:654: new
ImageView.Effect.Rotate(rotate90 > 0) :
rotate90 > 0 is bool, but number is expected

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:895: * @override
ImageView.Effect doesn't have transform() so shouldn't be @override?

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:907: * @param {Rect}
deviceTargetRect Target rectangle.
nit: please update jsdoc

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:926: * @override
ditto

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:951: * @override
ditto

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

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:29: */
optional: // for normal comments, /** ... */ for jsdoc? If jsdoc, then please
add @type etc.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:104:
Viewport.prototype.getFittingScaleForImageSize_ = function(width, height) {
nit: Please add jsdoc.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:107: // Scales > (1 /
this.getDevicePixelRatio()) do not look good. Also they are
Please update the comment (this.getDevicePixelRatio / devicePixelRatio).

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:109: return Math.min(1 /
devicePixelRatio, scaleX, scaleY);
nit: devicePixelRatio looks undefined. How about: How about
window.devicePixelRatio?

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:467: 'scale(' + (scaleRatio)
+ ')',
nit: inner () redundant?

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:512: var scaleY =
screenRect.height /screenImageHeight;
nit: space after '/'.

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

https://codereview.chromium.org/384573002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:11: function Viewport() {
IIUC, you want to add "zooming by user" and "offset by user" to this class in
order to implement zoom/pinch-to-zoom, and then adjust all of the transform
strings based on these arguments?

https://codereview.chromium.org/384573002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:30: this.scale_ = 1;
nit: Is scale_, setScale and getFittingScale used?

https://codereview.chromium.org/384573002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:31: this.offsetX_ = 0;
nit: Seems that offsetX and offsetY are not used?

[hirono, 2014-07-10 08:16:06]

Thank you!

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
File ui/file_manager/gallery/js/image_editor/image_view.js (right):

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:654: new
ImageView.Effect.Rotate(rotate90 > 0) :
On 2014/07/10 07:26:52, mtomasz wrote:
> rotate90 > 0 is bool, but number is expected

Fixed the JSDoc.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:895: * @override
On 2014/07/10 07:26:52, mtomasz wrote:
> ImageView.Effect doesn't have transform() so shouldn't be @override?

Added an abstract transform method to the super class.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:907: * @param {Rect}
deviceTargetRect Target rectangle.
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: please update jsdoc

Acknowledged.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:926: * @override
On 2014/07/10 07:26:52, mtomasz wrote:
> ditto

Done.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/image_view.js:951: * @override
On 2014/07/10 07:26:52, mtomasz wrote:
> ditto

Done.

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

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:29: */
On 2014/07/10 07:26:52, mtomasz wrote:
> optional: // for normal comments, /** ... */ for jsdoc? If jsdoc, then please
> add @type etc.

Done.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:104:
Viewport.prototype.getFittingScaleForImageSize_ = function(width, height) {
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: Please add jsdoc.

Done.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:107: // Scales > (1 /
this.getDevicePixelRatio()) do not look good. Also they are
On 2014/07/10 07:26:52, mtomasz wrote:
> Please update the comment (this.getDevicePixelRatio / devicePixelRatio).

Done.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:109: return Math.min(1 /
devicePixelRatio, scaleX, scaleY);
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: devicePixelRatio looks undefined. How about: How about
> window.devicePixelRatio?

Done.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:467: 'scale(' + (scaleRatio)
+ ')',
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: inner () redundant?

Done.

https://codereview.chromium.org/384573002/diff/40001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:512: var scaleY =
screenRect.height /screenImageHeight;
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: space after '/'.

Done.

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

https://codereview.chromium.org/384573002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:11: function Viewport() {
On 2014/07/10 07:26:52, mtomasz wrote:
> IIUC, you want to add "zooming by user" and "offset by user" to this class in
> order to implement zoom/pinch-to-zoom, and then adjust all of the transform
> strings based on these arguments?

Yes, that is my plan.

https://codereview.chromium.org/384573002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:30: this.scale_ = 1;
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: Is scale_, setScale and getFittingScale used?

setScale uses it. getFittingScale actually does not use the variable, but the
return value is expected to assign to the variable.

https://codereview.chromium.org/384573002/diff/60001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/image_editor/viewport.js:31: this.offsetX_ = 0;
On 2014/07/10 07:26:52, mtomasz wrote:
> nit: Seems that offsetX and offsetY are not used?

This is not used so far. But will start to use soon.

[mtomasz, 2014-07-11 00:54:33]

lgtm!

[mtomasz, 2014-07-11 00:54:34]

lgtm!

[hirono, 2014-07-11 02:07:03]

The CQ bit was checked by hirono@chromium.org

[hirono, 2014-07-11 02:07:09]

On 2014/07/11 00:54:34, mtomasz wrote:
> lgtm!

Thank you!

[commit-bot: I haz the power, 2014-07-11 02:09:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hirono@chromium.org/384573002/80001

[commit-bot: I haz the power, 2014-07-11 05:05:24]

Change committed as 282553