Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2728183002: RendererCompositorFrameSink should handle local surface id allocation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 3 weeks ago by Saman Sami
Modified:
6 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, piman+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, kalyank, danakj+watch_chromium.org, James Su, xjz+watch_chromium.org, pfeldman, miu+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

RendererCompositorFrameSink should handle local surface id allocation We are planning to use MojoCompositorFrameSink in the renderer process, which means RendererCompositorFrameSink must be aware of LocalSurfaceId of the surface it's submitting to. BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2728183002 Cr-Commit-Position: refs/heads/master@{#458556} Committed: https://chromium.googlesource.com/chromium/src/+/e7398c55e00a085cc03f10e16f3f42fe37f980ac

Patch Set 1 #

Patch Set 2 : Remove ShouldCreateNewSurfaceId #

Patch Set 3 : Fixed unit tests #

Patch Set 4 : Fixed mac #

Patch Set 5 : Fixed Android #

Patch Set 6 : Fixed Android #

Patch Set 7 : Cleanup #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Patch Set 10 : c #

Patch Set 11 : rebase #

Patch Set 12 : fix #

Patch Set 13 : Clean-up android #

Patch Set 14 : DFH Android #

Patch Set 15 : Remove LocalSurfaceId allocators #

Patch Set 16 : Set factory in CreateSurface #

Patch Set 17 : Rebase #

Total comments: 52

Patch Set 18 : const #

Patch Set 19 : Fix DelegatedFrameHost #

Patch Set 20 : Android #

Patch Set 21 : Fixed #

Patch Set 22 : Update RendererCompositorFrameSink #

Patch Set 23 : Added comment #

Total comments: 3

Patch Set 24 : Add checks to RWHI #

Patch Set 25 : Revert cc/ #

Patch Set 26 : rebase #

Patch Set 27 : Updated unittest #

Patch Set 28 : up #

Patch Set 29 : Fix unit tst #

Patch Set 30 : notreached #

Total comments: 9

Patch Set 31 : Remove NOTREACHED #

Patch Set 32 : c #

