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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 2 weeks ago by Eric Seckler
Modified:
6 months, 1 week 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 (OOO), 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 > ...
6 months, 2 weeks 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 > ...
6 months, 2 weeks 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: > > > ...
6 months, 2 weeks 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: > > ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 1 week 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: > ...
6 months, 1 week 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 ...
6 months, 1 week 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: > > ...
6 months, 1 week 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: > > ...
6 months, 1 week 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 ...
6 months, 1 week 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, ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week ago (2017-03-15 11:56:48 UTC) #31
brianderson
lgtm
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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@ ...
6 months, 1 week 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", ...
6 months, 1 week 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 ...
6 months, 1 week ago (2017-03-16 18:32:14 UTC) #56
Steven Holte
histograms lgtm
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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> ...
6 months, 1 week 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, ...
6 months, 1 week 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
6 months, 1 week 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
6 months, 1 week ago (2017-03-18 06:11:52 UTC) #74
Fady Samuel
6 months, 1 week 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 ...
6 months, 1 week ago (2017-03-18 23:14:02 UTC) #76
Eric Seckler
6 months, 1 week 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 b40b6558b