Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2877343003:
Switch browser/extensions/api/file_system/file_system_api.[h|cc] to the TaskScheduler API (Closed)
|
Created:
3 years, 7 months ago by Sébastien Marchand Modified:
3 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tzik, kinuko+fileapi, nhiroki, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch browser/extensions/api/file_system/file_system_api.[h|cc] to the TaskScheduler API
With this CL, tasks posted from this file can run in parallel. Previously, they
were all posted to the file thread so that was not possible. Please verify that
this is ok.
Also, please verify that appropriate traits are used to post task
https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h
BUG=689520
Review-Url: https://codereview.chromium.org/2877343003
Cr-Commit-Position: refs/heads/master@{#472785}
Committed: https://chromium.googlesource.com/chromium/src/+/8bf05bb205b2693eae180c0b69c7aa5385a3592f
Patch Set 1 : Switch FileSystemRetainEntryFunction to the TaskScheduler API. #Patch Set 2 : Fixes. #
Total comments: 6
Patch Set 3 : Address Francois' comments. #
Total comments: 2
Patch Set 4 : Address Ben's comments. #
Messages
Total messages: 47 (37 generated)
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sebmarchand@chromium.org changed reviewers: + fdoray@chromium.org
PTAL
https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:596: task_runner_->PostTaskAndReply( Unless RunAsync() is invoked multiple times on the same instance and it is important to have mutual exclusion between calls to SetIsDirectoryOnFileThread(), I don't think we need a sequence here. You could simply write: base::PostTaskWithTraitsAndReply( FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND}, base::BindOnce(&FileSystem...), base::BindOnce(&FileSystem...)); You can ask owners to confirm whether this is ok. https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:599: &FileSystemGetWritableEntryFunction::SetIsDirectoryOnFileThread, s/OnFileThread/Async/ https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:883: task_runner_->PostTask( ditto (a sequence may not be required)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
Thanks, PTAnL. https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:596: task_runner_->PostTaskAndReply( On 2017/05/16 15:23:26, fdoray wrote: > Unless RunAsync() is invoked multiple times on the same instance and it is > important to have mutual exclusion between calls to > SetIsDirectoryOnFileThread(), I don't think we need a sequence here. > > You could simply write: > > base::PostTaskWithTraitsAndReply( > FROM_HERE, > {base::MayBlock(), base::TaskPriority::BACKGROUND}, > base::BindOnce(&FileSystem...), > base::BindOnce(&FileSystem...)); > > You can ask owners to confirm whether this is ok. SGTM, I'll check with the owner to make sure that it's fine to run this in parallel. https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:599: &FileSystemGetWritableEntryFunction::SetIsDirectoryOnFileThread, On 2017/05/16 15:23:26, fdoray wrote: > s/OnFileThread/Async/ Done. https://codereview.chromium.org/2877343003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:883: task_runner_->PostTask( On 2017/05/16 15:23:26, fdoray wrote: > ditto (a sequence may not be required) Done.
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Switch FileSystemRetainEntryFunction to the TaskScheduler API. BUG=689520 ========== to ========== Switch FileSystemRetainEntryFunction to the TaskScheduler API. See Francois' comment here: https://codereview.chromium.org/2877343003/#msg15 , I also don't think that we need a sequence here, but it'd be good if an owner was able to confirm this. BUG=689520 ==========
lgtm Possible message for owner: With this CL, tasks posted from this file can run in parallel. Previously, they were all posted to the file thread so that was not possible. Please verify that this is ok. Also, please verify that appropriate traits are used to post task https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h
Description was changed from ========== Switch FileSystemRetainEntryFunction to the TaskScheduler API. See Francois' comment here: https://codereview.chromium.org/2877343003/#msg15 , I also don't think that we need a sequence here, but it'd be good if an owner was able to confirm this. BUG=689520 ========== to ========== Switch FileSystemRetainEntryFunction to the TaskScheduler API. With this CL, tasks posted from this file can run in parallel. Previously, they were all posted to the file thread so that was not possible. Please verify that this is ok. Also, please verify that appropriate traits are used to post task https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h BUG=689520 ==========
sebmarchand@chromium.org changed reviewers: + benwells@chromium.org
Thanks, +benwells@, PTAL.
benwells@chromium.org changed reviewers: + mtomasz@chromium.org
I think this is OK but would like to get a second opinion. +mtomasz: I am thinking that the users of the file systems could not currently send multiple dependent file system requests without waiting for each one to finish. E.g. if you created a directory, and immediately tried to open that directory without waiting for the create to complete, you'd have problems. Put another way, sending these tasks all in parallel should be OK as the users of the API wouldn't be relying on the ordering, they would be waiting for completion when it mattered. https://codereview.chromium.org/2877343003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2877343003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:883: &FileSystemChooseEntryFunction::ConfirmDirectoryAccessOnFileThread, Could you rename ConfirmDirectoryAccessOnFileThread while you're here?
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
Thanks, waiting for mtomasz@'s review. https://codereview.chromium.org/2877343003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/file_system/file_system_api.cc (right): https://codereview.chromium.org/2877343003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/file_system/file_system_api.cc:883: &FileSystemChooseEntryFunction::ConfirmDirectoryAccessOnFileThread, On 2017/05/17 01:03:52, benwells wrote: > Could you rename ConfirmDirectoryAccessOnFileThread while you're here? Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please fix the CL title as this CL fixes different function that the title says. As Ben said, I think this change is safe. Also, this method is for picking files/directories with a file picker showing UI, so (1) it's unlikely that the app wants the user to pick something the app created just before, (2) the app must be using a callback to get the result or error, as otherwise there is no point calling this api (in contrast to eg. creating a directory). lgtm
Description was changed from ========== Switch FileSystemRetainEntryFunction to the TaskScheduler API. With this CL, tasks posted from this file can run in parallel. Previously, they were all posted to the file thread so that was not possible. Please verify that this is ok. Also, please verify that appropriate traits are used to post task https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h BUG=689520 ========== to ========== Switch browser/extensions/api/file_system/file_system_api.[h|cc] to the TaskScheduler API With this CL, tasks posted from this file can run in parallel. Previously, they were all posted to the file thread so that was not possible. Please verify that this is ok. Also, please verify that appropriate traits are used to post task https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h BUG=689520 ==========
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2877343003/#ps160001 (title: "Address Ben's comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495112728862590,
"parent_rev": "1d5b2074f9265b918ef7571e13cfc2c2d794d940", "commit_rev":
"8bf05bb205b2693eae180c0b69c7aa5385a3592f"}
Message was sent while issue was closed.
Description was changed from ========== Switch browser/extensions/api/file_system/file_system_api.[h|cc] to the TaskScheduler API With this CL, tasks posted from this file can run in parallel. Previously, they were all posted to the file thread so that was not possible. Please verify that this is ok. Also, please verify that appropriate traits are used to post task https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h BUG=689520 ========== to ========== Switch browser/extensions/api/file_system/file_system_api.[h|cc] to the TaskScheduler API With this CL, tasks posted from this file can run in parallel. Previously, they were all posted to the file thread so that was not possible. Please verify that this is ok. Also, please verify that appropriate traits are used to post task https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h BUG=689520 Review-Url: https://codereview.chromium.org/2877343003 Cr-Commit-Position: refs/heads/master@{#472785} Committed: https://chromium.googlesource.com/chromium/src/+/8bf05bb205b2693eae180c0b69c7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/chromium/src/+/8bf05bb205b2693eae180c0b69c7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
