Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(993)

Issue 2608093002: Use TaskScheduler instead of WorkerPool in file_ios.cc. (Closed)

Created:
3 years, 11 months ago by fdoray
Modified:
3 years, 11 months ago
Reviewers:
gavinp
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use TaskScheduler instead of WorkerPool in file_ios.cc. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). Note: Tasks that were previously posted to base::WorkerPool should use this shutdown behavior because this is how base::WorkerPool handles all its tasks. May Block: Tasks posted with MayBlock() may block. This includes but is not limited to tasks that wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. This trait isn't required for the mere use of locks. BUG=659191 Review-Url: https://codereview.chromium.org/2608093002 Cr-Commit-Position: refs/heads/master@{#442971} Committed: https://chromium.googlesource.com/chromium/src/+/34db9f35de6a4531e82d3bf068f0529ca94b348b

Patch Set 1 #

Patch Set 2 : Fix build error #

Patch Set 3 : Add ScopedTaskScheduler #

Patch Set 4 : ScopedAsyncTaskScheduler #

Patch Set 5 : self-review #

Patch Set 6 : self-review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -5 lines) Patch
M net/disk_cache/blockfile/file_ios.cc View 1 3 chunks +13 lines, -5 lines 0 comments Download
M net/disk_cache/disk_cache_test_base.h View 1 2 3 4 5 2 chunks +7 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 33 (26 generated)
fdoray
PTAL
3 years, 11 months ago (2017-01-11 14:37:30 UTC) #21
gavinp
lgtm
3 years, 11 months ago (2017-01-11 18:34:43 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2608093002/100001
3 years, 11 months ago (2017-01-11 18:35:35 UTC) #27
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/34db9f35de6a4531e82d3bf068f0529ca94b348b
3 years, 11 months ago (2017-01-11 19:04:20 UTC) #30
krasin1
On 2017/01/11 19:04:20, commit-bot: I haz the power wrote: > Committed patchset #6 (id:100001) as ...
3 years, 11 months ago (2017-01-11 22:28:27 UTC) #31
krasin1
Sadly, I am failing to reproduce the issue locally. Let's wait for more bots failures. ...
3 years, 11 months ago (2017-01-12 00:08:41 UTC) #32
mef
3 years, 11 months ago (2017-01-17 19:31:14 UTC) #33
Message was sent while issue was closed.
On 2017/01/12 00:08:41, krasin1 wrote:
> Sadly, I am failing to reproduce the issue locally. Let's wait for more bots
> failures. Since it was a Clang ToT bot failing, there's a non zero chance that
> the issue was on the LLVM side, not on Chromium side.

This also makes Cronet_tests for iOS failing, but we don't have any testbots on
CQ or even separate waterfall for that yet.
There is potential fix for Cronet tests from lilyhoughton@ here:
https://codereview.chromium.org/2639673002/
Could you take a look to see if it makes sense?

Powered by Google App Engine
This is Rietveld 408576698