Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890453002:
Implement MusContextFactory::SharedMainThreadContextProvider() (Closed)
DescriptionImplement MusContextFactory::SharedMainThreadContextProvider()
This CL implements MusContextFactory::SharedMainThreadContextProvider()
which is needed by exo for uploading resources to GPU.
BUG=720600
Review-Url: https://codereview.chromium.org/2890453002
Cr-Commit-Position: refs/heads/master@{#472237}
Committed: https://chromium.googlesource.com/chromium/src/+/9513fb02ee27908bacb21482a31ac3e800a135e9
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Address review issues #
Messages
Total messages: 18 (10 generated)
penghuang@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, PTAL. Thanks.
The CQ bit was checked by penghuang@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 penghuang@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...
https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... ui/aura/mus/mus_context_factory.cc:68: // immediately. Can you verify that here? i.e. have a bool flag that gets turned on in ::OnEstablishedGpuChannel() above, and either DCHECK() on the flag here, or return nullptr if the flag is not set.
https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... ui/aura/mus/mus_context_factory.cc:68: // immediately. On 2017/05/16 19:34:27, sadrul wrote: > Can you verify that here? i.e. have a bool flag that gets turned on in > ::OnEstablishedGpuChannel() above, and either DCHECK() on the flag here, or > return nullptr if the flag is not set. I think we shouldn't DCHECK it, because ui::ContextFactory is defined in this way. In most case, the GpuChannel has been established, if not we have to establish the channel synchronously. Also see [1], we also do the sync IPC in GpuProcessTransportFactory::SharedMainThreadContextProvider(). [1] https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...
https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... ui/aura/mus/mus_context_factory.cc:68: // immediately. On 2017/05/16 19:52:08, Peng wrote: > On 2017/05/16 19:34:27, sadrul wrote: > > Can you verify that here? i.e. have a bool flag that gets turned on in > > ::OnEstablishedGpuChannel() above, and either DCHECK() on the flag here, or > > return nullptr if the flag is not set. > > I think we shouldn't DCHECK it, because ui::ContextFactory is defined in this > way. In most case, the GpuChannel has been established, if not we have to > establish the channel synchronously. Also see [1], we also do the sync IPC in > GpuProcessTransportFactory::SharedMainThreadContextProvider(). > > [1] > https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t... If we cannot depend on this, can you remove/update the comment?
https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2890453002/diff/20001/ui/aura/mus/mus_context... ui/aura/mus/mus_context_factory.cc:68: // immediately. On 2017/05/16 19:53:27, sadrul wrote: > On 2017/05/16 19:52:08, Peng wrote: > > On 2017/05/16 19:34:27, sadrul wrote: > > > Can you verify that here? i.e. have a bool flag that gets turned on in > > > ::OnEstablishedGpuChannel() above, and either DCHECK() on the flag here, or > > > return nullptr if the flag is not set. > > > > I think we shouldn't DCHECK it, because ui::ContextFactory is defined in this > > way. In most case, the GpuChannel has been established, if not we have to > > establish the channel synchronously. Also see [1], we also do the sync IPC in > > GpuProcessTransportFactory::SharedMainThreadContextProvider(). > > > > [1] > > > https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t... > > If we cannot depend on this, can you remove/update the comment? Removed. Done
lgtm
The CQ bit was checked by penghuang@chromium.org
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": 40001, "attempt_start_ts": 1494964854455100,
"parent_rev": "63a028cbc670c8aa4ca59c1ec56b14b22d433ebb", "commit_rev":
"9513fb02ee27908bacb21482a31ac3e800a135e9"}
Message was sent while issue was closed.
Description was changed from ========== Implement MusContextFactory::SharedMainThreadContextProvider() This CL implements MusContextFactory::SharedMainThreadContextProvider() which is needed by exo for uploading resources to GPU. BUG=720600 ========== to ========== Implement MusContextFactory::SharedMainThreadContextProvider() This CL implements MusContextFactory::SharedMainThreadContextProvider() which is needed by exo for uploading resources to GPU. BUG=720600 Review-Url: https://codereview.chromium.org/2890453002 Cr-Commit-Position: refs/heads/master@{#472237} Committed: https://chromium.googlesource.com/chromium/src/+/9513fb02ee27908bacb21482a31a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9513fb02ee27908bacb21482a31a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
