Quick View: Space key opens quick view for image, video and audio in Download.

Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo: https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/view?pli=1

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/8adb61c8b82ae2ede29e1d81cec7e025ecbabf02
Cr-Commit-Position: refs/heads/master@{#399165}

[oka, 2016-06-08 10:59:55]

Description was changed from

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

BUG=613465
TEST=manually
==========

to

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
==========

[oka, 2016-06-08 10:59:56]

oka@chromium.org changed reviewers:
+ fukino@chromium.org

[oka, 2016-06-08 12:57:09]

On 2016/06/08 10:59:56, oka wrote:
> mailto:oka@chromium.org changed reviewers:
> + mailto:fukino@chromium.org

PTAL

[fukino, 2016-06-09 08:52:51]

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/quick_view.html (right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.html:9: <dom-module
id="quick-view">
This view is Files app specific, so how about prefix it with files, like,
files-quick-view.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/quick_view.js (right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.js:13: },
missing blank line

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.js:19: this.image =
url;
It might be better to call clear_() and this.image  = url. (In case that we have
more media types.)

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.js:28: },
missing blank line

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:5: /**
Add a brief explanation about this element.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:20: /**
nit: insert one blank line between this property and the annotation of next
property. (Same goes for all properties.)

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:45:
QuickViewController.prototype.onKeyPressToOpen_ = function(event) {
Please keep the previous behavior when no file is selected.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:45:
QuickViewController.prototype.onKeyPressToOpen_ = function(event) {
Please add comments/annotations for these functions in this file.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:46: if
(event.keyIdentifier == 'U+0020') {
keyIdentifier is obsolete. Use event.key === ' '

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:53: if
(event.keyIdentifier == 'U+0020') {
ditto

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:84:
assert(this.entries_.length > 0);
This conditions doesn't seem to be met.

[oka, 2016-06-09 15:13:28]

PTAL

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/quick_view.html (right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.html:9: <dom-module
id="quick-view">
On 2016/06/09 08:52:50, fukino wrote:
> This view is Files app specific, so how about prefix it with files, like,
> files-quick-view.

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/quick_view.js (right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.js:13: },
On 2016/06/09 08:52:50, fukino wrote:
> missing blank line

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.js:19: this.image =
url;
On 2016/06/09 08:52:50, fukino wrote:
> It might be better to call clear_() and this.image  = url. (In case that we
have
> more media types.)

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.js:28: },
On 2016/06/09 08:52:50, fukino wrote:
> missing blank line

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:5: /**
On 2016/06/09 08:52:51, fukino wrote:
> Add a brief explanation about this element.

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:20: /**
On 2016/06/09 08:52:51, fukino wrote:
> nit: insert one blank line between this property and the annotation of next
> property. (Same goes for all properties.)

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:45:
QuickViewController.prototype.onKeyPressToOpen_ = function(event) {
On 2016/06/09 08:52:51, fukino wrote:
> Please keep the previous behavior when no file is selected.

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:45:
QuickViewController.prototype.onKeyPressToOpen_ = function(event) {
On 2016/06/09 08:52:51, fukino wrote:
> Please add comments/annotations for these functions in this file.

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:46: if
(event.keyIdentifier == 'U+0020') {
On 2016/06/09 08:52:50, fukino wrote:
> keyIdentifier is obsolete. Use event.key === ' '

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:53: if
(event.keyIdentifier == 'U+0020') {
On 2016/06/09 08:52:51, fukino wrote:
> ditto

Done.

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:84:
assert(this.entries_.length > 0);
On 2016/06/09 08:52:51, fukino wrote:
> This conditions doesn't seem to be met.

Updated so that assert is met.

[oka, 2016-06-09 15:13:36]

Description was changed from

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
==========

to

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler
==========

[oka, 2016-06-09 15:16:14]

nit

[fukino, 2016-06-09 16:08:26]

lgtm with a nit

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/quick_view.html (right):

https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/quick_view.html:9: <dom-module
id="quick-view">
On 2016/06/09 15:13:28, oka wrote:
> On 2016/06/09 08:52:50, fukino wrote:
> > This view is Files app specific, so how about prefix it with files, like,
> > files-quick-view.
> 
> Done.

Please update the file name and class name, too.

[oka, 2016-06-10 12:04:02]

On 2016/06/09 16:08:26, fukino wrote:
> lgtm with a nit
> 
>
https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
> File ui/file_manager/file_manager/foreground/elements/quick_view.html (right):
> 
>
https://codereview.chromium.org/2043333002/diff/1/ui/file_manager/file_manage...
> ui/file_manager/file_manager/foreground/elements/quick_view.html:9:
<dom-module
> id="quick-view">
> On 2016/06/09 15:13:28, oka wrote:
> > On 2016/06/09 08:52:50, fukino wrote:
> > > This view is Files app specific, so how about prefix it with files, like,
> > > files-quick-view.
> > 
> > Done.
> 
> Please update the file name and class name, too.

Done.

[oka, 2016-06-10 12:04:40]

The CQ bit was checked by oka@chromium.org

[oka, 2016-06-10 12:04:41]

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/2043333002/#ps60001
(title: "Resolve comment")

[commit-bot: I haz the power, 2016-06-10 12:04:50]

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

[commit-bot: I haz the power, 2016-06-10 12:46:05]

Description was changed from

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler
==========

[commit-bot: I haz the power, 2016-06-10 12:46:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-10 12:46:13]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 12:47:47]

Description was changed from

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler
==========

to

==========
Quick View: Space key opens quick view for image, video and audio in Download.
--disable-files-quick-view disables quick view.

Demo:
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhbkw5UW5GVGNDLXc/vie...

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/8adb61c8b82ae2ede29e1d81cec7e025ecbabf02
Cr-Commit-Position: refs/heads/master@{#399165}
==========

[commit-bot: I haz the power, 2016-06-10 12:47:48]

Patchset 4 (id:??) landed as
https://crrev.com/8adb61c8b82ae2ede29e1d81cec7e025ecbabf02
Cr-Commit-Position: refs/heads/master@{#399165}