Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(36)

Issue 2833413003: Reuse FileTasks when entries are not changed. (Closed)

Created:
2 years, 6 months ago by tetsui2
Modified:
2 years, 6 months ago
Reviewers:
fukino
CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Reuse FileTasks when entries are not changed. fileManagerPrivate.getFileTasks is slow. However, every time a file is copied during multi file copy, the method was called via CHANGE_THROTTLED event listener. This CL stops creating FileTasks again when file entries for the previous FileTasks are same as the current ones. Other possible solution to this problem is to change C++ interface of fileManagerPrivate.getFileTasks to support incremental update of FileTasks. BUG=462989, 712121 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2833413003 Cr-Commit-Position: refs/heads/master@{#467264} Committed: https://chromium.googlesource.com/chromium/src/+/6e537c987f4d8cc6fcfda8f80b4c3216663371d4

Patch Set 1 #

Total comments: 8

Patch Set 2 : Move areEntriesSame to common/util and rename to isSameEntries. #

Total comments: 2

Patch Set 3 : Use getter for FileTasks.entries. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -23 lines) Patch
M ui/file_manager/file_manager/common/js/util.js View 1 1 chunk +22 lines, -0 lines 0 comments Download
M ui/file_manager/file_manager/foreground/js/file_tasks.js View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M ui/file_manager/file_manager/foreground/js/task_controller.js View 1 2 3 chunks +45 lines, -23 lines 0 comments Download

Messages

Total messages: 18 (10 generated)
tetsui2
Please take a look. Suggestion: maybe discuss with mtomasz@ about incremental fileManagerPrivate.getFileTasks implementation?
2 years, 6 months ago (2017-04-25 08:21:01 UTC) #7
fukino
https://codereview.chromium.org/2833413003/diff/1/ui/file_manager/file_manager/foreground/js/file_tasks.js File ui/file_manager/file_manager/foreground/js/file_tasks.js (right): https://codereview.chromium.org/2833413003/diff/1/ui/file_manager/file_manager/foreground/js/file_tasks.js#newcode802 ui/file_manager/file_manager/foreground/js/file_tasks.js:802: * Compares if the given file entry set and ...
2 years, 6 months ago (2017-04-25 09:06:10 UTC) #8
tetsui2
https://codereview.chromium.org/2833413003/diff/1/ui/file_manager/file_manager/foreground/js/file_tasks.js File ui/file_manager/file_manager/foreground/js/file_tasks.js (right): https://codereview.chromium.org/2833413003/diff/1/ui/file_manager/file_manager/foreground/js/file_tasks.js#newcode802 ui/file_manager/file_manager/foreground/js/file_tasks.js:802: * Compares if the given file entry set and ...
2 years, 6 months ago (2017-04-26 01:58:05 UTC) #9
fukino
lgtm with a nit. https://codereview.chromium.org/2833413003/diff/20001/ui/file_manager/file_manager/foreground/js/task_controller.js File ui/file_manager/file_manager/foreground/js/task_controller.js (right): https://codereview.chromium.org/2833413003/diff/20001/ui/file_manager/file_manager/foreground/js/task_controller.js#newcode292 ui/file_manager/file_manager/foreground/js/task_controller.js:292: if (!util.isSameEntries(tasks.entries_, selection.entries)) { tasks_.entries_ ...
2 years, 6 months ago (2017-04-26 03:01:22 UTC) #10
tetsui2
Thank you! https://codereview.chromium.org/2833413003/diff/20001/ui/file_manager/file_manager/foreground/js/task_controller.js File ui/file_manager/file_manager/foreground/js/task_controller.js (right): https://codereview.chromium.org/2833413003/diff/20001/ui/file_manager/file_manager/foreground/js/task_controller.js#newcode292 ui/file_manager/file_manager/foreground/js/task_controller.js:292: if (!util.isSameEntries(tasks.entries_, selection.entries)) { On 2017/04/26 03:01:22, ...
2 years, 6 months ago (2017-04-26 07:11:17 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2833413003/40001
2 years, 6 months ago (2017-04-26 07:12:00 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6e537c987f4d8cc6fcfda8f80b4c3216663371d4
2 years, 6 months ago (2017-04-26 07:50:30 UTC) #17
yamaguchi
2 years, 4 months ago (2017-07-06 04:47:18 UTC) #18
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2966163005/ by yamaguchi@chromium.org.

The reason for reverting is: We'd like to revert this patch because this seem to
have caused regression crbug.com/738803.
2973003002 needs to be merged before this CL in order to revert another
dependent change first..

Powered by Google App Engine
This is Rietveld 408576698