Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
576063002:
Modify the behavior of cancel button in syncing status to cancel all 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, yoshiki+watch_chromium.org, rginda+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. |
DescriptionModify the action of cancel button in syncing status to cancel all jobs.
* Previous behavior is to cancel only one item.
* After Issue 400636, this behavior doesn't make sense because only the
number is decreased.
BUG=414610
TEST=manually
1. Copy the file from Download to Drive.
2. Push the button in syncing status.
3. Confirm all jobs are canceled.
Committed: https://crrev.com/6521367eb71a626aaf6c1596086ee6ba410a299c
Cr-Commit-Position: refs/heads/master@{#295638}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 21 (4 generated)
iseki@chromium.org changed reviewers: + kinaba@chromium.org
Please take a look:)
https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (left): https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:562: EXTENSION_FUNCTION_VALIDATE(params); I guess you are planning to update the IDL of this API as well in the later patch. Could you add a comment on the plan to the patch description? https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:565: // Create the mapping from file path to job ID. Please update the comment. https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:570: // Cancel by Job ID. Please update the comment. https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:579: result->file_url = jobs[i].file_path.value(); These response values are not used at all in Files.app actually. Could you remove them entirely on this occasion? Or, if you have a plan to use these value, please be sure to return file_url, not file_path.
Thank you for your review :) The attachment from Gmail needs to cancel single jobs. So I remain the previous implementation. https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (left): https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:562: EXTENSION_FUNCTION_VALIDATE(params); On 2014/09/18 02:15:54, kinaba wrote: > I guess you are planning to update the IDL of this API as well in the later > patch. > Could you add a comment on the plan to the patch description? Done. https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:565: // Create the mapping from file path to job ID. On 2014/09/18 02:15:54, kinaba wrote: > Please update the comment. Done. https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:570: // Cancel by Job ID. On 2014/09/18 02:15:54, kinaba wrote: > Please update the comment. Done. https://codereview.chromium.org/576063002/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:579: result->file_url = jobs[i].file_path.value(); On 2014/09/18 02:15:54, kinaba wrote: > These response values are not used at all in Files.app actually. > Could you remove them entirely on this occasion? > > Or, if you have a plan to use these value, please be sure to return file_url, > not file_path. Done.
lgtm https://codereview.chromium.org/576063002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/576063002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:580: } else { The "// Create the mapping from file path to job ID." comment above should be moved here. https://codereview.chromium.org/576063002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/576063002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:155: chrome.fileManagerPrivate.cancelFileTransfers([], function() {}); Please add a comment that this is cancelling all file transfers.
Thanks! https://codereview.chromium.org/576063002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/576063002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:580: } else { On 2014/09/18 05:39:16, kinaba wrote: > The "// Create the mapping from file path to job ID." comment above should be > moved here. Done. https://codereview.chromium.org/576063002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/576063002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:155: chrome.fileManagerPrivate.cancelFileTransfers([], function() {}); On 2014/09/18 05:39:16, kinaba wrote: > Please add a comment that this is cancelling all file transfers. Done.
iseki@chromium.org changed reviewers: + hirono@chromium.org
Please take a look.
@hirono ui/file_manager/file_manager/background/js/drive_sync_handler.js
https://codereview.chromium.org/576063002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/576063002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:156: chrome.fileManagerPrivate.cancelFileTransfers([], function() {}); Can we make it optional argument? Empty array may be generated unintentionally. e.g. Array#filter method.
Thank you for your review:) https://codereview.chromium.org/576063002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/576063002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/drive_sync_handler.js:156: chrome.fileManagerPrivate.cancelFileTransfers([], function() {}); On 2014/09/18 05:50:42, hirono wrote: > Can we make it optional argument? > Empty array may be generated unintentionally. e.g. Array#filter method. Done.
lgtm with a nit. Thank you! https://codereview.chromium.org/576063002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/576063002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:596: file_path = drive::util::ExtractDrivePath(file_path); Actually IsUnderDriveMountPoint calles ExtractDrivePath and checks if the path is empty or not. Please DCHECK(file_path.empty()) instead.
Thank you for your review :) https://codereview.chromium.org/576063002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/576063002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:596: file_path = drive::util::ExtractDrivePath(file_path); On 2014/09/18 08:17:59, hirono wrote: > Actually IsUnderDriveMountPoint calles ExtractDrivePath and checks if the path > is empty or not. Please DCHECK(file_path.empty()) instead. Done.
iseki@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Please take a look. @rdevlin.cronin chrome/common/extensions/api/file_manager_private.idl
chrome/common/extensions/api/file_manager_private.idl lgtm once comment is added. https://codereview.chromium.org/576063002/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/file_manager_private.idl (right): https://codereview.chromium.org/576063002/diff/80001/chrome/common/extensions... chrome/common/extensions/api/file_manager_private.idl:667: // |fileUrls| Array of files for which ongoing transfer should be canceled. comment on the behavior if this is absent?
Thank you for your review. https://codereview.chromium.org/576063002/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/file_manager_private.idl (right): https://codereview.chromium.org/576063002/diff/80001/chrome/common/extensions... chrome/common/extensions/api/file_manager_private.idl:667: // |fileUrls| Array of files for which ongoing transfer should be canceled. On 2014/09/18 18:47:55, Devlin wrote: > comment on the behavior if this is absent? Done.
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/576063002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 4a18d34f19dbfc5488075f3615131103487fb7fc
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6521367eb71a626aaf6c1596086ee6ba410a299c Cr-Commit-Position: refs/heads/master@{#295638} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
