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

Issue 85693007: cc: Defer first OutputSurface creation until client is ready (Closed)

Created:
7 years ago by no sievers
Modified:
7 years ago
CC:
chromium-reviews, jbauman+watch_chromium.org, Ian Vollick, jam, sievers+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, cc-bugs_chromium.org, danakj+watch_chromium.org, scottmg
Visibility:
Public.

Description

cc: Defer first OutputSurface creation until client is ready Remove the |first_output_surface| which was not used before the client signals ready. This allows the client to wait before creating a graphics context until the gpu thread and client channel are set up. BUG=270179, 329739 R=danakj@chromium.org, jamesr@chromium.org, jochen@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238458 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241897

Patch Set 1 #

Total comments: 7

Patch Set 2 : remove |first_output_surface_| #

Patch Set 3 : add comment #

Patch Set 4 : add tests #

Total comments: 1

Patch Set 5 : comment, rebase #

Patch Set 6 : rebase #

Patch Set 7 : rebase #

Patch Set 8 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -105 lines) Patch
M cc/test/fake_proxy.h View 1 1 chunk +1 line, -1 line 0 comments Download
M cc/test/layer_tree_test.h View 1 2 3 4 5 6 2 chunks +5 lines, -4 lines 0 comments Download
M cc/test/layer_tree_test.cc View 1 2 3 4 5 6 3 chunks +7 lines, -5 lines 0 comments Download
M cc/trees/layer_tree_host.h View 1 2 3 4 5 6 2 chunks +4 lines, -4 lines 0 comments Download
M cc/trees/layer_tree_host.cc View 1 2 3 4 5 6 3 chunks +10 lines, -18 lines 0 comments Download
M cc/trees/layer_tree_host_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M cc/trees/layer_tree_host_unittest_context.cc View 1 2 3 4 5 6 4 chunks +127 lines, -38 lines 0 comments Download
M cc/trees/proxy.h View 1 1 chunk +1 line, -1 line 0 comments Download
M cc/trees/single_thread_proxy.h View 1 2 3 4 5 6 2 chunks +1 line, -5 lines 0 comments Download
M cc/trees/single_thread_proxy.cc View 1 2 3 4 5 6 2 chunks +3 lines, -6 lines 0 comments Download
M cc/trees/thread_proxy.h View 1 2 3 4 5 6 2 chunks +1 line, -4 lines 0 comments Download
M cc/trees/thread_proxy.cc View 1 2 3 4 5 6 3 chunks +3 lines, -6 lines 0 comments Download
M chrome/test/gpu/gpu_feature_browsertest.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -0 lines 0 comments Download
M content/renderer/gpu/render_widget_compositor.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/gpu/render_widget_compositor.cc View 1 2 3 4 5 6 3 chunks +3 lines, -4 lines 0 comments Download
M content/test/test_webkit_platform_support.cc View 1 chunk +1 line, -2 lines 0 comments Download
M content/test/web_layer_tree_view_impl_for_testing.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M content/test/web_layer_tree_view_impl_for_testing.cc View 1 2 3 4 2 chunks +2 lines, -4 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
no sievers
One subtle change is that when a client creates an LTH and the first OutputSurface ...
7 years ago (2013-11-25 23:09:44 UTC) #1
danakj
This LGTM but I have one question https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host_unittest_context.cc File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host_unittest_context.cc#newcode1289 cc/trees/layer_tree_host_unittest_context.cc:1289: if (layer_tree_host()->source_frame_number() ...
7 years ago (2013-11-26 16:35:15 UTC) #2
jamesr
https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h#newcode98 cc/trees/layer_tree_host.h:98: scoped_ptr<OutputSurface> first_output_surface); hmm, why do you need this? we ...
7 years ago (2013-11-26 18:20:27 UTC) #3
no sievers
https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h#newcode98 cc/trees/layer_tree_host.h:98: scoped_ptr<OutputSurface> first_output_surface); On 2013/11/26 18:20:28, jamesr wrote: > hmm, ...
7 years ago (2013-11-26 19:28:37 UTC) #4
no sievers
https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host_unittest_context.cc File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host_unittest_context.cc#newcode1289 cc/trees/layer_tree_host_unittest_context.cc:1289: if (layer_tree_host()->source_frame_number() > 0) { On 2013/11/26 19:28:37, sievers ...
7 years ago (2013-11-26 19:30:23 UTC) #5
danakj
https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host_unittest_context.cc File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host_unittest_context.cc#newcode1289 cc/trees/layer_tree_host_unittest_context.cc:1289: if (layer_tree_host()->source_frame_number() > 0) { On 2013/11/26 19:30:23, sievers ...
7 years ago (2013-11-26 19:44:57 UTC) #6
no sievers
https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h#newcode98 cc/trees/layer_tree_host.h:98: scoped_ptr<OutputSurface> first_output_surface); On 2013/11/26 19:28:37, sievers wrote: > On ...
7 years ago (2013-11-26 22:19:56 UTC) #7
no sievers
On 2013/11/26 22:19:56, sievers wrote: > https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h > File cc/trees/layer_tree_host.h (right): > > https://codereview.chromium.org/85693007/diff/1/cc/trees/layer_tree_host.h#newcode98 > ...
7 years ago (2013-11-26 22:21:22 UTC) #8
no sievers
New version that removes first_output_surface entirely.
7 years ago (2013-11-26 22:57:19 UTC) #9
danakj
Can you add a couple tests like LayerTreeHostContextTestOffscreenContextFails but for the OutputSurface failing create of ...
7 years ago (2013-11-26 23:04:20 UTC) #10
no sievers
On 2013/11/26 23:04:20, danakj wrote: > Can you add a couple tests like LayerTreeHostContextTestOffscreenContextFails > ...
7 years ago (2013-11-28 00:57:12 UTC) #11
danakj
Thanks! Tests LGTM https://codereview.chromium.org/85693007/diff/70001/cc/trees/layer_tree_host_unittest_context.cc File cc/trees/layer_tree_host_unittest_context.cc (right): https://codereview.chromium.org/85693007/diff/70001/cc/trees/layer_tree_host_unittest_context.cc#newcode622 cc/trees/layer_tree_host_unittest_context.cc:622: // After 4 failures we expect ...
7 years ago (2013-12-02 14:53:02 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/85693007/90001
7 years ago (2013-12-03 00:13:43 UTC) #13
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=38995
7 years ago (2013-12-03 00:29:45 UTC) #14
jamesr
content/renderer/ lgtm. I don't have owners for content/test/, but those lgtm as well. +jochen for ...
7 years ago (2013-12-03 00:31:09 UTC) #15
jochen (gone - plz use gerrit)
stamp-lgtm
7 years ago (2013-12-03 08:45:18 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/85693007/90001
7 years ago (2013-12-03 16:23:28 UTC) #17
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=196166
7 years ago (2013-12-03 18:17:51 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/85693007/90001
7 years ago (2013-12-03 18:19:54 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/85693007/90001
7 years ago (2013-12-03 19:12:56 UTC) #20
no sievers
Committed patchset #6 manually as r238458 (presubmit successful).
7 years ago (2013-12-03 21:21:02 UTC) #21
robliao
A revert of this CL has been created in https://codereview.chromium.org/101573005/ by robliao@chromium.org. The reason for ...
7 years ago (2013-12-03 22:35:43 UTC) #22
no sievers
7 years ago (2013-12-19 18:56:12 UTC) #23
Message was sent while issue was closed.
Committed patchset #8 manually as r241897 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698