New issue
Advanced search Search tips

Issue 879958 link

Starred by 1 user
Status: Started
Owner:
tapted@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 879959



Sign in to add a comment

file_manager ImageLoader needs TLC

Project Member Reported by tapted@chromium.org, Sep 3 
Chrome Version       : 70.0.3534.4
OS Version: Chrome

Gallery should use ImageLoader more, but its API is not clean.

For example:
 - server-side, orientation is not part of the cache key
 - a null timestamp disables caching server-side, but disables *expiration* client-side
 - tasks that do not pass opt_isValid are never removed from the pending task map on the client
 - cancel messages for a task can be sent multiple times
 - Large parts of the API are undocumented (e.g. `crop`)
 - ImageLoaderClient.prototype.handleMessage_ is dead code
 - Many Closure annotations use "{Object}" which effectively disables type checking


This affects thumbnail loading in the Files App as well as Gallery. (Gallery currently only uses ImageLoader to load RAW /Piex images).

Comment 1 by tapted@chromium.org, Sep 3

Blocking: 879959
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea7811b3b120de2c8d844d36aaf0a168319f3005

commit ea7811b3b120de2c8d844d36aaf0a168319f3005
Author: Trent Apted <tapted@chromium.org>
Date: Wed Sep 12 06:37:45 2018

CrOS file_manager: Use a formal parameter for LoadImageRequest/Response

file_manager component apps share an `ImageLoader` extension that acts
as a persistent cache, and services thumbnail requests over an
extension messaging API. Gallery should use this for loading thumbnails
but currently does not.

This API is currently not well documented - a request is built up in
different ways, and exactly what gets passed to the isolated context
is unclear. Existing closure annotations are misleading and lack
coverage. Feeding through type information exposed a big memory leak
( https://crbug.com/881680 ): the code that was meant to remove items
from successful ImageLoaderClient::tasks_ does not compile and was just
dead code.

This CL:
 - Adds ES6 classes to fully describe the request/response messages
   passed over the extension messaging API.
 - Removes all `{Object}` closure annotations from image_loader -
   these effectively disable type checking.
 - Encapsulates the |url| into the request throughout the call stack.
 - Formalizes parameters, adding checks were necessary.
 - Replace duplicate 'success'/'error' strings with an enum.
 - Incorporates |orientation| into the extension-side cache key by
   consolidating code (previously this only formed part of the cache
   key on the client side).

Note ImageLoaderClient currently does (effectively)
  var request = opt_options || {};
  request.url = url;

So the main API change comes from combining |options| and |url| in a
single LoadImageRequest that's plumbed through, is never undefined, and
always has a type.

Bug: 879958
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Id22372f2d8aef7ba916fe6747ce8278e48dc7041
Reviewed-on: https://chromium-review.googlesource.com/1198673
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590605}
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/file_manager/foreground/js/main_scripts.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/file_manager/foreground/js/metadata/image_orientation.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/file_manager/foreground/js/quick_view_controller.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/file_manager/foreground/js/thumbnail_loader.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/file_manager/foreground/js/thumbnail_loader_unittest.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/gallery/js/gallery_scripts.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/gallery/js/image_editor/image_loader.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/BUILD.gn
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/background_scripts.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/cache.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/cache_unittest.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/image_loader.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/image_loader_client.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/image_loader_client_unittest.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/image_loader_unittest.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/image_loader_util.js
[add] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/load_image_request.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/piex_loader.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/request.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/image_loader/scheduler.js
[modify] https://crrev.com/ea7811b3b120de2c8d844d36aaf0a168319f3005/ui/file_manager/video_player/js/video_player_scripts.js

Comment 3 by tapted@chromium.org, Sep 12 

note a TODO from #c2 - rename 'options' to 'request' in various places.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 14 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0777cf7819fb7511e92e4b058e2f51ce303b01cb

commit 0777cf7819fb7511e92e4b058e2f51ce303b01cb
Author: Trent Apted <tapted@chromium.org>
Date: Fri Sep 14 00:50:21 2018

CrOS Files: Rename options -> request appropriately in ImageLoaderUtil

Mechanical follow-up to r590605. Also eliminate `var` usage.
Includes a cl format.

Bug: 879958
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I8239730a3118a41ee31aacfc2752f64bd88c173f
Reviewed-on: https://chromium-review.googlesource.com/1223126
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591238}
[modify] https://crrev.com/0777cf7819fb7511e92e4b058e2f51ce303b01cb/ui/file_manager/image_loader/image_loader_util.js
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 19 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0f36c578070e13bae2ff3e5722b3740df3fd709

commit a0f36c578070e13bae2ff3e5722b3740df3fd709
Author: Trent Apted <tapted@chromium.org>
Date: Wed Sep 19 01:38:41 2018

CrOS Files app: Typecheck the image_loader module with strictCheckTypes.

Bug: 879958, 882750
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ia5992149ad55370a912ed02271dbaa1a93332ff1
Reviewed-on: https://chromium-review.googlesource.com/1226483
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592298}
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/third_party/closure_compiler/README.chromium
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/third_party/closure_compiler/compile_js.gni
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/third_party/closure_compiler/js_unit_tests.gni
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/ui/file_manager/file_manager/common/js/BUILD.gn
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/ui/file_manager/file_manager/common/js/file_type.js
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/ui/file_manager/file_manager/common/js/metrics_base.js
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/ui/file_manager/image_loader/BUILD.gn
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/ui/file_manager/image_loader/piex_loader.js
[modify] https://crrev.com/a0f36c578070e13bae2ff3e5722b3740df3fd709/ui/file_manager/image_loader/request.js
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/689a69cdc4828221f0c268a49b4eab06e949d92f

commit 689a69cdc4828221f0c268a49b4eab06e949d92f
Author: Trent Apted <tapted@chromium.org>
Date: Fri Sep 28 01:21:01 2018

Use strictCheckTypes for image_loader unittests.

 - capture unittest_util.js's use of commandLinePrivate
 - audit remaining things using command_line_private.js unnecessarily
   (Only files in file_manager/common/js and file_manager/foreground/js
    use it directly.)
 - Remove an annoying dep that piex_loader_unittest.js has on webui
   (Use a custom element instead: cr/ui.js dislikes strictCheckTypes.).

Bug: 882750, 879958
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I70c9e16b1ba7b9af64e0ce01b7177ef02500c948
Reviewed-on: https://chromium-review.googlesource.com/1245079
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594945}
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/audio_player/js/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/file_manager/background/js/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/file_manager/common/js/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/file_manager/common/js/unittest_util.js
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/file_manager/foreground/js/metadata/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/file_manager/foreground/js/ui/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/image_loader/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/image_loader/piex_loader_unittest.js
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/video_player/js/BUILD.gn
[modify] https://crrev.com/689a69cdc4828221f0c268a49b4eab06e949d92f/ui/file_manager/video_player/js/cast/BUILD.gn
►

Sign in to add a comment