Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
486783006:
Files.app: Update the list partially when changed (Closed)
|
Created:
6 years, 4 months ago by yoshiki Modified:
6 years, 4 months ago CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFiles.app: Update the list partially when changed
This patch makes the list redraw partially instead of full-redraw when some contents are changed in the directory. This should improve the performance of redraw.
In addition, as the result of the fix, the cache is not flashed on partial redraw, and the issue 374713 has gone.
BUG=374713
TEST=manaul tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291366
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed the comments #
Total comments: 6
Patch Set 3 : Addressed the comments #Patch Set 4 : Addressed the comments #Patch Set 5 : Fix the test filure. #Patch Set 6 : Fix the test filure & rebase #
Messages
Total messages: 18 (0 generated)
@hirono, PTAL. Thanks.
@fukino, please take a look because this may conflict with your current work.
This CL eliminates flickers on copy in Drive! https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:671: var changed = false; This variable seems to be unused. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:176: this.partialUpdate_(result.entries, result.failureUrls); nit: Could you add a comment about that the failure of converting to entry means that the entry was removed? To me, this was not obvious. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:437: * Adds/remove/update items of file list. nit: remove and update should also have present form. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:454: Promise.resolve().then(function() { Is this promise necessary? https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:476: console.log('finish'); This might be a remaining debug log.
Sorry for late. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:666: * Adds/remove/update items of file list. nit: removes/updates ? https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:456: this.partialUpdate_(changedEntries, removedUrls, callback); callback is undefined? https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:534: }.bind(this); We can remove bind(this).
@hirono, @fukino, PTAL. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:666: * Adds/remove/update items of file list. On 2014/08/19 06:51:27, hirono wrote: > nit: removes/updates ? Done. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:671: var changed = false; On 2014/08/19 06:01:27, fukino wrote: > This variable seems to be unused. Done. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:176: this.partialUpdate_(result.entries, result.failureUrls); On 2014/08/19 06:01:27, fukino wrote: > nit: Could you add a comment about that the failure of converting to entry means > that the entry was removed? To me, this was not obvious. Done. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:437: * Adds/remove/update items of file list. On 2014/08/19 06:01:28, fukino wrote: > nit: remove and update should also have present form. Done. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:454: Promise.resolve().then(function() { On 2014/08/19 06:01:27, fukino wrote: > Is this promise necessary? Yes, I added the comment. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:456: this.partialUpdate_(changedEntries, removedUrls, callback); On 2014/08/19 06:51:27, hirono wrote: > callback is undefined? Done. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:476: console.log('finish'); On 2014/08/19 06:01:27, fukino wrote: > This might be a remaining debug log. Done. https://codereview.chromium.org/486783006/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:534: }.bind(this); On 2014/08/19 06:51:27, hirono wrote: > We can remove bind(this). Done.
https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:702: if (removedUrls > 0) removedUrls.length https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:481: var onFailure = function() { nit: var onFailure = onFinish; ? https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:485: var onCancelled = function() { nit: var onCancelled = onFinish; ?
On 2014/08/20 05:26:47, hirono wrote: > https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... > File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): > > https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... > ui/file_manager/file_manager/foreground/js/directory_contents.js:702: if > (removedUrls > 0) > removedUrls.length > > https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... > File ui/file_manager/file_manager/foreground/js/directory_model.js (right): > > https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... > ui/file_manager/file_manager/foreground/js/directory_model.js:481: var onFailure > = function() { > nit: var onFailure = onFinish; ? > > https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... > ui/file_manager/file_manager/foreground/js/directory_model.js:485: var > onCancelled = function() { > nit: var onCancelled = onFinish; ? lgtm except for hirono@'s points.
@hirono, PTAL https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_contents.js (right): https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_contents.js:702: if (removedUrls > 0) On 2014/08/20 05:26:47, hirono wrote: > removedUrls.length Done. https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:481: var onFailure = function() { On 2014/08/20 05:26:47, hirono wrote: > nit: var onFailure = onFinish; ? I'd like to keep this as it is, to keep consistency with onCompleted.
lgtm with a nit. https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/directory_model.js (right): https://codereview.chromium.org/486783006/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/directory_model.js:481: var onFailure = function() { On 2014/08/20 06:21:32, yoshiki wrote: > On 2014/08/20 05:26:47, hirono wrote: > > nit: var onFailure = onFinish; ? > > I'd like to keep this as it is, to keep consistency with onCompleted. Got it. May be we can remove .bind call.
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/486783006/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/486783006/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Message was sent while issue was closed.
Committed patchset #6 (140001) as 291366 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
