Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
507293002:
Enrich fileBrowserPrivate.onFileTransfersUpdated event to support displaying total number of jobs. (Closed)
|
Created:
6 years, 3 months ago by iseki Modified:
6 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, yoshiki+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionEnrich fileBrowserPrivate.onFileTransfersUpdated event to support displaying total number of jobs.
* "added" type is added
* "num_total_jobs" member is added
* and this is for displaying the total number of jobs in Chrome OS file manager
BUG=400636
TEST=
manually
1. Copy the local file to drive directory.
2. Observe the status of syncing status.
Committed: https://crrev.com/4c0895fd17dac300cfbeccd0bf236b58a5910a36
Cr-Commit-Position: refs/heads/master@{#293257}
Patch Set 1 #
Total comments: 17
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : #
Total comments: 30
Patch Set 5 : #
Total comments: 14
Patch Set 6 : #
Total comments: 14
Patch Set 7 : #
Total comments: 24
Patch Set 8 : #
Total comments: 13
Patch Set 9 : #
Total comments: 10
Patch Set 10 : #
Total comments: 10
Patch Set 11 : #
Total comments: 16
Patch Set 12 : #Patch Set 13 : #
Total comments: 8
Patch Set 14 : #
Total comments: 12
Patch Set 15 : #
Total comments: 2
Patch Set 16 : #Messages
Total messages: 47 (6 generated)
iseki@chromium.org changed reviewers: + hirono@chromium.org
Please take a look :)
https://codereview.chromium.org/507293002/diff/1/chrome/browser/chromeos/driv... File chrome/browser/chromeos/drive/job_scheduler.cc (right): https://codereview.chromium.org/507293002/diff/1/chrome/browser/chromeos/driv... chrome/browser/chromeos/drive/job_scheduler.cc:714: base::TimeDelta delay = base::TimeDelta::FromMilliseconds(500); It looks to affect actual performance. Can we delay events instead of jobs? https://codereview.chromium.org/507293002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/file_browser_private.idl (right): https://codereview.chromium.org/507293002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_browser_private.idl:327: double? num_total_jobs; The reason why we use double for number of bytes is that the size of long is not enough for bytes numbers. It is OK to use long for the number of jobs. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:89: this.removeItem_(status,'sync'); Can we just update the item? https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:89: this.removeItem_(status,'sync'); Please make the 'sync' constant. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:113: * @patam {String} label which indicate a lamp of process. @patam -> @param String -> string String is primitive in our style. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:113: * @patam {String} label which indicate a lamp of process. The style of JSDoc for parameters is: @param {type} veriableName Note. The note part starts with upper case. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:116: DriveSyncHandler.prototype.updateItem_ = function(status, label) { The label sounds like a text displayed in UI element. Please use 'id' instead. https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi... https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:129: item.message = strf('SYNC_FILE_NAME', status.num_total_jobs + ' items'); 'items' should be a part of locale string. This causes "同期中 3 items" The text "items" will not be translated. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:157: * @patam {String} label which indicate a lamp of process. ditto.
Thank you for your review. https://codereview.chromium.org/507293002/diff/1/chrome/browser/chromeos/driv... File chrome/browser/chromeos/drive/job_scheduler.cc (right): https://codereview.chromium.org/507293002/diff/1/chrome/browser/chromeos/driv... chrome/browser/chromeos/drive/job_scheduler.cc:714: base::TimeDelta delay = base::TimeDelta::FromMilliseconds(500); On 2014/08/27 08:37:20, hirono wrote: > It looks to affect actual performance. > Can we delay events instead of jobs? Done. https://codereview.chromium.org/507293002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/file_browser_private.idl (right): https://codereview.chromium.org/507293002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/file_browser_private.idl:327: double? num_total_jobs; On 2014/08/27 08:37:20, hirono wrote: > The reason why we use double for number of bytes is that the size of long is not > enough for bytes numbers. > It is OK to use long for the number of jobs. > Done. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:89: this.removeItem_(status,'sync'); On 2014/08/27 08:37:21, hirono wrote: > Can we just update the item? Done. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:89: this.removeItem_(status,'sync'); On 2014/08/27 08:37:20, hirono wrote: > Please make the 'sync' constant. Done. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:113: * @patam {String} label which indicate a lamp of process. On 2014/08/27 08:37:20, hirono wrote: > The style of JSDoc for parameters is: > > @param {type} veriableName Note. > > The note part starts with upper case. Done. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:113: * @patam {String} label which indicate a lamp of process. On 2014/08/27 08:37:21, hirono wrote: > @patam -> @param > String -> string > String is primitive in our style. > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... Done. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:116: DriveSyncHandler.prototype.updateItem_ = function(status, label) { On 2014/08/27 08:37:20, hirono wrote: > The label sounds like a text displayed in UI element. > Please use 'id' instead. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi... Done. https://codereview.chromium.org/507293002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/drive_sync_handler.js:157: * @patam {String} label which indicate a lamp of process. On 2014/08/27 08:37:20, hirono wrote: > ditto. Done.
https://codereview.chromium.org/507293002/diff/20001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/507293002/diff/20001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:504: Syncing <ph name="FILE_NAME">$1<ex>movie.avi</ex></ph> items... Please update the placeholder name and its example. https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/job_list.h (right): https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/job_list.h:47: STATE_NEW, Why is this state needed? https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:201: void SendDriveFileTransferEventDelay( Could you integrate this method with SendDriveFileTransferEvent? Currently the difference between SendDriveFileTransferDelay(is_delayed = false) and SendDriveFileTransferEvent is not clear. https://codereview.chromium.org/507293002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:71: for (var url in this.items_) { Please update this.
Thank you for your review :) https://codereview.chromium.org/507293002/diff/20001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/507293002/diff/20001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:504: Syncing <ph name="FILE_NAME">$1<ex>movie.avi</ex></ph> items... On 2014/08/28 08:31:38, hirono wrote: > Please update the placeholder name and its example. Done. https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/job_list.h (right): https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/job_list.h:47: STATE_NEW, On 2014/08/28 08:31:38, hirono wrote: > Why is this state needed? Done. https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:201: void SendDriveFileTransferEventDelay( On 2014/08/28 08:31:38, hirono wrote: > Could you integrate this method with SendDriveFileTransferEvent? > Currently the difference between SendDriveFileTransferDelay(is_delayed = false) > and SendDriveFileTransferEvent is not clear. Done. https://codereview.chromium.org/507293002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:71: for (var url in this.items_) { On 2014/08/28 08:31:38, hirono wrote: > Please update this. Done.
If there is no application window, the sync status is displayed in the desktop notifications. Please also check: https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi... https://codereview.chromium.org/507293002/diff/60001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/507293002/diff/60001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:504: Syncing <ph name="NUMBER_OF_FILE">$1<ex>7</ex></ph> items... nit: NUMBER_OF_FILE -> NUMBER_OF_FILES https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/job_list.h (right): https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/job_list.h:93: // Total number of job. nit: number of job -> number of jobs https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:629: DriveJobInfoWithStatus job_info_with_status = nit: const https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:647: DriveJobInfoWithStatus job_info_with_status = drive_jobs_[job_info.job_id]; nit: const https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:664: DriveJobInfoWithStatus job_info_with_status = drive_jobs_[job_info.job_id]; nit: const https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:675: bool is_delayed) { I think should_delay sounds more natural. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:693: if (call_time < last_post_delayed_task_) { nit: We don't add {} if the body of if statements is 1 line. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:694: return; It can skip an event even if the event is reporting sync error. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:22: * Map of file urls and progress center items. Please update the comment. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:26: this.item_ = new ProgressCenterItem(); Assign ID with DriveSyncHandler.PROGRESS_ITEM_ID_PREFIX here. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:64: if (this.item_.id === '') { Maybe we should check this.item_ !== null. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:107: if (!this.item_) { Can the item be null? https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:112: if (!this.item_.id) { Maybe we should check this.item_ !== null. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:126: console.log(this.item_.message); Please remove the debug log. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:160: delete this.item_; We usually do this.item_ = null, instead of deleting it. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:182: if (!this.item_.id) { Maybe we should check this.item_ !== null.
Thank you for your review. https://codereview.chromium.org/507293002/diff/60001/chrome/app/chromeos_stri... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/507293002/diff/60001/chrome/app/chromeos_stri... chrome/app/chromeos_strings.grdp:504: Syncing <ph name="NUMBER_OF_FILE">$1<ex>7</ex></ph> items... On 2014/08/29 03:34:16, hirono wrote: > nit: NUMBER_OF_FILE -> NUMBER_OF_FILES Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/job_list.h (right): https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/job_list.h:93: // Total number of job. On 2014/08/29 03:34:16, hirono wrote: > nit: number of job -> number of jobs Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:629: DriveJobInfoWithStatus job_info_with_status = On 2014/08/29 03:34:16, hirono wrote: > nit: const Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:647: DriveJobInfoWithStatus job_info_with_status = drive_jobs_[job_info.job_id]; On 2014/08/29 03:34:16, hirono wrote: > nit: const Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:664: DriveJobInfoWithStatus job_info_with_status = drive_jobs_[job_info.job_id]; On 2014/08/29 03:34:16, hirono wrote: > nit: const Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:675: bool is_delayed) { On 2014/08/29 03:34:16, hirono wrote: > I think should_delay sounds more natural. Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:693: if (call_time < last_post_delayed_task_) { On 2014/08/29 03:34:16, hirono wrote: > nit: We don't add {} if the body of if statements is 1 line. Done. https://codereview.chromium.org/507293002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:694: return; On 2014/08/29 03:34:16, hirono wrote: > It can skip an event even if the event is reporting sync error. Done. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:64: if (this.item_.id === '') { On 2014/08/29 03:34:17, hirono wrote: > Maybe we should check this.item_ !== null. Done. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:107: if (!this.item_) { On 2014/08/29 03:34:16, hirono wrote: > Can the item be null? Done. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:112: if (!this.item_.id) { On 2014/08/29 03:34:17, hirono wrote: > Maybe we should check this.item_ !== null. Done. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:126: console.log(this.item_.message); On 2014/08/29 03:34:16, hirono wrote: > Please remove the debug log. Done. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:160: delete this.item_; On 2014/08/29 03:34:17, hirono wrote: > We usually do this.item_ = null, instead of deleting it. > > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... Done. https://codereview.chromium.org/507293002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:182: if (!this.item_.id) { On 2014/08/29 03:34:16, hirono wrote: > Maybe we should check this.item_ !== null. Done.
If there is no application window, the sync status is displayed in the desktop notifications. Please also check "https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/file_manager/background/js/progress_center.js&l=44" ? https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:668: job_info_with_status, base::Time::Now(), false /* should_delay */); It looks delayed progress events can be dispatched after the complete/failure event. https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:195: // after enough time has passed from the previous event. Please update the comment with mentioning |call_time| and |should_delay|. https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:206: const base::TimeDelta startup_time_delta_; Could you rename startup_time_delta_? It is not only for startup now. Maybe drive_transfer_event_delay_? https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:65: if (this.item_.sync) { We also does not add {} for 1-line body in JavaScript. https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:109: if (this.item_.state !== ProgressItemState.PROGRESSING) { We also does not add {} for 1-line body in JavaScript. https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:115: if (status.num_total_jobs > 1) { Even if it has an else block, we also does not add {}. Just a problem of code style :) https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/progress_center_common.js (right): https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/progress_center_common.js:116: this.sync = false; Although the progress center item is used widely, the variable is only used in drive_sync_handler.js. Why does not the DriveSyncHandler have the property?
Thank you for your review. https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.cc:668: job_info_with_status, base::Time::Now(), false /* should_delay */); On 2014/08/29 05:38:30, hirono wrote: > It looks delayed progress events can be dispatched after the complete/failure > event. Done. https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:195: // after enough time has passed from the previous event. On 2014/08/29 05:38:30, hirono wrote: > Please update the comment with mentioning |call_time| and |should_delay|. Done. https://codereview.chromium.org/507293002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/event_router.h:206: const base::TimeDelta startup_time_delta_; On 2014/08/29 05:38:30, hirono wrote: > Could you rename startup_time_delta_? It is not only for startup now. > Maybe drive_transfer_event_delay_? Done. https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:65: if (this.item_.sync) { On 2014/08/29 05:38:30, hirono wrote: > We also does not add {} for 1-line body in JavaScript. Done. https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:109: if (this.item_.state !== ProgressItemState.PROGRESSING) { On 2014/08/29 05:38:31, hirono wrote: > We also does not add {} for 1-line body in JavaScript. Done. https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:115: if (status.num_total_jobs > 1) { On 2014/08/29 05:38:30, hirono wrote: > Even if it has an else block, we also does not add {}. Just a problem of code > style :) Done. https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/progress_center_common.js (right): https://codereview.chromium.org/507293002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/progress_center_common.js:116: this.sync = false; On 2014/08/29 05:38:31, hirono wrote: > Although the progress center item is used widely, the variable is only used in > drive_sync_handler.js. > Why does not the DriveSyncHandler have the property? > Done.
https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/job_scheduler.cc (right): https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:707: QueueType queue_type = GetJobQueueType(job->job_info.job_type); const https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:859: QueueType queue_type = GetJobQueueType(job_info->job_type); const https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:868: QueueType queue_type = GetJobQueueType(job_info->job_type); const https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:1047: QueueType queue_type = GetJobQueueType(job_entry->job_info.job_type); const https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:681: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); How about doing this? ScheduleEvent(Event event, bool immediate) { // Update next send time. Time now = Now(); if (immediate) { nextSendTime = now; else if (event_ == NULL) nextSendTime = Math.max(now + 100ms, nextSendTime); // Update the latest event. event_ = event; // Schedule event. TimeDelta delta = nextSendTime - now; if (delta <= 0) SendEvent(); else PostDelayedTask(..., SendEvent, delta); } SendEvent() { Time now = Now(); if (now < nextSendTime_ || event_ == null) return; Dispatch(event_); event_ = null; nextSendTime = now + 1000ms. } https://codereview.chromium.org/507293002/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:29: /** Please fix the indent. https://codereview.chromium.org/507293002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:31: * @type {boolean} @private
Thank you for your review:) https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/job_scheduler.cc (right): https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:707: QueueType queue_type = GetJobQueueType(job->job_info.job_type); On 2014/08/29 11:50:46, hirono wrote: > const Done. https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:859: QueueType queue_type = GetJobQueueType(job_info->job_type); On 2014/08/29 11:50:46, hirono wrote: > const Done. https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:868: QueueType queue_type = GetJobQueueType(job_info->job_type); On 2014/08/29 11:50:46, hirono wrote: > const Done. https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_scheduler.cc:1047: QueueType queue_type = GetJobQueueType(job_entry->job_info.job_type); On 2014/08/29 11:50:46, hirono wrote: > const Done. https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:681: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2014/08/29 11:50:46, hirono wrote: > How about doing this? > > ScheduleEvent(Event event, bool immediate) { > // Update next send time. > Time now = Now(); > if (immediate) { > nextSendTime = now; > else if (event_ == NULL) > nextSendTime = Math.max(now + 100ms, nextSendTime); > > // Update the latest event. > event_ = event; > > // Schedule event. > TimeDelta delta = nextSendTime - now; > if (delta <= 0) > SendEvent(); > else > PostDelayedTask(..., SendEvent, delta); > } > > SendEvent() { > Time now = Now(); > if (now < nextSendTime_ || event_ == null) > return; > Dispatch(event_); > event_ = null; > nextSendTime = now + 1000ms. > } Done. https://codereview.chromium.org/507293002/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:29: /** On 2014/08/29 11:50:46, hirono wrote: > Please fix the indent. Done. https://codereview.chromium.org/507293002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:31: * @type {boolean} On 2014/08/29 11:50:46, hirono wrote: > @private Done.
https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:720: drive_job_info_with_status_.status.clear(); nit: How about using scoped_ptr instead of using an empty string as a marker of empty drive job info? https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:196: void ScheduleEvent(DriveJobInfoWithStatus& job_info_with_status, Sorry this is my fault. The naming in my pseudo code is so bad. Please fix the name as we can find out the function is for DriveFileTransferEvent like as SendDriveFileTransferEvent. https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:206: base::Time nextSendTime_; ditto. Please fix the name. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:95: this.dispatchEvent(new Event(DriveSyncHandler.COMPLETED_EVENT)); Although this is existing bug, please move the event dispatching in removeItem. The variable this.synching_ is not updated here because the update procedure is queued in this.queue_. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:113: this.item_.state = ProgressItemState.PROGRESSING; We can just assign the state. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:129: if (!this.item_) { this.item_ is never null. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:129: if (!this.item_) { If webkitResolveLocalFileSystemURL #111 fails, the information of the item is not updated. Please handle the case. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:133: this.item_.progressValue = status.processed || 0; Please assign 0 and add a comment. status.processed does not show the progress of whole of sync now. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:134: this.item_.progressMax = status.total || 1; ditto. Please assign 1. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:147: if (!this.item_) { this.item_ is never null. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:175: this.item_.type = ProgressItemType.SYNC; Please use new item (and new ID) here. In the current code, the error message may be overwritten by following progressing events.
Thank you for your review :) https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:720: drive_job_info_with_status_.status.clear(); On 2014/09/01 03:53:44, hirono wrote: > nit: How about using scoped_ptr instead of using an empty string as a marker of > empty drive job info? > Done. https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:196: void ScheduleEvent(DriveJobInfoWithStatus& job_info_with_status, On 2014/09/01 03:53:44, hirono wrote: > Sorry this is my fault. The naming in my pseudo code is so bad. > Please fix the name as we can find out the function is for > DriveFileTransferEvent like as SendDriveFileTransferEvent. Done. https://codereview.chromium.org/507293002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:206: base::Time nextSendTime_; On 2014/09/01 03:53:44, hirono wrote: > ditto. Please fix the name. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:95: this.dispatchEvent(new Event(DriveSyncHandler.COMPLETED_EVENT)); On 2014/09/01 03:53:44, hirono wrote: > Although this is existing bug, please move the event dispatching in removeItem. > The variable this.synching_ is not updated here because the update procedure is > queued in this.queue_. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:113: this.item_.state = ProgressItemState.PROGRESSING; On 2014/09/01 03:53:45, hirono wrote: > We can just assign the state. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:129: if (!this.item_) { On 2014/09/01 03:53:45, hirono wrote: > this.item_ is never null. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:129: if (!this.item_) { On 2014/09/01 03:53:44, hirono wrote: > If webkitResolveLocalFileSystemURL #111 fails, the information of the item is > not updated. Please handle the case. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:133: this.item_.progressValue = status.processed || 0; On 2014/09/01 03:53:45, hirono wrote: > Please assign 0 and add a comment. status.processed does not show the progress > of whole of sync now. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:134: this.item_.progressMax = status.total || 1; On 2014/09/01 03:53:44, hirono wrote: > ditto. Please assign 1. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:147: if (!this.item_) { On 2014/09/01 03:53:45, hirono wrote: > this.item_ is never null. Done. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:175: this.item_.type = ProgressItemType.SYNC; On 2014/09/01 03:53:44, hirono wrote: > Please use new item (and new ID) here. > In the current code, the error message may be overwritten by following > progressing events. Done.
https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:175: this.item_.type = ProgressItemType.SYNC; On 2014/09/01 06:45:40, iseki wrote: > On 2014/09/01 03:53:44, hirono wrote: > > Please use new item (and new ID) here. > > In the current code, the error message may be overwritten by following > > progressing events. > > Done. Sorry for less description. The problem is we uses the same item for normal processes and errors. After reporting an error, the app still processes the remaining files. It means the handler receives the following events. If we update the error item with the following events, the error message may disappear before users read it. The solution is creating new item here and pass it to this.progressCenter_.updateItem. Note: the error message will disappear automatically after several seconds. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:672: next_send_time_ = Please add comment why we need to delay the events. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:673: std::max(now + base::TimeDelta::FromMilliseconds(100), next_send_time_); Please turn the number into constant. note: We don't have constants of non-primitive types. So we need to define int constant and pass it to the base::TimeDelta::FromMilliseconds here. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:684: base::TimeDelta delta = next_send_time_ - now; const https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:718: next_send_time_ = now + base::TimeDelta::FromMilliseconds(500); ditto: Please add comment why we need to delay the event. (Maybe readers cannot find out why the number of milliseconds is different from #673. ditto: turn it into constant. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:206: base::Time next_send_time_; Please include information for the kind of event (DriveFileTransferEvent) in the name as well as #204 and #205. Because the class handles many kinds of events, the name is ambiguous here. https://codereview.chromium.org/507293002/diff/140001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/140001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:135: if (!this.successWebkitResolveLocalFileSystemURL) { There is a race. 1. Call updateItem_ (successWebkitResolve... = false). 2. Process the first callback in queue (successWebkitResolve... = true). 3. Call updateItem_ again (successWebkitResolve... = false). 4. Process the second callback in queue. How about changing a plan so that the class has a null item. And assign null to the item on failure?
Thank you for your review. https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:175: this.item_.type = ProgressItemType.SYNC; On 2014/09/01 07:29:01, hirono wrote: > On 2014/09/01 06:45:40, iseki wrote: > > On 2014/09/01 03:53:44, hirono wrote: > > > Please use new item (and new ID) here. > > > In the current code, the error message may be overwritten by following > > > progressing events. > > > > Done. > > Sorry for less description. The problem is we uses the same item for normal > processes and errors. > After reporting an error, the app still processes the remaining files. It means > the handler receives the following events. > > If we update the error item with the following events, the error message may > disappear before users read it. The solution is creating new item here and pass > it to this.progressCenter_.updateItem. > > Note: the error message will disappear automatically after several seconds. Done. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:672: next_send_time_ = On 2014/09/01 07:29:01, hirono wrote: > Please add comment why we need to delay the events. Done. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:673: std::max(now + base::TimeDelta::FromMilliseconds(100), next_send_time_); On 2014/09/01 07:29:01, hirono wrote: > Please turn the number into constant. > note: We don't have constants of non-primitive types. So we need to define int > constant and pass it to the base::TimeDelta::FromMilliseconds here. Done. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:684: base::TimeDelta delta = next_send_time_ - now; On 2014/09/01 07:29:01, hirono wrote: > const Done. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:718: next_send_time_ = now + base::TimeDelta::FromMilliseconds(500); On 2014/09/01 07:29:01, hirono wrote: > ditto: Please add comment why we need to delay the event. (Maybe readers cannot > find out why the number of milliseconds is different from #673. > ditto: turn it into constant. Done. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:718: next_send_time_ = now + base::TimeDelta::FromMilliseconds(500); On 2014/09/01 07:29:01, hirono wrote: > ditto: Please add comment why we need to delay the event. (Maybe readers cannot > find out why the number of milliseconds is different from #673. > ditto: turn it into constant. Done. https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:206: base::Time next_send_time_; On 2014/09/01 07:29:02, hirono wrote: > Please include information for the kind of event (DriveFileTransferEvent) in the > name as well as #204 and #205. > Because the class handles many kinds of events, the name is ambiguous here. > Done. https://codereview.chromium.org/507293002/diff/140001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/140001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:135: if (!this.successWebkitResolveLocalFileSystemURL) { On 2014/09/01 07:29:02, hirono wrote: > There is a race. > > 1. Call updateItem_ (successWebkitResolve... = false). > 2. Process the first callback in queue (successWebkitResolve... = true). > 3. Call updateItem_ again (successWebkitResolve... = false). > 4. Process the second callback in queue. > > > How about changing a plan so that the class has a null item. And assign null to > the item on failure? Done.
Thanks. The patch has much more improved. Maybe last comments. https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:670: base::Time now = base::Time::Now(); nit: const https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:207: const base::TimeDelta delay_time_; 1. Does it need to be a member of class? We usually tries to reduce the number of members because header files are shared widely. 2. Please make the name more descriptive. The delay_time is actually only for file transfer events. It's OK the name is a bit long because the variable is used only once. https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:208: const base::TimeDelta force_send_interval_; ditto https://codereview.chromium.org/507293002/diff/160001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:117: if (!this.item_.id) We don't need to check the ID. https://codereview.chromium.org/507293002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:128: this.successWebkitResolveLocalFileSystemURL = true; Remove the line.
Thank you for your review :) https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:670: base::Time now = base::Time::Now(); On 2014/09/01 09:41:46, hirono wrote: > nit: const Done. https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:207: const base::TimeDelta delay_time_; On 2014/09/01 09:41:46, hirono wrote: > 1. Does it need to be a member of class? We usually tries to reduce the number > of members because header files are shared widely. > 2. Please make the name more descriptive. The delay_time is actually only for > file transfer events. It's OK the name is a bit long because the variable is > used only once. Done. https://codereview.chromium.org/507293002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:208: const base::TimeDelta force_send_interval_; On 2014/09/01 09:41:46, hirono wrote: > ditto Done. https://codereview.chromium.org/507293002/diff/160001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:117: if (!this.item_.id) On 2014/09/01 09:41:46, hirono wrote: > We don't need to check the ID. Done. https://codereview.chromium.org/507293002/diff/160001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:128: this.successWebkitResolveLocalFileSystemURL = true; On 2014/09/01 09:41:46, hirono wrote: > Remove the line. Done.
LGTM after fixing the nits. Thank you! https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:80: const base::TimeDelta file_transfer_event_delay_time = We don't have constants of non-primitive types. So we need to define int constant here and pass it to the base::TimeDelta::FromMilliseconds when using it. https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:662: ? kFileTransferStateCompleted Please run git cl before uploading the next patch. The indent of #662 may not match with our style. https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:702: base::Time now = base::Time::Now(); const https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: // To avoid terriable delay due to dense consecutive event, set the Please fix "... set the the force ..." -> "... set the force ...". https://codereview.chromium.org/507293002/diff/180001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:22: * Counter for error ID nit: Please add period.
Thank you for your review:) https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:80: const base::TimeDelta file_transfer_event_delay_time = On 2014/09/01 12:04:54, hirono wrote: > We don't have constants of non-primitive types. So we need to define int > constant here and pass it to the base::TimeDelta::FromMilliseconds when using > it. Done. https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:662: ? kFileTransferStateCompleted On 2014/09/01 12:04:54, hirono wrote: > Please run git cl before uploading the next patch. > The indent of #662 may not match with our style. Done. https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:702: base::Time now = base::Time::Now(); On 2014/09/01 12:04:54, hirono wrote: > const Done. https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: // To avoid terriable delay due to dense consecutive event, set the On 2014/09/01 12:04:54, hirono wrote: > Please fix "... set the the force ..." -> "... set the force ...". Done. https://codereview.chromium.org/507293002/diff/180001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/507293002/diff/180001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/drive_sync_handler.js:22: * Counter for error ID On 2014/09/01 12:04:54, hirono wrote: > nit: Please add period. Done.
iseki@chromium.org changed reviewers: + finnur@chromium.org, kinaba@chromium.org
Please take a look. finnur@ chrome/common/extensions/api/file_browser_private.idl kinaba@ chrome/browser/chromeos/drive/job_list.cc chrome/browser/chromeos/drive/job_list.h chrome/browser/chromeos/drive/job_scheduler.cc chrome/browser/chromeos/extensions/file_manager/event_router.cc chrome/browser/chromeos/extensions/file_manager/event_router.h chrome/browser/chromeos/extensions/file_manager/private_api_strings.cc
On 2014/09/02 01:05:43, iseki wrote: > Thank you for your review:) > > https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... > File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): > > https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:80: const > base::TimeDelta file_transfer_event_delay_time = > On 2014/09/01 12:04:54, hirono wrote: > > We don't have constants of non-primitive types. So we need to define int > > constant here and pass it to the base::TimeDelta::FromMilliseconds when using > > it. > > Done. > > https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:662: ? > kFileTransferStateCompleted > On 2014/09/01 12:04:54, hirono wrote: > > Please run git cl before uploading the next patch. > > The indent of #662 may not match with our style. > > Done. > > https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:702: base::Time > now = base::Time::Now(); > On 2014/09/01 12:04:54, hirono wrote: > > const > > Done. > > https://codereview.chromium.org/507293002/diff/180001/chrome/browser/chromeos... > chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: // To avoid > terriable delay due to dense consecutive event, set the > On 2014/09/01 12:04:54, hirono wrote: > > Please fix "... set the the force ..." -> "... set the force ...". > > Done. > > https://codereview.chromium.org/507293002/diff/180001/ui/file_manager/file_ma... > File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): > > https://codereview.chromium.org/507293002/diff/180001/ui/file_manager/file_ma... > ui/file_manager/file_manager/background/js/drive_sync_handler.js:22: * Counter > for error ID > On 2014/09/01 12:04:54, hirono wrote: > > nit: Please add period. > > Done. still lgtm
https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/job_list.h (right): https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_list.h:94: long num_total_jobs; JobInfo is a structure to represent the status of each single jobs. It looks strange that it includes the info of whole job list. Rather than adding it to the field, wouldn't it be possible to obtain the data by, for instance, calling GetJobInfoList().size() in each listener? (If it is too inefficient, you can add a new method to get just the number in JobList class) https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:80: const int64 kFileTransferEventDelayTime = 100; Could you add the uint (millisecond?) of the value at least in the comment, or preferably in the variable name? https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:82: const int64 kFileTransferEventForceSendInterval = 500; ditto https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:702: if (now < next_send_file_transfer_event_ || !drive_job_info_with_status_) This may be fragile due to reasons. First, according to the document in task_runner.h, PostDelayedTask() do not always ensure the specified time to be elapsed. In that case, it's still now < next_send_file_transfer_event_ and since no more task for SendDriveFileTransferEvent is posted, the event does not fire forever. Even if PostDelayedTask ensures the gap, in rare conditions (like the system clock was synced and Time::Now() rolled back), |immediate| events may be discarded. Is the JS side implementation fine with that? https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:716: status_list.push_back(status); Previously the onFileTransferUpdated event sent all the list of active events, while now it sends only the last updated event. Is it intentional? If it is so, the private API should also be updated. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: // force send time. I couldn't see how it works, or what is it for. Is it (1) for avoiding to send too many events in short interval, or (2) for avoiding to suppress too many events by always sending some event at the frequency of ForceSendInterval? If it is (1), the comment and the variable name is unclear, and moreover, can't you reuse the existing ShouldSendProgressEvent? If it is (2), it doesn't look working as intended (event without it the events are sent in the interval of kFileTransferEventDelayTime) https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:202: scoped_ptr<DriveJobInfoWithStatus> drive_job_info_with_status_; Maybe: drive_job_info_for_scheduled_event_ or something like that. The class is dealing with many drive job info. The name has to specify which drive job info the variable is holding. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:203: base::Time last_file_transfer_event_; this member looks not used anymore.
file_browser_private.idl LGTM. But I don't understand the CL description and TEST=manually is not really helpful. Suggest revising.
Thank you for your review. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/job_list.h (right): https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/job_list.h:94: long num_total_jobs; On 2014/09/02 05:01:39, kinaba wrote: > JobInfo is a structure to represent the status of each single jobs. > It looks strange that it includes the info of whole job list. > > Rather than adding it to the field, wouldn't it be possible to obtain the data > by, for instance, calling GetJobInfoList().size() in each listener? > (If it is too inefficient, you can add a new method to get just the number > in JobList class) Now, EventRouter can not call GetJobInfoList().size(). EventRouter needs to have the instance of JobScheduler to call it. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:80: const int64 kFileTransferEventDelayTime = 100; On 2014/09/02 05:01:39, kinaba wrote: > Could you add the uint (millisecond?) of the value at least in the comment, or > preferably in the variable name? Done. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:82: const int64 kFileTransferEventForceSendInterval = 500; On 2014/09/02 05:01:39, kinaba wrote: > ditto Done. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:702: if (now < next_send_file_transfer_event_ || !drive_job_info_with_status_) On 2014/09/02 05:01:39, kinaba wrote: > This may be fragile due to reasons. > > First, according to the document in task_runner.h, PostDelayedTask() do not > always ensure the specified time to be elapsed. In that case, it's still now < > next_send_file_transfer_event_ and since no more task for > SendDriveFileTransferEvent is posted, the event does not fire forever. > > Even if PostDelayedTask ensures the gap, in rare conditions (like the system > clock was synced and Time::Now() rolled back), |immediate| events may be > discarded. Is the JS side implementation fine with that? Done. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:716: status_list.push_back(status); On 2014/09/02 05:01:39, kinaba wrote: > Previously the onFileTransferUpdated event sent all the list of active events, > while now it sends only the last updated event. Is it intentional? > If it is so, the private API should also be updated. I make the clean up issue(crbug.com/408482) and try to fix it another patch. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:725: // force send time. On 2014/09/02 05:01:39, kinaba wrote: > I couldn't see how it works, or what is it for. > > Is it (1) for avoiding to send too many events in short interval, or (2) for > avoiding to suppress too many events by always sending some event at the > frequency of ForceSendInterval? > > If it is (1), the comment and the variable name is unclear, and moreover, can't > you reuse the existing ShouldSendProgressEvent? > > If it is (2), it doesn't look working as intended (event without it the events > are sent in the interval of kFileTransferEventDelayTime) Done. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:202: scoped_ptr<DriveJobInfoWithStatus> drive_job_info_with_status_; On 2014/09/02 05:01:40, kinaba wrote: > Maybe: drive_job_info_for_scheduled_event_ or something like that. > The class is dealing with many drive job info. The name has to specify which > drive job info the variable is holding. Done. https://codereview.chromium.org/507293002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:203: base::Time last_file_transfer_event_; On 2014/09/02 05:01:40, kinaba wrote: > this member looks not used anymore. Done.
I update the CL. Please take a look.
Patch title would be something like: "Enrich fileBrowserPrivate.onFileTransfersUpdated event to support displaying total number of jobs." and then you can add more details below. You don't need to write everything in one line. Details may include that * "added" type is added * "num_total_jobs" member is added * and this is for displaying the total number of jobs in Chrome OS file manager app. https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:114: integration_service->job_list()->GetJobInfoList().size(); This includes the number of jobs that is not drive::IsActiveFileTransferJobInfo. Is it fine for your purpose? https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:695: if (next_send_file_transfer_event_ < now) { <=, rather than < ("immediate" events should fire right now, I think.) Or, what about something like this? void Schedule...(...) { const bool no_pending_task = !drive_job_info_for_scheduled_event_; drive_job_info_for_scheduled_event_.reset(new ...); if(immediate) { SendDriveFileTransferEvent(); } else if(no_pending_task) { PostDelayedTask(...); } } void Send...() { if(!drive_job_info_for_scheduled_event_) return; status_list... drive_job_info_for_scheduled_event_.reset(); BroadCastEvent(...); } The point is * SendDriveFileTransferEvent always sends if there is an unflushed event. * Only ScheduleDriveFileTransferEvent controls the timing logic, in a way that non-immediate events are delayed for 100ms. I think doing Post(next_send_time_ - now) every time during the delay as in the current patch is not quite useful, since all the posted tasks fire at |next_send_time_|, but only the first one of them sees non-null drive_job_info_. https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:193: // Schedule onFileTransferUpdated event. Please add some comment on why scheduling is necessary. https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:198: // Sends onFileTranferUpdated to extensions if after nextSendTime_. "if after nextSendTime_" is not grammatical. Probably you can just drop the phrase if the implementation is adjusted (see my comment there.)
Thank you for your review :) https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:114: integration_service->job_list()->GetJobInfoList().size(); On 2014/09/03 05:42:24, kinaba wrote: > This includes the number of jobs that is not drive::IsActiveFileTransferJobInfo. > Is it fine for your purpose? Done. https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:695: if (next_send_file_transfer_event_ < now) { On 2014/09/03 05:42:24, kinaba wrote: > <=, rather than < > > ("immediate" events should fire right now, I think.) > > Or, what about something like this? > > void Schedule...(...) { > const bool no_pending_task = !drive_job_info_for_scheduled_event_; > drive_job_info_for_scheduled_event_.reset(new ...); > if(immediate) { > SendDriveFileTransferEvent(); > } else if(no_pending_task) { > PostDelayedTask(...); > } > } > > void Send...() { > if(!drive_job_info_for_scheduled_event_) > return; > status_list... > drive_job_info_for_scheduled_event_.reset(); > BroadCastEvent(...); > } > > The point is > * SendDriveFileTransferEvent always sends if there is an unflushed event. > * Only ScheduleDriveFileTransferEvent controls the timing logic, in a way that > non-immediate events are delayed for 100ms. > > I think doing Post(next_send_time_ - now) every time during the delay as in the > current patch is not quite useful, since all the posted tasks fire at > |next_send_time_|, > but only the first one of them sees non-null drive_job_info_. Done. https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:193: // Schedule onFileTransferUpdated event. On 2014/09/03 05:42:24, kinaba wrote: > Please add some comment on why scheduling is necessary. Done. https://codereview.chromium.org/507293002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:198: // Sends onFileTranferUpdated to extensions if after nextSendTime_. On 2014/09/03 05:42:24, kinaba wrote: > "if after nextSendTime_" is not grammatical. Probably you can just drop the > phrase if the implementation is adjusted (see my comment there.) Done.
Thanks, almost there :) Please update the first line of "Description", not only the "Subject" line. I think the final commit only includes the text from Description. Also, put one blank line between the first line of Description and the following lines. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:87: std::vector<drive::JobInfo> GetActiveFileTransferJobInfo( You can simply return only the number of jobs size_t CountActiveFileTransferJob(...); (But you can leave it as is if you have a plan to use the vector in future patches.) https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:88: std::vector<drive::JobInfo> job_info_list) { const std::vector<drive::JobInfo>& job_info_list https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:91: end = job_info_list.end(); (Though I personally prefer iterators :)), we are preferring index based loop like for (size_t i = 0; i < job_info_list.size(); ++i) {} for code simplicity in file manager code. Let's be consistent with it. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:630: is_new_job ? kFileTransferStateStarted : kFileTransferStateInProgress; Just a question, do we need to distinguish these two status? If not, it looks you can completely remove |drive_jobs_| member. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:176: // Schedule onFileTransferUpdated event to count following events. "to count following events" is still vague. I guess it can be clearer if first "what it does" is explained, and then "why". How about this: Sends onFileTransferUpdate event right now if |immediate| is set. Otherwise it refrains from sending for a short while, and after that it sends the most recently scheduled event once. The delay is for waiting subsequent 'added' events to come after the first one when multiple tasks are added. This way, we can avoid frequent UI update caused by differences between singular and plural cases. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:186: void SendDriveFileTransferEvent(); This method itself does not refer timing anymore, so the comment is not quite adequate. How about: Sends the most recently scheduled onFileTransferUpdated event to extensions. This is used for implementing ScheduledDriveFileTransferEvent().
Thank you for your review. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:87: std::vector<drive::JobInfo> GetActiveFileTransferJobInfo( On 2014/09/03 15:54:18, kinaba wrote: > You can simply return only the number of jobs > size_t CountActiveFileTransferJob(...); > > (But you can leave it as is if you have a plan to use the vector in future > patches.) Done. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:88: std::vector<drive::JobInfo> job_info_list) { On 2014/09/03 15:54:18, kinaba wrote: > const std::vector<drive::JobInfo>& job_info_list Done. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:91: end = job_info_list.end(); On 2014/09/03 15:54:18, kinaba wrote: > (Though I personally prefer iterators :)), we are preferring index based loop > like > for (size_t i = 0; i < job_info_list.size(); ++i) {} > for code simplicity in file manager code. Let's be consistent with it. Done. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.cc:630: is_new_job ? kFileTransferStateStarted : kFileTransferStateInProgress; On 2014/09/03 15:54:18, kinaba wrote: > Just a question, do we need to distinguish these two status? > If not, it looks you can completely remove |drive_jobs_| member. Now, kFileTransferStateInProgress is not necessary because it is not used in Files.app. I make new issue https://code.google.com/p/chromium/issues/detail?id=410631&thanks=410631&ts=1... https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:176: // Schedule onFileTransferUpdated event to count following events. On 2014/09/03 15:54:19, kinaba wrote: > "to count following events" is still vague. > I guess it can be clearer if first "what it does" is explained, and then "why". > > How about this: > > Sends onFileTransferUpdate event right now if |immediate| is set. Otherwise it > refrains from sending for a short while, and after that it sends the most > recently scheduled event once. > > The delay is for waiting subsequent 'added' events to come after the first one > when multiple tasks are added. This way, we can avoid frequent UI update caused > by differences between singular and plural cases. Done. https://codereview.chromium.org/507293002/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:186: void SendDriveFileTransferEvent(); On 2014/09/03 15:54:19, kinaba wrote: > This method itself does not refer timing anymore, so the comment is not quite > adequate. > > How about: > > Sends the most recently scheduled onFileTransferUpdated event to extensions. > This is used for implementing ScheduledDriveFileTransferEvent(). Done.
lgtm (with a nitty nit)! https://codereview.chromium.org/507293002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:177: // it please reduce newlines.
Thanks :) https://codereview.chromium.org/507293002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/event_router.h (right): https://codereview.chromium.org/507293002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/event_router.h:177: // it On 2014/09/04 01:40:00, kinaba wrote: > please reduce newlines. Done.
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iseki@chromium.org/507293002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iseki@chromium.org/507293002/300001
On 2014/09/04 03:06:17, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_clang_dbg_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) How about following cases? Schedule A (100ms) Schedule B with immediate = true, Send B (... 50ms) Schedule C (100ms) (... 50ms) Send C (requested by A) C does not wait 100ms. Is it intended?
I observe sync status and it don't flicker. The complete or error event must come in 300ms. I try to sync 0byte file and syncing status don't flicker. So this case is not problem in normal situation. May be, it is problem in error situation.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iseki@chromium.org/507293002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as 81cd210ef8dae920c47443ccbc959ceb7f4d4d79
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/4c0895fd17dac300cfbeccd0bf236b58a5910a36 Cr-Commit-Position: refs/heads/master@{#293257} |
