Quick View: Initial implementation of "No preview availale" and UI update.

Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview available", and update appearance to somewhat match mocks.
Also supported audio thumbnail.

Demo(internal): https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/view?usp=sharing

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/743414df7e53e88c911f469d604779e5399c85ed
Cr-Commit-Position: refs/heads/master@{#400576}

[oka, 2016-06-16 22:07:34]

Description was changed from

==========
Quick View: Support "No preview availale".
Also update appearance to match mocks.

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

to

==========
Quick View: Support "No preview availale", and update appearance to match mocks.

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

[oka, 2016-06-16 22:10:42]

Nit

[oka, 2016-06-16 22:10:48]

Description was changed from

==========
Quick View: Support "No preview availale", and update appearance to match mocks.

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

to

==========
Quick View: Support "No preview availale", and update appearance to match mocks.

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

[oka, 2016-06-16 22:11:56]

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

[oka, 2016-06-16 22:11:57]

PTAL

[oka, 2016-06-16 22:12:45]

Description was changed from

==========
Quick View: Support "No preview availale", and update appearance to match mocks.

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

to

==========
Quick View: Add initial implementation for "No preview availale", and update
appearance to match mocks.

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

[oka, 2016-06-16 22:13:40]

Description was changed from

==========
Quick View: Add initial implementation for "No preview availale", and update
appearance to match mocks.

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
match mocks.

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

[oka, 2016-06-16 22:22:20]

Description was changed from

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
match mocks.

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
somewhat match mocks.

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

[oka, 2016-06-16 23:08:47]

Description was changed from

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
somewhat match mocks.

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

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

[oka, 2016-06-16 23:15:04]

Description was changed from

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

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

[fukino, 2016-06-17 02:35:13]

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.html
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:27:
margin-left: 100px;
Consider RTL situation. (--webkit-margin-start)

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:33:
<!-- TODO(oka): i18n -->
use i18n-content.

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:57:
background: rgba(0,0,0,0.5);
needs spaces after ','

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:103: <div
class="content" generic-thumbnail="audio"/>
self-closing tag for div is invalid in HTML5

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.js
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:15: 
unnecessary blank line?

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:174:
['thumbnailUrl', 'contentThumbnailUrl', 'croppedThumbnailUrl'])
question: When do we need croppedThumbnailUrl?

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:174:
['thumbnailUrl', 'contentThumbnailUrl', 'croppedThumbnailUrl'])
I don't remember the exact implementation, but if we request contentThumbnailUrl
for Drive files, the file content will be downloaded. Is it intended?

[oka, 2016-06-17 23:09:15]

nit

[oka, 2016-06-17 23:12:29]

nit

[oka, 2016-06-17 23:13:55]

PTAL

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.html
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:27:
margin-left: 100px;
On 2016/06/17 02:35:13, fukino wrote:
> Consider RTL situation. (--webkit-margin-start)

Done.

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:33:
<!-- TODO(oka): i18n -->
On 2016/06/17 02:35:13, fukino wrote:
> use i18n-content.

Done.

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:103: <div
class="content" generic-thumbnail="audio"/>
On 2016/06/17 02:35:13, fukino wrote:
> self-closing tag for div is invalid in HTML5

Done.

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.js
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.js:15: 
On 2016/06/17 02:35:13, fukino wrote:
> unnecessary blank line?

Done.

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/quick_view_controller.js
(right):

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:174:
['thumbnailUrl', 'contentThumbnailUrl', 'croppedThumbnailUrl'])
On 2016/06/17 02:35:13, fukino wrote:
> question: When do we need croppedThumbnailUrl?

It worked without croppedThumbnailUrl.

https://codereview.chromium.org/2074953002/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/quick_view_controller.js:174:
['thumbnailUrl', 'contentThumbnailUrl', 'croppedThumbnailUrl'])
On 2016/06/17 02:35:13, fukino wrote:
> I don't remember the exact implementation, but if we request
contentThumbnailUrl
> for Drive files, the file content will be downloaded. Is it intended?

Avoided to use contentThumbnailUrl for Drive files.

[oka, 2016-06-17 23:16:43]

nit

[fukino, 2016-06-17 23:33:45]

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.html
(right):

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:27:
--webkit-margin-start: 100px;
nit: sort properties

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:33:
<!-- TODO(oka): i18n -->
Remove this TODO comment

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:15: <link
rel="import" type="css" href="../css/file_types.css">
Could you use a separate CSS file?
Most of the embedded images in file_types.css will not be used, so we can reduce
the package size by using a new css which includes only necessary images.

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:57:
background: rgba(0,0,0,0.5);
rgba(0, 0, 0, 0.5)

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:89: <img
class="content" src="[[image]]"/>
Still there are some self-closing tags.

[oka, 2016-06-18 01:30:13]

nit

[oka, 2016-06-18 02:48:35]

nit

[oka, 2016-06-18 02:50:38]

PTAL

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.html
(right):

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:27:
--webkit-margin-start: 100px;
On 2016/06/17 23:33:44, fukino wrote:
> nit: sort properties

Done.

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.html:33:
<!-- TODO(oka): i18n -->
On 2016/06/17 23:33:44, fukino wrote:
> Remove this TODO comment

Done.

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:15: <link
rel="import" type="css" href="../css/file_types.css">
On 2016/06/17 23:33:44, fukino wrote:
> Could you use a separate CSS file?
> Most of the embedded images in file_types.css will not be used, so we can
reduce
> the package size by using a new css which includes only necessary images.

