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

Issue 2879463002: Initialize RendererSettings in ContextFactory (Closed)

Created:
3 years, 7 months ago by Alex Z.
Modified:
3 years, 7 months ago
Reviewers:
danakj, sadrul
CC:
chromium-reviews, sadrul, Ian Vollick, jam, jbauman+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, cc-bugs_chromium.org, Fady Samuel
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Initialize RendererSettings in ContextFactory Implementations of ContextFactory initialize RendererSettings based on switches on the command line. ui::Compositor gets RendererSettings via ContextFactory::GetRendererSettings. ui::Compositor::GetRendererSettings and ContextFactory::GetImageTextureTarget is removed since it is no longer being used. By centralizing the initialization of RendererSettings into ContextFactory, there is no longer any usage of LayerTreeSettings::renderer_settings outside of LayerTreeHostImpl. Therefore, this CL brings us one step closer to having separate LayerTreeSettings and RendererSettings. BUG=711146 Review-Url: https://codereview.chromium.org/2879463002 Cr-Commit-Position: refs/heads/master@{#471445} Committed: https://chromium.googlesource.com/chromium/src/+/3f98e54b610b6f3ddbf07cc610380a77e554c7c5

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Fix FakeContextFactory #

Total comments: 10

Patch Set 4 : Address comments #

Total comments: 13

Patch Set 5 : Address comments #

Total comments: 4

Patch Set 6 : const RendererSettings #

Unified diffs Side-by-side diffs Delta from patch set Stats (+192 lines, -82 lines) Patch
M content/browser/compositor/gpu_process_transport_factory.h View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments Download
M content/browser/compositor/gpu_process_transport_factory.cc View 1 2 3 4 5 6 chunks +11 lines, -7 lines 0 comments Download
M ui/aura/mus/mus_context_factory.h View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M ui/aura/mus/mus_context_factory.cc View 1 2 3 4 5 3 chunks +17 lines, -7 lines 0 comments Download
M ui/compositor/BUILD.gn View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M ui/compositor/compositor.h View 1 2 chunks +3 lines, -5 lines 0 comments Download
M ui/compositor/compositor.cc View 1 3 chunks +3 lines, -37 lines 0 comments Download
A ui/compositor/compositor_util.h View 1 2 3 4 1 chunk +24 lines, -0 lines 0 comments Download
A ui/compositor/compositor_util.cc View 1 2 3 4 1 chunk +55 lines, -0 lines 0 comments Download
M ui/compositor/test/fake_context_factory.h View 4 chunks +5 lines, -4 lines 0 comments Download
M ui/compositor/test/fake_context_factory.cc View 1 2 3 4 3 chunks +32 lines, -5 lines 0 comments Download
M ui/compositor/test/in_process_context_factory.h View 3 chunks +2 lines, -2 lines 0 comments Download
M ui/compositor/test/in_process_context_factory.cc View 1 2 3 4 6 chunks +32 lines, -11 lines 0 comments Download

Messages

Total messages: 43 (29 generated)
Alex Z.
danakj@: This CL should unblock having buffer_to_texture_target_map in both LTSettings and RendererSettings. PTAL. Thank you!
3 years, 7 months ago (2017-05-10 21:06:17 UTC) #5
danakj
LG but let's make the test code simpler https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc#newcode30 ui/aura/mus/mus_context_factory.cc:30: renderer_settings_.gl_composited_texture_quad_border ...
3 years, 7 months ago (2017-05-11 15:17:15 UTC) #16
Alex Z.
https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc#newcode48 ui/aura/mus/mus_context_factory.cc:48: uint32_t target = GL_TEXTURE_2D; On 2017/05/11 15:17:15, danakj wrote: ...
3 years, 7 months ago (2017-05-11 17:28:32 UTC) #17
danakj
https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc#newcode48 ui/aura/mus/mus_context_factory.cc:48: uint32_t target = GL_TEXTURE_2D; On 2017/05/11 17:28:32, Alex Z. ...
3 years, 7 months ago (2017-05-11 17:34:53 UTC) #18
Alex Z.
https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2879463002/diff/40001/ui/aura/mus/mus_context_factory.cc#newcode30 ui/aura/mus/mus_context_factory.cc:30: renderer_settings_.gl_composited_texture_quad_border = On 2017/05/11 15:17:15, danakj wrote: > It ...
3 years, 7 months ago (2017-05-11 19:25:22 UTC) #20
danakj
Thanks, I like the shared method. Few comments https://codereview.chromium.org/2879463002/diff/40001/ui/compositor/test/fake_context_factory.cc File ui/compositor/test/fake_context_factory.cc (right): https://codereview.chromium.org/2879463002/diff/40001/ui/compositor/test/fake_context_factory.cc#newcode23 ui/compositor/test/fake_context_factory.cc:23: renderer_settings_.partial_swap_enabled ...
3 years, 7 months ago (2017-05-11 20:10:17 UTC) #22
Alex Z.
https://codereview.chromium.org/2879463002/diff/60001/content/browser/compositor/gpu_process_transport_factory.cc File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2879463002/diff/60001/content/browser/compositor/gpu_process_transport_factory.cc#newcode216 content/browser/compositor/gpu_process_transport_factory.cc:216: ui::InitializeRendererSettings(base::Bind(&gpu::GetImageTextureTarget), On 2017/05/11 20:10:16, danakj wrote: > Bind/Callback feels ...
3 years, 7 months ago (2017-05-12 14:12:36 UTC) #27
danakj
LGTM https://codereview.chromium.org/2879463002/diff/60001/ui/compositor/test/fake_context_factory.cc File ui/compositor/test/fake_context_factory.cc (right): https://codereview.chromium.org/2879463002/diff/60001/ui/compositor/test/fake_context_factory.cc#newcode23 ui/compositor/test/fake_context_factory.cc:23: renderer_settings_.partial_swap_enabled = false; On 2017/05/12 14:12:36, Alex Z. ...
3 years, 7 months ago (2017-05-12 14:21:37 UTC) #28
Alex Z.
sadrul@chromium.org: Please review changes in mus_context_factory*
3 years, 7 months ago (2017-05-12 14:32:28 UTC) #30
Alex Z.
https://codereview.chromium.org/2879463002/diff/80001/content/browser/compositor/gpu_process_transport_factory.cc File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2879463002/diff/80001/content/browser/compositor/gpu_process_transport_factory.cc#newcode221 content/browser/compositor/gpu_process_transport_factory.cc:221: renderer_settings_ = ui::CreateRendererSettings(&gpu::GetImageTextureTarget); On 2017/05/12 14:21:37, danakj wrote: > ...
3 years, 7 months ago (2017-05-12 14:37:49 UTC) #32
sadrul
3 years, 7 months ago (2017-05-12 20:49:17 UTC) #36
sadrul
lgtm
3 years, 7 months ago (2017-05-12 20:49:18 UTC) #37
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/2879463002/100001
3 years, 7 months ago (2017-05-12 21:10:58 UTC) #40
commit-bot: I haz the power
3 years, 7 months ago (2017-05-12 21:21:39 UTC) #43
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3f98e54b610b6f3ddbf07cc61038...

Powered by Google App Engine
This is Rietveld 408576698