Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628903002:
Change DONT_START_THREADS to DONT_CREATE_THREADS in TestBrowserThreadBundle. (Closed)
DescriptionChange DONT_START_THREADS to DONT_CREATE_THREADS in TestBrowserThreadBundle.
In a real browser process, threads are started as BrowserThreadImpl
objects are created. TestBrowserThreadBundle should do the same.
This is a pre-requisite to add the option to create a real TaskScheduler
in TestBrowserThreadBundle, because TaskScheduler has to be created and
started at the same time.
BUG=659191
Review-Url: https://codereview.chromium.org/2628903002
Cr-Commit-Position: refs/heads/master@{#443536}
Committed: https://chromium.googlesource.com/chromium/src/+/93605d248126928596c785734491ea7b8ea1b7cf
Patch Set 1 #Patch Set 2 : fix build error #Patch Set 3 : fix build error #Patch Set 4 : fix test error #Patch Set 5 : fix test error #
Total comments: 10
Patch Set 6 : CR gab #23 #Patch Set 7 : CR gab #27 #
Dependent Patchsets: Messages
Total messages: 42 (34 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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:80: new TestBrowserThread(BrowserThread::UI, message_loop_.get())); Why is the UI thread unconditionally created? https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:30: // Passing DONT_START_THREADS to constructor will delay creating other threads DONT_CREATE_THREADS https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:31: // until the test explicitly calls Start(). CreateThreads() https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:72: // DONT_START_THREADS option was used when the bundle was created. Update (and search for others?)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:80: new TestBrowserThread(BrowserThread::UI, message_loop_.get())); On 2017/01/12 19:47:35, gab wrote: > Why is the UI thread unconditionally created? TestBrowserThreadBundle::CreateThreads() mimics BrowserMainLoop::CreateThreads(). BrowserMainLoop::CreateThreads() initializes TaskScheduler and create non-UI BrowserThreads. The UI thread is created in BrowserMainLoop::MainMessageLoopStart(). Note: The only user of DONT_CREATE_THREADS is IOThreadTestWithIOThreadObject. This test needs the UI thread to exist (ThreadTaskRunnerHandle and DCHECK_CURRENTLY_ON(BrowserThread::UI) work on the main thread) when the IO thread hasn't been created yet. https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.h (right): https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:30: // Passing DONT_START_THREADS to constructor will delay creating other threads On 2017/01/12 19:47:35, gab wrote: > DONT_CREATE_THREADS Done. https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:31: // until the test explicitly calls Start(). On 2017/01/12 19:47:35, gab wrote: > CreateThreads() Done. https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.h:72: // DONT_START_THREADS option was used when the bundle was created. On 2017/01/12 19:47:35, gab wrote: > Update (and search for others?) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:80: new TestBrowserThread(BrowserThread::UI, message_loop_.get())); On 2017/01/12 19:59:10, fdoray wrote: > On 2017/01/12 19:47:35, gab wrote: > > Why is the UI thread unconditionally created? > > TestBrowserThreadBundle::CreateThreads() mimics > BrowserMainLoop::CreateThreads(). BrowserMainLoop::CreateThreads() initializes > TaskScheduler and create non-UI BrowserThreads. The UI thread is created in > BrowserMainLoop::MainMessageLoopStart(). > > Note: The only user of DONT_CREATE_THREADS is IOThreadTestWithIOThreadObject. > This test needs the UI thread to exist (ThreadTaskRunnerHandle and > DCHECK_CURRENTLY_ON(BrowserThread::UI) work on the main thread) when the IO > thread hasn't been created yet. I see, then add a comment to that effect here and 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 to run a CQ dry run
fdoray@chromium.org changed reviewers: + jochen@chromium.org
jochen@: PTAL https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2628903002/diff/80001/content/public/test/tes... content/public/test/test_browser_thread_bundle.cc:80: new TestBrowserThread(BrowserThread::UI, message_loop_.get())); On 2017/01/12 20:13:27, gab wrote: > On 2017/01/12 19:59:10, fdoray wrote: > > On 2017/01/12 19:47:35, gab wrote: > > > Why is the UI thread unconditionally created? > > > > TestBrowserThreadBundle::CreateThreads() mimics > > BrowserMainLoop::CreateThreads(). BrowserMainLoop::CreateThreads() initializes > > TaskScheduler and create non-UI BrowserThreads. The UI thread is created in > > BrowserMainLoop::MainMessageLoopStart(). > > > > Note: The only user of DONT_CREATE_THREADS is IOThreadTestWithIOThreadObject. > > This test needs the UI thread to exist (ThreadTaskRunnerHandle and > > DCHECK_CURRENTLY_ON(BrowserThread::UI) work on the main thread) when the IO > > thread hasn't been created yet. > > I see, then add a comment to that effect here and lgtm 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.
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2628903002/#ps120001 (title: "CR gab #27")
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": 120001, "attempt_start_ts": 1484309636295260,
"parent_rev": "0e1f16653bdf3cacd121bfbc1f32b25f01ac6aed", "commit_rev":
"93605d248126928596c785734491ea7b8ea1b7cf"}
Message was sent while issue was closed.
Description was changed from ========== Change DONT_START_THREADS to DONT_CREATE_THREADS in TestBrowserThreadBundle. In a real browser process, threads are started as BrowserThreadImpl objects are created. TestBrowserThreadBundle should do the same. This is a pre-requisite to add the option to create a real TaskScheduler in TestBrowserThreadBundle, because TaskScheduler has to be created and started at the same time. BUG=659191 ========== to ========== Change DONT_START_THREADS to DONT_CREATE_THREADS in TestBrowserThreadBundle. In a real browser process, threads are started as BrowserThreadImpl objects are created. TestBrowserThreadBundle should do the same. This is a pre-requisite to add the option to create a real TaskScheduler in TestBrowserThreadBundle, because TaskScheduler has to be created and started at the same time. BUG=659191 Review-Url: https://codereview.chromium.org/2628903002 Cr-Commit-Position: refs/heads/master@{#443536} Committed: https://chromium.googlesource.com/chromium/src/+/93605d248126928596c785734491... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/93605d248126928596c785734491... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
