Integrate ListThumbnailLoader to list view and grid view.

Integrate ListThumbnailLoader to list view and grid view.

This CL also contains following fixes.
* Add error handling to ThumbnailLoader.loadAsDataUrl.
* Removed unnecessary document argument from ListThumbnailLoader.

BUG=438050
TEST=manually tested;Open a folder which contains photos in it with list view and grid view.

Committed: https://crrev.com/e3ef182d6c4a760249728211a4278a1b224c4b50
Cr-Commit-Position: refs/heads/master@{#316185}

[yawano, 2015-02-10 10:34:18]

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

[yawano, 2015-02-10 10:34:18]

Sorry for a large CL. PTAL. Thank you!

[fukino, 2015-02-10 14:46:14]

Thanks!

- Please add TEST in description. How can we verify this patch?
- It seems master branch have some divergence. Could you rebase?

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_table.js (right):

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:375:
FileTable.prototype.onScroll_ = function(event) {
Before the user starts scrolling, setHighPriorityRange won't be called. Is it
OK?

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:379: var viewport =
this.list_.getItemsInViewPort(
getItemsInViewPort is cpu-consuming.
Could you avoid calling this too frequently?
RateLimiter might come in handy here.

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:411: for (var i = 0;
i < this.dataModel.length; i++) {
It seems loading thumbnails for n files takes O(n^2)?
I thought the ListThumbnailLoader had enough information to pass the index of
loaded item. Is that correct?

[yawano, 2015-02-12 03:28:50]

Sorry for missing test description. PTAL. Thank you!

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_table.js (right):

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:375:
FileTable.prototype.onScroll_ = function(event) {
On 2015/02/10 14:46:14, fukino wrote:
> Before the user starts scrolling, setHighPriorityRange won't be called. Is it
> OK?

Done. Added calls to run update high priority range at redraw and
setListThumbnailLoader.

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:379: var viewport =
this.list_.getItemsInViewPort(
On 2015/02/10 14:46:14, fukino wrote:
> getItemsInViewPort is cpu-consuming.
> Could you avoid calling this too frequently?
> RateLimiter might come in handy here.

Done. Added RateLimiter.

https://codereview.chromium.org/912783002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:411: for (var i = 0;
i < this.dataModel.length; i++) {
On 2015/02/10 14:46:14, fukino wrote:
> It seems loading thumbnails for n files takes O(n^2)?
> I thought the ListThumbnailLoader had enough information to pass the index of
> loaded item. Is that correct?

Yes, this takes O(n) for 1 file.
Actually, ListThumbnailLoader loads thumbnails based on index, so we can use it
to find corresponding element in the UI.
However, the problem is that the index is an index of 'before' the task is
euqueued. If some change had happened in the list during thumbnail fetch, the
index becomes invalid.
The UI can check the index passed from ListThumbnailLoader is correct or not
with cost of O(1) (i.e. checks the url of the item). We can fallback to O(n)
update when the index is invalid.

[yawano, 2015-02-12 05:16:57]

Added index property to thumbnail loaded event, and use it in onThumbnailLoaded.
PTAL. Thank you!

[fukino, 2015-02-12 07:25:01]

Thank you!

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/css/file_manager.css:812:
background-position: center;
nit: posision < size in alphabetical order

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js
(right):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:156:
ListThumbnailLoader.prototype.continue_ = function() {
As far as I checked, thumbnails outside the viewport were not loaded at all.
Could you double check?

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:118:
FileGrid.prototype.mergeItems = function() {
As the startIndex and endIndex are in use now, could you declare them explicitly
in parameter list?

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:118:
FileGrid.prototype.mergeItems = function() {
For grid item, we use mergeItems to call setHighPriorityRange, but for list
view, we use 'scroll' event to call setHighPriorityRange. Can we use consistent
way to call setHighPriorityRange?

[fukino, 2015-02-12 07:35:16]

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (left):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:225: * @param
{boolean} animation Whther to use fadein animation or not.
fadein animation was removed. Is it intended modification?

[yawano, 2015-02-12 09:38:24]

Thank you for the review! PTAL.

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/css/file_manager.css (right):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/css/file_manager.css:812:
background-position: center;
On 2015/02/12 07:25:01, fukino wrote:
> nit: posision < size in alphabetical order

Done.

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js
(right):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:156:
ListThumbnailLoader.prototype.continue_ = function() {
On 2015/02/12 07:25:01, fukino wrote:
> As far as I checked, thumbnails outside the viewport were not loaded at all.
> Could you double check?

Actually, it worked. However, number of prefetch tasks is small and it may have
caused to feel slow (or prefetch is not working).
I increased number of prefetch tasks (also increased cache size and maximum
number of active tasks).

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (left):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:225: * @param
{boolean} animation Whther to use fadein animation or not.
On 2015/02/12 07:35:16, fukino wrote:
> fadein animation was removed. Is it intended modification?

Yes. This is intentional. Since we use css-transition for the new
implementation, we don't need this parameter any more.

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/912783002/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:118:
FileGrid.prototype.mergeItems = function() {
On 2015/02/12 07:25:01, fukino wrote:
> For grid item, we use mergeItems to call setHighPriorityRange, but for list
> view, we use 'scroll' event to call setHighPriorityRange. Can we use
consistent
> way to call setHighPriorityRange?

Done. Changed grid view to onScroll event based implementation.

[yawano, 2015-02-13 02:14:26]

Added FileTableList. PTAL. Thank you!

[fukino, 2015-02-13 04:49:43]

Thank you! lgtm with nits.

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:104: if
(this.beginIndex_ < this.endIndex_) {
Is this if condition necessary?
When the list becomes empty and beginIndex==endIndex==0, I think we can still
call setHighPriorityRange(0, 0). (And may be it's more consistent)

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_table.js (right):

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:265:
self.beginIndex_ = -1;
When the list gets empty, mergeItems(0, 0) is called. So, I think 0 sounds more
natural as the initial value.

https://codereview.chromium.org/912783002/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/912783002/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_manager.js:888:
this.directoryModel_.getFileList(), this.metadataCache_);
this.directoryModel_.getFileList() has type {cr.ui.ArrayDataModel}, compile
error will occur here.

[yawano, 2015-02-13 05:15:54]

New patchsets have been uploaded after l-g-t-m from fukino@chromium.org

[yawano, 2015-02-13 05:21:45]

Thank you for the review!

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:104: if
(this.beginIndex_ < this.endIndex_) {
On 2015/02/13 04:49:43, fukino wrote:
> Is this if condition necessary?
> When the list becomes empty and beginIndex==endIndex==0, I think we can still
> call setHighPriorityRange(0, 0). (And may be it's more consistent)

Done. Yes, actually we check arguments inside setHighPriorityRange, so it
doesn't have any problem to call this with (0,0).

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/ui/file_table.js (right):

https://codereview.chromium.org/912783002/diff/140001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/ui/file_table.js:265:
self.beginIndex_ = -1;
On 2015/02/13 04:49:43, fukino wrote:
> When the list gets empty, mergeItems(0, 0) is called. So, I think 0 sounds
more
> natural as the initial value.

Done.

https://codereview.chromium.org/912783002/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/912783002/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/file_manager.js:888:
this.directoryModel_.getFileList(), this.metadataCache_);
On 2015/02/13 04:49:43, fukino wrote:
> this.directoryModel_.getFileList() has type {cr.ui.ArrayDataModel}, compile
> error will occur here.

Done.

[yawano, 2015-02-13 05:21:50]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-13 05:23:02]

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

[commit-bot: I haz the power, 2015-02-13 06:26:05]

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

[commit-bot: I haz the power, 2015-02-13 06:26:07]

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

[yawano, 2015-02-13 07:03:15]

I forgot to also change constructor call in test after I changed interface of
ListThumbnailLoader constructor. PTAL. Thank you!

[fukino, 2015-02-13 07:31:27]

On 2015/02/13 07:03:15, yawano wrote:
> I forgot to also change constructor call in test after I changed interface of
> ListThumbnailLoader constructor. PTAL. Thank you!

still lgtm

[yawano, 2015-02-13 07:32:00]

On 2015/02/13 07:31:27, fukino wrote:
> On 2015/02/13 07:03:15, yawano wrote:
> > I forgot to also change constructor call in test after I changed interface
of
> > ListThumbnailLoader constructor. PTAL. Thank you!
> 
> still lgtm

Thank you!

[yawano, 2015-02-13 07:32:04]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-13 07:32:28]

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

[commit-bot: I haz the power, 2015-02-13 07:35:18]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-02-13 07:35:58]

Patchset 12 (id:??) landed as
https://crrev.com/e3ef182d6c4a760249728211a4278a1b224c4b50
Cr-Commit-Position: refs/heads/master@{#316185}