Add an ability for resize in gallery.

Add an ability for resize in gallery.

BUG=554440
TEST=browser_tests --gtest_filter=GalleryBrowserTestInGuestMode.ResizeImageOnDownloads

Committed: https://crrev.com/2a9b96ee109657042e20d9e5bc82e0b430c78e87
Cr-Commit-Position: refs/heads/master@{#419151}

[harukam, 2016-08-31 02:00:37]

harukam@google.com changed reviewers:
+ fukino@chromium.org

[harukam, 2016-08-31 02:00:38]

Fukino-san, PTAL.

[fukino, 2016-08-31 05:10:25]

Could you add a ui test to prevent this function from being broken in the
future?

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/css...
File ui/file_manager/gallery/css/gallery.css (right):

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/css...
ui/file_manager/gallery/css/gallery.css:757: url(../images/100/image-resize.png)
1x,
Use filename 'image_resize.png' instead of 'image-resize.png' to be consistent
with other image file names.

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

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/gallery.js:781: event.preventDefault();
indent is needed

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

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/commands.js:462: Command.ImageResize =
function(inputWidth, inputHeight) {
Command.Resize sounds more consistent with Command.Crop, Command.Rotate, etc...

I think we won't confuse with 'resize' event on app window, since these commands
are always executed on images.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/commands.js:474: document,
4 spaces indent.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/commands.js:475: srcImage,
All arguments can be fit in one line.

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

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:12:
ImageEditor.Mode.ImageResize = function() {
Mode.Resize looks more consistent with other mode.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:16: * @type {number}
@private {number}

ditto for following properties.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:98: name, event) {
4 space indent

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:224: function() {
4 space indent.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:238:
ImageEditor.Mode.prototype.cleanUpUI.apply(this, arguments);
If you do nothing special in this method, you don't have to override it.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:243: * @type {number}
@const {number}

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:245:
ImageEditor.Mode.ImageResize.MINIMUM_VALID_VALUE = 1;
Could you put these constants just after the constructor definition? (around
line #70?)

[harukam, 2016-09-05 10:36:22]

Description was changed from

==========
Add an ability for resize in gallery.

BUG=554440
TEST=None
==========

to

==========
Add an ability for resize in gallery.

BUG=554440
TEST=browser_tests
--gtest_filter=GalleryBrowserTestInGuestMode.ResizeImageOnDownloads
==========

[harukam, 2016-09-05 10:37:01]

Thanks for review.
Please take another look.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/css...
File ui/file_manager/gallery/css/gallery.css (right):

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/css...
ui/file_manager/gallery/css/gallery.css:757: url(../images/100/image-resize.png)
1x,
On 2016/08/31 05:10:24, fukino wrote:
> Use filename 'image_resize.png' instead of 'image-resize.png' to be consistent
> with other image file names.

We've decided to use filename 'resize.png' according to the new mode name
'ImageEditor.Mode.Resize'.

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

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/gallery.js:781: event.preventDefault();
On 2016/08/31 05:10:24, fukino wrote:
> indent is needed

Done.

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

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/commands.js:462: Command.ImageResize =
function(inputWidth, inputHeight) {
On 2016/08/31 05:10:24, fukino wrote:
> Command.Resize sounds more consistent with Command.Crop, Command.Rotate,
etc...
> 
> I think we won't confuse with 'resize' event on app window, since these
commands
> are always executed on images.

Agree. Thanks! done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/commands.js:474: document,
On 2016/08/31 05:10:24, fukino wrote:
> 4 spaces indent.

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/commands.js:475: srcImage,
On 2016/08/31 05:10:24, fukino wrote:
> All arguments can be fit in one line.

Done.

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

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:12:
ImageEditor.Mode.ImageResize = function() {
On 2016/08/31 05:10:24, fukino wrote:
> Mode.Resize looks more consistent with other mode.

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:16: * @type {number}
On 2016/08/31 05:10:25, fukino wrote:
> @private {number}
> 
> ditto for following properties.

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:98: name, event) {
On 2016/08/31 05:10:25, fukino wrote:
> 4 space indent

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:224: function() {
On 2016/08/31 05:10:25, fukino wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:238:
ImageEditor.Mode.prototype.cleanUpUI.apply(this, arguments);
On 2016/08/31 05:10:25, fukino wrote:
> If you do nothing special in this method, you don't have to override it.

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:243: * @type {number}
On 2016/08/31 05:10:24, fukino wrote:
> @const {number}

Done.

https://codereview.chromium.org/2299493002/diff/1/ui/file_manager/gallery/js/...
ui/file_manager/gallery/js/image_editor/image_resize.js:245:
ImageEditor.Mode.ImageResize.MINIMUM_VALID_VALUE = 1;
On 2016/08/31 05:10:24, fukino wrote:
> Could you put these constants just after the constructor definition? (around
> line #70?)

Done.

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1370:
text.addEventListener('change', handler, false);
I set a handler listening for 'change' event to adjust
test.util.sync.changeValue in gallery/js/test_util.js.

[fukino, 2016-09-05 13:44:34]

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:689: if(commit &&
this.currentMode_.name === 'image-resize') {
I think this 'image-resize' should be 'resize'?
The dialog won't be shown on the latest code.

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1367: // We should
listen for not only 'change' event, but also 'input' because of
listen to

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1368: // calling handler
as fast as possible.
"because of calling..." doesn't sound saying purpose.

How about "because we want to update values as soon as the user types
characters." or something like that?

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:133: this.maxValidValue_
= Math.max(
Can we have max value for width and height separately?
If width is 20000 and height is 10000, we don't want the height to be increased.

[fukino, 2016-09-05 13:44:35]

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:689: if(commit &&
this.currentMode_.name === 'image-resize') {
I think this 'image-resize' should be 'resize'?
The dialog won't be shown on the latest code.

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1367: // We should
listen for not only 'change' event, but also 'input' because of
listen to

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1368: // calling handler
as fast as possible.
"because of calling..." doesn't sound saying purpose.

How about "because we want to update values as soon as the user types
characters." or something like that?

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:133: this.maxValidValue_
= Math.max(
Can we have max value for width and height separately?
If width is 20000 and height is 10000, we don't want the height to be increased.

[harukam, 2016-09-06 08:10:29]

Thanks for your review.
Please take another look.

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:689: if(commit &&
this.currentMode_.name === 'image-resize') {
On 2016/09/05 13:44:34, fukino wrote:
> I think this 'image-resize' should be 'resize'?
> The dialog won't be shown on the latest code.

Sorry, thanks.

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1367: // We should
listen for not only 'change' event, but also 'input' because of
On 2016/09/05 13:44:34, fukino wrote:
> listen to

Acknowledged.

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1368: // calling handler
as fast as possible.
On 2016/09/05 13:44:34, fukino wrote:
> "because of calling..." doesn't sound saying purpose.
> 
> How about "because we want to update values as soon as the user types
> characters." or something like that?

It's helpful, thanks.

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

https://codereview.chromium.org/2299493002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:133: this.maxValidValue_
= Math.max(
On 2016/09/05 13:44:35, fukino wrote:
> Can we have max value for width and height separately?
> If width is 20000 and height is 10000, we don't want the height to be
increased.

Acknowledged.

[fukino, 2016-09-06 10:50:35]

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

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:747: *
comment?

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:749:
ImageEditor.prototype.setKeyEventHandled = function(handled) {
I'd like to avoid circular dependency which this method involves.
* ImageEditor has a instance of ResizeMode.
* ResizeMode calls ImageEditor's method.

To avoid accessing ImageEditor instance from ResizeMode, how about doing this?
- Add a function ImageEditor.Mode.prototype.isConsumingKeyEvents and the default
implementation is just returning false.
- Override the function in ResizeMode which returns true while dialog is open.
- ImageEditor checks the current mode's isConsumingKeyEvents().

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1379:
text.addEventListener('blur', onblur, true);
true -> false.

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

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:69:
this.onInputChanged_.bind(this, 'width'),
indent should be 4.

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:73:
this.onInputChanged_.bind(this, 'height'),
ditto

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:77:
this.onCheckBoxChanged_.bind(this), this.fixedRatio_);
ditto

[harukam, 2016-09-09 10:37:43]

Thanks. Please take a look again.

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

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:747: *
On 2016/09/06 10:50:35, fukino wrote:
> comment?

Sorry for the carelessness.

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:749:
ImageEditor.prototype.setKeyEventHandled = function(handled) {
On 2016/09/06 10:50:35, fukino wrote:
> I'd like to avoid circular dependency which this method involves.
> * ImageEditor has a instance of ResizeMode.
> * ResizeMode calls ImageEditor's method.
> 
> To avoid accessing ImageEditor instance from ResizeMode, how about doing this?
> - Add a function ImageEditor.Mode.prototype.isConsumingKeyEvents and the
default
> implementation is just returning false.
> - Override the function in ResizeMode which returns true while dialog is open.
> - ImageEditor checks the current mode's isConsumingKeyEvents().

Sounds good, thanks.

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:1379:
text.addEventListener('blur', onblur, true);
On 2016/09/06 10:50:35, fukino wrote:
> true -> false.

Done.

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

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:69:
this.onInputChanged_.bind(this, 'width'),
On 2016/09/06 10:50:35, fukino wrote:
> indent should be 4.

Thanks, done.

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:73:
this.onInputChanged_.bind(this, 'height'),
On 2016/09/06 10:50:35, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2299493002/diff/40001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:77:
this.onCheckBoxChanged_.bind(this), this.fixedRatio_);
On 2016/09/06 10:50:35, fukino wrote:
> ditto

Done.

[fukino, 2016-09-16 02:24:50]

https://codereview.chromium.org/2299493002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/gallery_browsertest.cc (right):

https://codereview.chromium.org/2299493002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/gallery_browsertest.cc:325:
IN_PROC_BROWSER_TEST_F(GalleryBrowserTestInGuestMode, ResizeImageOnDownloads) {
MAYBE_ is missing

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/css/gallery.css (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/css/gallery.css:791: height: 56px;
Should not affect edit modals other than "Resize"

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/gallery.html (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:82: color: rgba(255, 255, 255, 0.54);
Sort

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:86: }
; is missing

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/commands.js (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/commands.js:472: document, srcImage,
callback, uiContext) {
4 space indent.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_editor.js (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:767: if
(this.currentMode_ && this.currentMode_.isConsumingKeyEvents()) {
{} is not necessary

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:768: return false;
The key is handled in currentMode, so returning true sounds consistent.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_resize.js (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:77:
this.onLockIconClicked_.bind(this), 'lockicon');
4 space indent.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:78: if(this.fixedRatio_)
this.lockIcon_.setAttribute('locked', true);
if (this.fixedRatio_)
  this.lockIcon_.setAttribute('locked', true);

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:78: if(this.fixedRatio_)
this.lockIcon_.setAttribute('locked', true);
If the 'locked' is a boolean attribute, setAttribute('locked', '') is
sufficient.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:87: * @param {string}
name Name
Remove 'Name' or add informative description.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:88: * @param {Event}
event An event.
ditto

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:129:
this.lockIcon_.removeAttribute('locked');
this line can be moved to:
if (toggled) {
  ...
} else {
  this.lockIcon_.removeAttribute('locked');
}

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:134:
this.lockIcon_.setAttribute('locked', true);
setAttribute('locked', '');

[harukam, 2016-09-16 05:21:59]

fukino-san, please take another look.

https://codereview.chromium.org/2299493002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/gallery_browsertest.cc (right):

https://codereview.chromium.org/2299493002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/gallery_browsertest.cc:325:
IN_PROC_BROWSER_TEST_F(GalleryBrowserTestInGuestMode, ResizeImageOnDownloads) {
On 2016/09/16 02:24:49, fukino wrote:
> MAYBE_ is missing

Acknowledged.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/css/gallery.css (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/css/gallery.css:791: height: 56px;
On 2016/09/16 02:24:49, fukino wrote:
> Should not affect edit modals other than "Resize"

Acknowledged.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/gallery.html (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:82: color: rgba(255, 255, 255, 0.54);
On 2016/09/16 02:24:49, fukino wrote:
> Sort

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:86: }
On 2016/09/16 02:24:49, fukino wrote:
> ; is missing

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/commands.js (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/commands.js:472: document, srcImage,
callback, uiContext) {
On 2016/09/16 02:24:49, fukino wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_editor.js (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:767: if
(this.currentMode_ && this.currentMode_.isConsumingKeyEvents()) {
On 2016/09/16 02:24:49, fukino wrote:
> {} is not necessary

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_editor.js:768: return false;
On 2016/09/16 02:24:49, fukino wrote:
> The key is handled in currentMode, so returning true sounds consistent.

The event cannot be consumed by preventDefault() in line 771, 'gallery.js' if
this function returns true. Maybe we should get another approach?

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_resize.js (right):

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:77:
this.onLockIconClicked_.bind(this), 'lockicon');
On 2016/09/16 02:24:49, fukino wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:78: if(this.fixedRatio_)
this.lockIcon_.setAttribute('locked', true);
On 2016/09/16 02:24:49, fukino wrote:
> If the 'locked' is a boolean attribute, setAttribute('locked', '') is
> sufficient.

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:78: if(this.fixedRatio_)
this.lockIcon_.setAttribute('locked', true);
On 2016/09/16 02:24:49, fukino wrote:
> If the 'locked' is a boolean attribute, setAttribute('locked', '') is
> sufficient.

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:87: * @param {string}
name Name
On 2016/09/16 02:24:49, fukino wrote:
> Remove 'Name' or add informative description.

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:88: * @param {Event}
event An event.
On 2016/09/16 02:24:49, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:129:
this.lockIcon_.removeAttribute('locked');
On 2016/09/16 02:24:49, fukino wrote:
> this line can be moved to:
> if (toggled) {
>   ...
> } else {
>   this.lockIcon_.removeAttribute('locked');
> }

Acknowledged.

https://codereview.chromium.org/2299493002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_resize.js:134:
this.lockIcon_.setAttribute('locked', true);
On 2016/09/16 02:24:49, fukino wrote:
> setAttribute('locked', '');

Done.

[fukino, 2016-09-16 06:09:01]

LGTM!

[harukam, 2016-09-16 06:24:01]

The CQ bit was checked by harukam@google.com

[commit-bot: I haz the power, 2016-09-16 06:24:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 07:35:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 07:35:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[harukam, 2016-09-16 12:09:35]

The CQ bit was checked by harukam@google.com

[harukam, 2016-09-16 12:09:36]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2299493002/#ps120001
(title: "Make a change in GalleryJsTest.SlideModeTest")

[commit-bot: I haz the power, 2016-09-16 12:09:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 12:56:05]

Description was changed from

==========
Add an ability for resize in gallery.

BUG=554440
TEST=browser_tests
--gtest_filter=GalleryBrowserTestInGuestMode.ResizeImageOnDownloads
==========

to

==========
Add an ability for resize in gallery.

BUG=554440
TEST=browser_tests
--gtest_filter=GalleryBrowserTestInGuestMode.ResizeImageOnDownloads
==========

[commit-bot: I haz the power, 2016-09-16 12:56:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-16 12:57:58]

Description was changed from

==========
Add an ability for resize in gallery.

BUG=554440
TEST=browser_tests
--gtest_filter=GalleryBrowserTestInGuestMode.ResizeImageOnDownloads
==========

to

==========
Add an ability for resize in gallery.

BUG=554440
TEST=browser_tests
--gtest_filter=GalleryBrowserTestInGuestMode.ResizeImageOnDownloads

Committed: https://crrev.com/2a9b96ee109657042e20d9e5bc82e0b430c78e87
Cr-Commit-Position: refs/heads/master@{#419151}
==========

[commit-bot: I haz the power, 2016-09-16 12:57:59]

Patchset 7 (id:??) landed as
https://crrev.com/2a9b96ee109657042e20d9e5bc82e0b430c78e87
Cr-Commit-Position: refs/heads/master@{#419151}