Patch Set 33 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -251 lines) Patch
M content/browser/devtools/render_frame_devtools_agent_host.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -5 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +2 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +11 lines, -16 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 5 chunks +25 lines, -17 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest_unittest.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +7 lines, -4 lines 0 comments Download
M content/browser/renderer_host/browser_compositor_view_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/browser_compositor_view_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -1 line 0 comments Download
M content/browser/renderer_host/delegated_frame_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 3 chunks +1 line, -2 lines 0 comments Download
M content/browser/renderer_host/delegated_frame_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 5 chunks +9 lines, -13 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 2 chunks +30 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +54 lines, -26 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +6 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +3 lines, -4 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 34 chunks +76 lines, -73 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +2 lines, -1 line 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +2 lines, -1 line 0 comments Download
M content/public/test/browser_test_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/gpu/renderer_compositor_frame_sink.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +23 lines, -0 lines 0 comments Download
M content/renderer/gpu/renderer_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +53 lines, -3 lines 0 comments Download
M content/test/test_render_view_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M content/test/test_render_view_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +1 line, -0 lines 0 comments Download
M ui/android/delegated_frame_host_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 3 chunks +5 lines, -18 lines 0 comments Download
M ui/android/delegated_frame_host_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 8 chunks +21 lines, -58 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 146 (127 generated)
Fady Samuel
I agree that updating the factory in SurfaceManager is the right solution but this needs ...
6 months, 1 week ago (2017-03-15 12:05:48 UTC) #61
Saman Sami
PTAL https://codereview.chromium.org/2728183002/diff/320001/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2728183002/diff/320001/cc/surfaces/surface_manager.cc#newcode125 cc/surfaces/surface_manager.cc:125: surface->set_factory(surface_factory); On 2017/03/15 12:05:47, Fady Samuel wrote: > ...
6 months, 1 week ago (2017-03-16 18:33:08 UTC) #76
Fady Samuel
LGTM! Yaay! Please pass this to piman@ for overall review! Thanks! This takes us in ...
6 months, 1 week ago (2017-03-16 23:34:13 UTC) #84
Saman Sami
piman@ PTAL The cc/ files will land separately and will be removed from the final ...
6 months, 1 week ago (2017-03-16 23:45:44 UTC) #86
piman
It seems almost too easy, but I don't see anything obvious that would be wrong. ...
6 months, 1 week ago (2017-03-17 00:05:39 UTC) #87
Fady Samuel
WDYT about my comment of doing the security check some place central such as Surface::QueueFrame ...
6 months, 1 week ago (2017-03-17 00:33:12 UTC) #88
piman
https://codereview.chromium.org/2728183002/diff/440001/content/browser/renderer_host/delegated_frame_host.cc File content/browser/renderer_host/delegated_frame_host.cc (right): https://codereview.chromium.org/2728183002/diff/440001/content/browser/renderer_host/delegated_frame_host.cc#newcode451 content/browser/renderer_host/delegated_frame_host.cc:451: if (local_surface_id != local_surface_id_ || !has_frame_) { On 2017/03/17 ...
6 months, 1 week ago (2017-03-17 17:40:26 UTC) #89
Saman Sami
PTAL I added code for rejecting messages when the renderer does not allocate a new ...
6 months, 1 week ago (2017-03-20 22:16:21 UTC) #108
Saman Sami
tsepez@: Please review IPC.
6 months, 1 week ago (2017-03-20 22:18:23 UTC) #110
Saman Sami
boliu@: Please review *android* and RendererCompositorFrameSink
6 months, 1 week ago (2017-03-20 22:25:31 UTC) #112
piman
LGTM % nits / question https://codereview.chromium.org/2728183002/diff/580001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2728183002/diff/580001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1834 content/browser/renderer_host/render_widget_host_impl.cc:1834: NOTREACHED(); nit: not NOTREACHED ...
6 months, 1 week ago (2017-03-20 22:51:45 UTC) #113
boliu
I don't own RendererCompositorFrameSink, although it lgtm. And android lgtm % comment https://codereview.chromium.org/2728183002/diff/580001/ui/android/delegated_frame_host_android.h File ui/android/delegated_frame_host_android.h ...
6 months, 1 week ago (2017-03-20 23:06:01 UTC) #114
Tom Sepez
LGTM, given the discussion we've had in the past about uniquenes of surface IDs.
6 months, 1 week ago (2017-03-20 23:20:41 UTC) #117
Saman Sami
https://codereview.chromium.org/2728183002/diff/580001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2728183002/diff/580001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1849 content/browser/renderer_host/render_widget_host_impl.cc:1849: last_local_surface_id_ = local_surface_id; On 2017/03/20 22:51:45, piman wrote: > ...
6 months, 1 week ago (2017-03-21 02:10:25 UTC) #118
Saman Sami
https://codereview.chromium.org/2728183002/diff/580001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2728183002/diff/580001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1834 content/browser/renderer_host/render_widget_host_impl.cc:1834: NOTREACHED(); On 2017/03/20 22:51:45, piman wrote: > nit: not ...
6 months, 1 week ago (2017-03-21 16:33:38 UTC) #121
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/2728183002/620001
6 months ago (2017-03-21 18:16:20 UTC) #130
commit-bot: I haz the power
Failed to apply patch for content/renderer/gpu/renderer_compositor_frame_sink.h: While running git apply --index -p1; error: patch failed: ...
6 months ago (2017-03-21 18:43:30 UTC) #132
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/2728183002/640001
6 months ago (2017-03-21 21:25:48 UTC) #143
commit-bot: I haz the power
6 months ago (2017-03-21 21:37:27 UTC) #146
Message was sent while issue was closed.
Committed patchset #33 (id:640001) as
https://chromium.googlesource.com/chromium/src/+/e7398c55e00a085cc03f10e16f3f...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b