Initial implementation for ListThumbnailLoader.

Initial implementation for ListThumbnailLoader.

ListThumbnailLoader is a thumbnail loader for list style ui.
For the images in the viewport, either thumbnailLoaded event is dispatched or getThumbnailFromCache will return from cache.

BUG=438050
TEST=out/Release/browser_tests --gtest_filter=FileManagerJsTest.ListThumbnailLoader

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

[yawano, 2015-01-30 10:50:38]

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

[yawano, 2015-01-30 10:50:38]

PTAL. Thank you!

[fukino, 2015-01-30 11:35:41]

Thanks!

Could you provide brief explanation about what is done and what needs to be
done?
A short bullet list (with checkmark?) is sufficient.

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

https://codereview.chromium.org/885323002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:80:
ListThumbnailLoader.prototype.setViewport = function(beginIndex, endIndex) {
Please avoid using 'viewport' to isolate view-related knowledge from this class
explicitly.

That is, "Viewport should be prioritized" is a knowledge of view.
"certain range in the list should be prioritized" is enough information for this
class. This class doesn't care if the range is viewport or not.

How about 'priority range', 'priority window', or something?

https://codereview.chromium.org/885323002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:96:
ListThumbnailLoader.prototype.getThumbnailFromCache = function(entry) {
optional: 'FromCache' might be redundant, as there's no API to get thumbnail
without cache.

[yawano, 2015-02-02 02:18:25]

Thank you for the review! I added TODO list in the class comment. PTAL.

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

https://codereview.chromium.org/885323002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:80:
ListThumbnailLoader.prototype.setViewport = function(beginIndex, endIndex) {
On 2015/01/30 11:35:41, fukino wrote:
> Please avoid using 'viewport' to isolate view-related knowledge from this
class
> explicitly.
> 
> That is, "Viewport should be prioritized" is a knowledge of view.
> "certain range in the list should be prioritized" is enough information for
this
> class. This class doesn't care if the range is viewport or not.
> 
> How about 'priority range', 'priority window', or something?

Done. Renamed setViewport to setHighPriorityRange in order to explicitly say
that the range is set as high priority.

https://codereview.chromium.org/885323002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:96:
ListThumbnailLoader.prototype.getThumbnailFromCache = function(entry) {
On 2015/01/30 11:35:41, fukino wrote:
> optional: 'FromCache' might be redundant, as there's no API to get thumbnail
> without cache.

Yes, actually this class does not provide a way to fetch thumbnail without
caching it. However, I added "FromCache" for people who don't know this class
well.

If the method name is getThumbnail, people who see this method might be
confused. Someone may think that this method returns null only when it fails to
fetch thumbnail with some fatal error, e.g. the image file does not exist.

I wanted to explicitly say that this method also fails when the thumbnail is not
in cache.

WDYT?

[fukino, 2015-02-02 18:55:00]

Thanks! lgtm with nits.

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

https://codereview.chromium.org/885323002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:96:
ListThumbnailLoader.prototype.getThumbnailFromCache = function(entry) {
On 2015/02/02 02:18:24, yawano wrote:
> On 2015/01/30 11:35:41, fukino wrote:
> > optional: 'FromCache' might be redundant, as there's no API to get thumbnail
> > without cache.
> 
> Yes, actually this class does not provide a way to fetch thumbnail without
> caching it. However, I added "FromCache" for people who don't know this class
> well.
> 
> If the method name is getThumbnail, people who see this method might be
> confused. Someone may think that this method returns null only when it fails
to
> fetch thumbnail with some fatal error, e.g. the image file does not exist.
> 
> I wanted to explicitly say that this method also fails when the thumbnail is
not
> in cache.
> 
> WDYT?

OK! sgtm.

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js
(right):

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:26: * @param
{!Document} document Document.
Do we need |document| passed as a parameter?

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:27: * @param
{Function=} opt_thumbnailLoader A constructor of thumbnail loader.
thumbnailLoader sounds like an instance of ThumbnailLoader. how about
thumbnailLoaderConstructor.
Alternatively, how about pass an instance of thumbnail loader?

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:36:
this.dataModel_ = dataModel;
Please add type annotations for member variables.

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:63: * Number
of maximum active tasks.
@const {number}

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:212:
ThumbnailLoader.LoadTarget.CONTENT_METADATA,
These LoadTargets are same as ThumbnailLoader's default. Can be removed from
argument list.

[yawano, 2015-02-03 02:50:57]

Thank you for the review!

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js
(right):

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:26: * @param
{!Document} document Document.
On 2015/02/02 18:55:00, fukino wrote:
> Do we need |document| passed as a parameter?

document is used to create an image element and canvas element around line 247.

After we added a change to make ThumbnailLoader directly return dataUrl of an
image instead of attaching it to an image, we can remove document.

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:27: * @param
{Function=} opt_thumbnailLoader A constructor of thumbnail loader.
On 2015/02/02 18:54:59, fukino wrote:
> thumbnailLoader sounds like an instance of ThumbnailLoader. how about
> thumbnailLoaderConstructor.
> Alternatively, how about pass an instance of thumbnail loader?

Done. Renamed to thumbnailLoaderConstructor. As far as my understanding, one
thumbnail loader instance corresponds to one thumbnail load task. So we need to
have it as a constructor.

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:36:
this.dataModel_ = dataModel;
On 2015/02/02 18:55:00, fukino wrote:
> Please add type annotations for member variables.

Done.

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:63: * Number
of maximum active tasks.
On 2015/02/02 18:54:59, fukino wrote:
> @const {number}

Done.

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:212:
ThumbnailLoader.LoadTarget.CONTENT_METADATA,
On 2015/02/02 18:55:00, fukino wrote:
> These LoadTargets are same as ThumbnailLoader's default. Can be removed from
> argument list.

Done.

[yawano, 2015-02-03 02:53:38]

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js
(right):

https://codereview.chromium.org/885323002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/list_thumbnail_loader.js:26: * @param
{!Document} document Document.
On 2015/02/03 02:50:57, yawano wrote:
> On 2015/02/02 18:55:00, fukino wrote:
> > Do we need |document| passed as a parameter?
> 
> document is used to create an image element and canvas element around line
247.
> 
> After we added a change to make ThumbnailLoader directly return dataUrl of an
> image instead of attaching it to an image, we can remove document.

Sorry. Not image element. It is div element.

[yawano, 2015-02-03 02:58:08]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-03 02:59:01]

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

[commit-bot: I haz the power, 2015-02-03 03:02:45]

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

[commit-bot: I haz the power, 2015-02-03 03:02:46]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yawano, 2015-02-03 03:20:56]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-03 03:22:14]

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

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

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

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

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (None)

[yawano, 2015-02-03 07:42:33]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-03 07:42:52]

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

[commit-bot: I haz the power, 2015-02-03 07:43:32]

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

[commit-bot: I haz the power, 2015-02-03 07:43:34]

Failed to apply patch for
chrome/browser/chromeos/file_manager/file_manager_jstest.cc:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/browser/chromeos/file_manager/file_manager_jstest.cc:134
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/chromeos/file_manager/file_manager_jstest.cc'
with conflicts.
  U chrome/browser/chromeos/file_manager/file_manager_jstest.cc

Patch:       chrome/browser/chromeos/file_manager/file_manager_jstest.cc
Index: chrome/browser/chromeos/file_manager/file_manager_jstest.cc
diff --git a/chrome/browser/chromeos/file_manager/file_manager_jstest.cc
b/chrome/browser/chromeos/file_manager/file_manager_jstest.cc
index
f64c0cacd675bda12e36d70ca1855375504c5940..3099865de9817cafdc33100c52cea5201d83b8c3
100644
--- a/chrome/browser/chromeos/file_manager/file_manager_jstest.cc
+++ b/chrome/browser/chromeos/file_manager/file_manager_jstest.cc
@@ -134,3 +134,8 @@ IN_PROC_BROWSER_TEST_F(FileManagerJsTest, MetadataCacheSet)
{
   RunTest(base::FilePath(FILE_PATH_LITERAL(
       "foreground/js/metadata/metadata_cache_set_unittest.html")));
 }
+
+IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ListThumbnailLoader) {
+  RunTest(base::FilePath(
+      FILE_PATH_LITERAL("foreground/js/list_thumbnail_loader_unittest.html")));
+}

[commit-bot: I haz the power, 2015-02-03 07:43:39]

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

[yawano, 2015-02-03 08:04:41]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-02-03 08:05:03]

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

[commit-bot: I haz the power, 2015-02-03 08:54:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-03 08:55:07]

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