I extracted files_quick_view.css, but loading resources fails with the following
error. I have no idea why.
chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/foreground/images/files/ui/filetype_placeholder_audio.png
Failed to load resource: net::ERR_FILE_NOT_FOUND

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:57:
background: rgba(0,0,0,0.5);
On 2016/06/17 23:33:44, fukino wrote:
> rgba(0, 0, 0, 0.5)

Done.

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:89: <img
class="content" src="[[image]]"/>
On 2016/06/17 23:33:44, fukino wrote:
> Still there are some self-closing tags.

Done.

[fukino, 2016-06-18 03:09:19]

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:15: <link
rel="import" type="css" href="../css/file_types.css">
On 2016/06/18 02:50:38, oka wrote:
> On 2016/06/17 23:33:44, fukino wrote:
> > Could you use a separate CSS file?
> > Most of the embedded images in file_types.css will not be used, so we can
> reduce
> > the package size by using a new css which includes only necessary images.
> 
> I extracted files_quick_view.css, but loading resources fails with the
following
> error. I have no idea why.
>
chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/foreground/images/files/ui/filetype_placeholder_audio.png
> Failed to load resource: net::ERR_FILE_NOT_FOUND

I haven't confirmed if it works, but there should be two options:
1) Embed images in files_quick_view.css by adding "flattenhtml="true"" in
file_manager_resources.grd
2) Include filetype_placeholder_audio.png and other icons to the package by
adding them in file_manager_resources.grd

#1 should be easier and slightly fast.

[fukino, 2016-06-18 03:12:27]

https://codereview.chromium.org/2074953002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.css
(right):

https://codereview.chromium.org/2074953002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:6:
display: block;
Please make sure to be consistent style with other similar files.
(In this file, blank line is needed between selectors, and there should be 4
spaces indent after '-webkit-image-set('

[oka, 2016-06-18 03:22:25]

Address comments

[oka, 2016-06-18 03:22:26]

Description was changed from

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview availale", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview available", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

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

[oka, 2016-06-18 03:23:18]

PTAL

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.html
(right):

https://codereview.chromium.org/2074953002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_quick_view.html:15: <link
rel="import" type="css" href="../css/file_types.css">
On 2016/06/18 03:09:19, fukino wrote:
> On 2016/06/18 02:50:38, oka wrote:
> > On 2016/06/17 23:33:44, fukino wrote:
> > > Could you use a separate CSS file?
> > > Most of the embedded images in file_types.css will not be used, so we can
> > reduce
> > > the package size by using a new css which includes only necessary images.
> > 
> > I extracted files_quick_view.css, but loading resources fails with the
> following
> > error. I have no idea why.
> >
>
chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/foreground/images/files/ui/filetype_placeholder_audio.png
> > Failed to load resource: net::ERR_FILE_NOT_FOUND
> 
> I haven't confirmed if it works, but there should be two options:
> 1) Embed images in files_quick_view.css by adding "flattenhtml="true"" in
> file_manager_resources.grd
> 2) Include filetype_placeholder_audio.png and other icons to the package by
> adding them in file_manager_resources.grd
> 
> #1 should be easier and slightly fast.

Thanks you! #1 worked well.

https://codereview.chromium.org/2074953002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.css
(right):

https://codereview.chromium.org/2074953002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:6:
display: block;
On 2016/06/18 03:12:27, fukino wrote:
> Please make sure to be consistent style with other similar files.
> (In this file, blank line is needed between selectors, and there should be 4
> spaces indent after '-webkit-image-set('

Done.

[fukino, 2016-06-18 03:27:08]

lgtm with a nit.

https://codereview.chromium.org/2074953002/diff/140001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.css
(right):

https://codereview.chromium.org/2074953002/diff/140001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:91: 
unnecessary blank line.

[oka, 2016-06-18 03:31:49]

nit

[oka, 2016-06-18 03:32:42]

Thank you for your prompt review!

https://codereview.chromium.org/2074953002/diff/140001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_quick_view.css
(right):

https://codereview.chromium.org/2074953002/diff/140001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_quick_view.css:91: 
On 2016/06/18 03:27:08, fukino wrote:
> unnecessary blank line.

Done.

[oka, 2016-06-18 03:32:53]

The CQ bit was checked by oka@chromium.org

[oka, 2016-06-18 03:32:53]

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/2074953002/#ps160001
(title: "nit")

[commit-bot: I haz the power, 2016-06-18 03:33:02]

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

[commit-bot: I haz the power, 2016-06-18 04:29:08]

Description was changed from

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview available", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview available", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

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

[commit-bot: I haz the power, 2016-06-18 04:29:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-18 04:30:30]

Description was changed from

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview available", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

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

to

==========
Quick View: Initial implementation of "No preview availale" and UI update.

Add initial implementation for "No preview available", and update appearance to
somewhat match mocks.
Also supported audio thumbnail.

Demo(internal):
https://drive.google.com/a/google.com/file/d/0B7EYjkGAjRAhQmczRld0MS1nVFU/vie...

BUG=613465
TEST=manually
third_party/closure_compiler/run_compiler

Committed: https://crrev.com/743414df7e53e88c911f469d604779e5399c85ed
Cr-Commit-Position: refs/heads/master@{#400576}
==========

[commit-bot: I haz the power, 2016-06-18 04:30:31]

Patchset 9 (id:??) landed as
https://crrev.com/743414df7e53e88c911f469d604779e5399c85ed
Cr-Commit-Position: refs/heads/master@{#400576}