|
|
DescriptionCreate TaskScheduler in BrowserMainLoop constructor.
This CL allows tasks to be posted via base/task_scheduler/post_task.h
as soon as BrowserMainLoop is instantiated. Tasks don't run until
TaskScheduler::Start() is called in BrowserMainLoop::CreateThreads().
BUG=690706
Review-Url: https://codereview.chromium.org/2835933004
Cr-Commit-Position: refs/heads/master@{#467283}
Committed: https://chromium.googlesource.com/chromium/src/+/54151d830574c77a0673105e99076bfa1fbd792a
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR-robliao-9 #
Depends on Patchset: Messages
Total messages: 22 (14 generated)
The CQ bit was checked by fdoray@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.
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:536: base::TaskScheduler::Create(""); Should we be more aggressive about the creation point now? Maybe browser_main_runner is the place to be?
lgtm https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:536: base::TaskScheduler::Create(""); On 2017/04/24 18:15:54, robliao wrote: > Should we be more aggressive about the creation point now? Maybe > browser_main_runner is the place to be? Agreed for ASAP but this is essentially the same logical point: BrowserMainLoop is called from BrowserMainRunnerImpl::Initialize() and BrowserMain() invokes BrowserMainRunner::Create() and BrowserMainRunnerImpl::Initialize() in a row @ https://cs.chromium.org/chromium/src/content/browser/browser_main.cc?rcl=76b4... So since this boils down to pretty much as early as possible in both cases, I'd say here is better since it's what manages threads and pools.
lgtm https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:536: base::TaskScheduler::Create(""); On 2017/04/25 15:29:15, gab wrote: > On 2017/04/24 18:15:54, robliao wrote: > > Should we be more aggressive about the creation point now? Maybe > > browser_main_runner is the place to be? > > Agreed for ASAP but this is essentially the same logical point: BrowserMainLoop > is called from BrowserMainRunnerImpl::Initialize() and BrowserMain() invokes > BrowserMainRunner::Create() and BrowserMainRunnerImpl::Initialize() in a row @ > https://cs.chromium.org/chromium/src/content/browser/browser_main.cc?rcl=76b4... > > So since this boils down to pretty much as early as possible in both cases, I'd > say here is better since it's what manages threads and pools. I think this change is fine for now, but given that we will be removing the browser threads management portion long term, it may make more sense to abstract that outside of BrowserMainLoop when we arrive at that point. https://codereview.chromium.org/2835933004/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:538: Document the use of "".
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
fdoray@chromium.org changed reviewers: + jam@chromium.org
jam@: PTAL
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2835933004/#ps20001 (title: "CR-robliao-9")
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": 20001, "attempt_start_ts": 1493200590675170, "parent_rev": "a7695ef873dfa0530d2ea4c1088e2fb67719607a", "commit_rev": "54151d830574c77a0673105e99076bfa1fbd792a"}
Message was sent while issue was closed.
Description was changed from ========== Create TaskScheduler in BrowserMainLoop constructor. This CL allows tasks to be posted via base/task_scheduler/post_task.h as soon as BrowserMainLoop is instantiated. Tasks don't run until TaskScheduler::Start() is called in BrowserMainLoop::CreateThreads(). BUG=690706 ========== to ========== Create TaskScheduler in BrowserMainLoop constructor. This CL allows tasks to be posted via base/task_scheduler/post_task.h as soon as BrowserMainLoop is instantiated. Tasks don't run until TaskScheduler::Start() is called in BrowserMainLoop::CreateThreads(). BUG=690706 Review-Url: https://codereview.chromium.org/2835933004 Cr-Commit-Position: refs/heads/master@{#467283} Committed: https://chromium.googlesource.com/chromium/src/+/54151d830574c77a0673105e9907... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/54151d830574c77a0673105e9907... |