Files.app: Let the thumbnail images in the preview panels managed by PreviewPanel class.

Files.app: Let the thumbnail images in the preview panels managed by PreviewPanel class.

This CL introduce PreviewPanel.Thumbnails class and make it manage the thumbnail
images in the preview panel.  PreviewPanel.Thumbnails receives the selection of
entries, and then it loads the images and show the thumbnail properly.

BUG=241693
TEST=manually
R=yoshiki@chromium.org

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

[hirono, 2013-09-09 10:28:55]

Could you take a look the CL?
Thank you very much!

[yoshiki, 2013-09-10 01:24:19]

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_selection.js (right):

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_selection.js:336:
this.previewPanel_.thumbnails.selection = thumbnailSelection;
How about passing thumbnailSelection to PreviewPanel.setSelection as the
optional 2nd argument? thumbnails.selection has been set in
PreviewPanel.setselection() once.

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:428: var height =
image.height;
Shouldn't we check width=0 or height=0?

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:446: var canvas =
this.fileManager_.document_.createElement('canvas');
You can't access 'document_' in fileManager, since it is private. Use
this.element_.ownerDocument instead.

[hirono, 2013-09-10 01:44:16]

Thanks!

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_selection.js (right):

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_selection.js:336:
this.previewPanel_.thumbnails.selection = thumbnailSelection;
On 2013/09/10 01:24:19, yoshiki wrote:
> How about passing thumbnailSelection to PreviewPanel.setSelection as the
> optional 2nd argument? thumbnails.selection has been set in
> PreviewPanel.setselection() once.

In the future, we may need to update only the thumbnails selection. E.g. when to
switch the current directory with empty selection.
I'd like to keep the capability to update both selections separately.

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:428: var height =
image.height;
On 2013/09/10 01:24:19, yoshiki wrote:
> Shouldn't we check width=0 or height=0?

Done.

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:446: var canvas =
this.fileManager_.document_.createElement('canvas');
On 2013/09/10 01:24:19, yoshiki wrote:
> You can't access 'document_' in fileManager, since it is private. Use
> this.element_.ownerDocument instead.

Done.

[yoshiki, 2013-09-10 01:45:38]

lgtm

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_selection.js (right):

https://codereview.chromium.org/23676006/diff/13001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_selection.js:336:
this.previewPanel_.thumbnails.selection = thumbnailSelection;
On 2013/09/10 01:44:17, hirono wrote:
> On 2013/09/10 01:24:19, yoshiki wrote:
> > How about passing thumbnailSelection to PreviewPanel.setSelection as the
> > optional 2nd argument? thumbnails.selection has been set in
> > PreviewPanel.setselection() once.
> 
> In the future, we may need to update only the thumbnails selection. E.g. when
to
> switch the current directory with empty selection.
> I'd like to keep the capability to update both selections separately.

Ok. Got it.

[commit-bot: I haz the power, 2013-09-10 01:47:21]

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

[commit-bot: I haz the power, 2013-09-10 02:10:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-10 02:14:40]

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

[commit-bot: I haz the power, 2013-09-10 02:24:30]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-10 02:29:48]

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

[yoshiki, 2013-09-10 02:36:27]

sorry for after lgtm, but some nits.

https://codereview.chromium.org/23676006/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23676006/diff/23001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:340: * @param
{MetadataCache} metadataCache MetadataCache.
@constructor

https://codereview.chromium.org/23676006/diff/23001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:345: this.sequence_
= 0;
Sorry for after lgtm, but could you add Object.seal(this) here?

[commit-bot: I haz the power, 2013-09-10 02:39:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[hirono, 2013-09-10 02:44:18]

Thanks!

https://codereview.chromium.org/23676006/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23676006/diff/23001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:340: * @param
{MetadataCache} metadataCache MetadataCache.
On 2013/09/10 02:36:28, yoshiki wrote:
> @constructor

Done.

https://codereview.chromium.org/23676006/diff/23001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:345: this.sequence_
= 0;
On 2013/09/10 02:36:28, yoshiki wrote:
> Sorry for after lgtm, but could you add Object.seal(this) here?

Done.

[commit-bot: I haz the power, 2013-09-10 02:44:44]

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

[commit-bot: I haz the power, 2013-09-10 02:53:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-10 03:18:37]

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

[commit-bot: I haz the power, 2013-09-10 03:27:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-10 03:29:04]

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

[commit-bot: I haz the power, 2013-09-10 03:37:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-10 06:56:31]

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

[hirono, 2013-09-10 08:48:06]

Committed patchset #4 manually as r222223 (presubmit successful).