Chromium Code Reviews
Help | Chromium Project | Sign in
(5)

Issue 2740833005: [cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 5 days ago by Eric Seckler
Modified:
1 week, 3 days ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, xjz+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, James Su, danakj+watch_chromium.org, miu+watch_chromium.org, Sami, enne, Tom Sepez, Alexei Svitkine (slow)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH. This patch adds forwarding of BeginFrameAcks from the renderer through the SwapCompositorFrame and BeginFrameDidNotSwap IPCs to the browser, where they are received and passed on by RenderWidgetHostViewAura. Other platforms to follow. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2740833005 Cr-Commit-Position: refs/heads/master@{#457962} Committed: https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf108411491f38632cc

Patch Set 1 #

Total comments: 11

Patch Set 2 : updated mojoms, rename new IPC. #

Total comments: 15

Patch Set 3 : move latest_confirmed tracking to DFH. #

Patch Set 4 : Add BeginFrameAck to compositor_frame_metadata.mojom. #

Patch Set 5 : add struct_traits tests. #

Total comments: 7

Patch Set 6 : move setting has_damage and dchecks to RWHI. #

Patch Set 7 : moved Read() back to .cc #

Patch Set 8 : fix android compilation. #

Patch Set 9 : fix RWH unittest. #

Patch Set 10 : add todo to QueueMessageSwapPromise::DidNotSwap. #

Total comments: 11

Patch Set 11 : address security comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+429 lines, -23 lines) Patch
M cc/ipc/begin_frame_args.mojom View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M cc/ipc/begin_frame_args.typemap View 1 3 4 5 6 1 chunk +11 lines, -2 lines 0 comments Download
A + cc/ipc/begin_frame_args_for_blink.typemap View 1 2 3 4 5 6 1 chunk +8 lines, -2 lines 0 comments Download
M cc/ipc/begin_frame_args_struct_traits.h View 1 2 3 4 5 6 1 chunk +19 lines, -12 lines 0 comments Download
A cc/ipc/begin_frame_args_struct_traits.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +39 lines, -0 lines 0 comments Download
M cc/ipc/cc_param_traits_macros.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -0 lines 0 comments Download
M cc/ipc/cc_serialization_perftest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata.mojom View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata_struct_traits.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata_struct_traits.cc View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M cc/ipc/struct_traits_unittest.cc View 1 2 3 4 5 6 7 8 chunks +39 lines, -0 lines 0 comments Download
M cc/ipc/traits_test_service.mojom View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M cc/scheduler/begin_frame_source.h View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/bad_message.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/delegated_frame_host.h View 1 2 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/renderer_host/delegated_frame_host.cc View 1 2 1 chunk +25 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 4 5 2 chunks +5 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 3 chunks +32 lines, -0 lines 2 comments Download
M content/browser/renderer_host/render_widget_host_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 1 chunk +11 lines, -0 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 3 chunks +163 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 chunks +6 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/gpu/compositor_external_begin_frame_source.h View 1 chunk +1 line, -2 lines 0 comments Download
M content/renderer/gpu/compositor_external_begin_frame_source.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +10 lines, -1 line 0 comments Download
M content/renderer/gpu/queue_message_swap_promise.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 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 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/public/blink_typemaps.gni View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 77 (39 generated)
Eric Seckler
Hi Brian, I'm wondering if there's a sensible way to test that the renderer actually ...
2 weeks, 5 days ago (2017-03-10 16:36:52 UTC) #4
Fady Samuel
Drive-by: Please update mojoms too! https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h File cc/ipc/cc_param_traits_macros.h (right): https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack) Please update mojoms ...
2 weeks, 5 days ago (2017-03-10 16:39:30 UTC) #6
danakj
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h File cc/ipc/cc_param_traits_macros.h (right): https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack) On 2017/03/10 16:39:30, Fady Samuel wrote: > Please ...
2 weeks, 5 days ago (2017-03-10 16:40:46 UTC) #8
Fady Samuel
On 2017/03/10 16:40:46, danakj wrote: > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h > File cc/ipc/cc_param_traits_macros.h (right): > > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 > ...
2 weeks, 5 days ago (2017-03-10 16:44:53 UTC) #9
Eric Seckler
On 2017/03/10 16:40:46, danakj wrote: > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h > File cc/ipc/cc_param_traits_macros.h (right): > > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 > ...
2 weeks, 5 days ago (2017-03-10 16:48:37 UTC) #10
Fady Samuel
On 2017/03/10 16:48:37, Eric Seckler wrote: > On 2017/03/10 16:40:46, danakj wrote: > > > ...
2 weeks, 5 days ago (2017-03-10 16:49:28 UTC) #11
Eric Seckler
On 2017/03/10 16:49:28, Fady Samuel wrote: > On 2017/03/10 16:48:37, Eric Seckler wrote: > > ...
2 weeks, 5 days ago (2017-03-10 16:53:57 UTC) #12
brianderson
I wouldn't worry about integration tests. This code path will be exercised extensively as part ...
2 weeks, 2 days ago (2017-03-13 22:55:39 UTC) #13
Eric Seckler
+dcheng for cc/ipc. Brian, Fady: PTAL at comment in rwhva.cc :) Daniel: I was trying ...
2 weeks, 1 day ago (2017-03-14 11:13:42 UTC) #15
Fady Samuel
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode975 content/browser/renderer_host/render_widget_host_view_aura.cc:975: begin_frame_source_->DidFinishFrame(this, ack); On 2017/03/14 11:13:41, Eric Seckler wrote: > ...
2 weeks, 1 day ago (2017-03-14 11:31:38 UTC) #16
Eric Seckler
On 2017/03/14 11:31:38, Fady Samuel wrote: > https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode975 ...
2 weeks, 1 day ago (2017-03-14 11:41:55 UTC) #17
Eric Seckler
On 2017/03/14 11:41:55, Eric Seckler wrote: > On 2017/03/14 11:31:38, Fady Samuel wrote: > > ...
2 weeks, 1 day ago (2017-03-14 12:46:58 UTC) #18
Fady Samuel
On 2017/03/14 12:46:58, Eric Seckler wrote: > On 2017/03/14 11:41:55, Eric Seckler wrote: > > ...
2 weeks, 1 day ago (2017-03-14 12:48:46 UTC) #19
Eric Seckler
On 2017/03/14 12:46:58, Eric Seckler wrote: > It looks like the interface used between CompositorFrameSinkSupport ...
2 weeks, 1 day ago (2017-03-14 15:53:00 UTC) #21
Eric Seckler
+tsepez@ for cc/ipc Fady mentioned that Tom is likely more familar with cc/mus-related IPCs. Tom, ...
2 weeks, 1 day ago (2017-03-14 16:02:49 UTC) #24
Tom Sepez
On 2017/03/14 16:02:49, Eric Seckler wrote: > +tsepez@ for cc/ipc > > Fady mentioned that ...
2 weeks, 1 day ago (2017-03-14 16:38:07 UTC) #25
dcheng
On 2017/03/14 16:02:49, Eric Seckler wrote: > +tsepez@ for cc/ipc > > Fady mentioned that ...
2 weeks, 1 day ago (2017-03-14 16:41:17 UTC) #26
piman
https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_macros.h File cc/ipc/cc_param_traits_macros.h (right): https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_macros.h#newcode180 cc/ipc/cc_param_traits_macros.h:180: IPC_STRUCT_TRAITS_MEMBER(latest_confirmed_sequence_number) Are there assumptions about this? E.g. I assume ...
2 weeks, 1 day ago (2017-03-14 20:06:44 UTC) #27
Eric Seckler
Brian, Antoine, PTAL :) On 2017/03/14 16:41:17, dcheng wrote: > For your compilation issue, see ...
2 weeks ago (2017-03-15 11:56:48 UTC) #31
brianderson
lgtm
2 weeks ago (2017-03-15 18:54:23 UTC) #36
dcheng
On 2017/03/15 11:56:48, Eric Seckler wrote: > Brian, Antoine, PTAL :) > > On 2017/03/14 ...
2 weeks ago (2017-03-15 20:10:47 UTC) #37
piman
Mostly good. Main thing is the question about the validity of the DCHECK assuming untrusted ...
2 weeks ago (2017-03-15 21:17:10 UTC) #38
Eric Seckler
Thanks, all addressed! Daniel, could you have a look at ipc security including third_party/WebKit/public/blink_typemaps.gni +asvitkine@ ...
1 week, 6 days ago (2017-03-16 10:24:15 UTC) #40
Eric Seckler
On 2017/03/16 10:24:15, Eric Seckler wrote: > +asvitkine@ for: > tools/metrics/histograms/histograms.xml Alexei is "very slow", ...
1 week, 6 days ago (2017-03-16 10:26:48 UTC) #42
piman
LGTM + couple of nits. https://codereview.chromium.org/2740833005/diff/100001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2740833005/diff/100001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode943 content/browser/renderer_host/render_widget_host_view_aura.cc:943: DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number); On 2017/03/16 ...
1 week, 6 days ago (2017-03-16 18:32:14 UTC) #56
Steven Holte
histograms lgtm
1 week, 6 days ago (2017-03-16 20:35:56 UTC) #57
dcheng
https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_args_struct_traits.cc File cc/ipc/begin_frame_args_struct_traits.cc (right): https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_args_struct_traits.cc#newcode22 cc/ipc/begin_frame_args_struct_traits.cc:22: out->type = static_cast<cc::BeginFrameArgs::BeginFrameArgsType>(data.type()); Mind adding a TODO to use ...
1 week, 5 days ago (2017-03-17 05:54:03 UTC) #58
Eric Seckler
On 2017/03/16 18:32:14, piman wrote: > That page explicitly says "Avoid browser side DCHECKs and ...
1 week, 5 days ago (2017-03-17 11:06:45 UTC) #61
piman
LGTM. I don't feel that strongly about ReceivedBadMessage, just that in the past we had ...
1 week, 5 days ago (2017-03-17 17:38:09 UTC) #64
dcheng
LGTM with one thought https://codereview.chromium.org/2740833005/diff/240001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2740833005/diff/240001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1839 content/browser/renderer_host/render_widget_host_impl.cc:1839: // |has_damage| and |remaining_frames| are ...
1 week, 5 days ago (2017-03-17 18:39:19 UTC) #65
piman
On Fri, Mar 17, 2017 at 11:39 AM, <dcheng@chromium.org> wrote: > LGTM with one thought ...
1 week, 5 days ago (2017-03-17 19:20:55 UTC) #66
dcheng
On 2017/03/17 19:20:55, piman wrote: > On Fri, Mar 17, 2017 at 11:39 AM, <mailto:dcheng@chromium.org> ...
1 week, 5 days ago (2017-03-17 19:24:40 UTC) #67
Eric Seckler
On 2017/03/17 19:24:40, dcheng wrote: > On 2017/03/17 19:20:55, piman wrote: > > On Fri, ...
1 week, 4 days ago (2017-03-18 06:04:06 UTC) #68
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/2740833005/240001
1 week, 4 days ago (2017-03-18 06:04:29 UTC) #71
commit-bot: I haz the power
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf108411491f38632cc
1 week, 4 days ago (2017-03-18 06:11:52 UTC) #74
Fady Samuel
1 week, 4 days ago (2017-03-18 11:04:31 UTC) #75
dcheng
On 2017/03/18 06:04:06, Eric Seckler wrote: > On 2017/03/17 19:24:40, dcheng wrote: > > On ...
1 week, 4 days ago (2017-03-18 23:14:02 UTC) #76
Eric Seckler
1 week, 3 days ago (2017-03-19 06:49:32 UTC) #77
Message was sent while issue was closed.
On 2017/03/18 23:14:02, dcheng wrote:
> Hmm, I may be missing something, but I thought piman's suggestion was to just
> not send the other fields: in other words, don't send across an entire
> BeginFrameAck, but just send across the sequence number?

We've explicitly left those fields out of the mojom and param_traits_macros so
that they are not transmitted - see comments on patch set 2.
Sign in to reply to this message.

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