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

Issue 2886873002: Only send the FrameSinkId to client when it is necessary (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by Peng
Modified:
1 month ago
Reviewers:
Tom Sepez, Fady Samuel, sky
CC:
chromium-reviews, rjkroege, mlamouri+watch-content_chromium.org, sadrul, jam, darin-cc_chromium.org, kalyank
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Only send the FrameSinkId to client when it is necessary The ui::ClientCompositorFrameSink doesn't really need the FameSinkId for submitting frames, so remove it. And for TopLevel Window, Embeddee and DisplayRoot, we don't need the FrameSinkId to create the ui::ClientCompositorFrameSink, so remove the FrameSinkId arg from related mojo IPC. This CL also passes enable_surface_synchronization flag to ui::ClientCompositorFrameSink via ctor. BUG=720600 Review-Url: https://codereview.chromium.org/2886873002 Cr-Commit-Position: refs/heads/master@{#472841} Committed: https://chromium.googlesource.com/chromium/src/+/77a8c758cc4731844a0308f23ccd2fd0e8fe14d9

Patch Set 1 #

Patch Set 2 : WIP #

Total comments: 6

Patch Set 3 : Address review issues #

Patch Set 4 : Address review issue #

Patch Set 5 : Fix unittests #

Patch Set 6 : Fix build issues. #

Patch Set 7 : Fix build issues #

Total comments: 6

Patch Set 8 : Address review issues. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+127 lines, -235 lines) Patch
M content/renderer/mus/renderer_window_tree_client.h View 1 2 3 chunks +0 lines, -3 lines 0 comments Download
M content/renderer/mus/renderer_window_tree_client.cc View 1 2 4 chunks +6 lines, -6 lines 0 comments Download
M services/ui/public/cpp/client_compositor_frame_sink.h View 1 3 chunks +5 lines, -6 lines 0 comments Download
M services/ui/public/cpp/client_compositor_frame_sink.cc View 1 4 chunks +13 lines, -18 lines 0 comments Download
M services/ui/public/interfaces/window_manager.mojom View 1 2 3 4 5 6 7 2 chunks +5 lines, -8 lines 0 comments Download
M services/ui/public/interfaces/window_tree.mojom View 1 2 3 chunks +2 lines, -6 lines 0 comments Download
M services/ui/test_wm/test_wm.cc View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M services/ui/ws/test_change_tracker.h View 1 2 3 4 2 chunks +2 lines, -4 lines 0 comments Download
M services/ui/ws/test_change_tracker.cc View 1 2 3 4 4 chunks +9 lines, -16 lines 0 comments Download
M services/ui/ws/test_utils.h View 1 2 3 4 3 chunks +0 lines, -3 lines 0 comments Download
M services/ui/ws/test_utils.cc View 1 2 3 4 3 chunks +2 lines, -5 lines 0 comments Download
M services/ui/ws/window_manager_client_unittest.cc View 1 2 3 4 5 6 7 13 chunks +31 lines, -16 lines 0 comments Download
M services/ui/ws/window_tree.cc View 1 2 3 4 chunks +5 lines, -6 lines 0 comments Download
M services/ui/ws/window_tree_client_unittest.cc View 1 2 3 4 5 chunks +4 lines, -12 lines 0 comments Download
M services/ui/ws/window_tree_unittest.cc View 1 2 3 4 1 chunk +4 lines, -6 lines 0 comments Download
M ui/aura/mus/mus_context_factory.h View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M ui/aura/mus/mus_context_factory.cc View 1 2 1 chunk +4 lines, -12 lines 0 comments Download
M ui/aura/mus/window_port_mus.h View 2 chunks +2 lines, -11 lines 0 comments Download
M ui/aura/mus/window_port_mus.cc View 1 2 3 2 chunks +9 lines, -35 lines 0 comments Download
M ui/aura/mus/window_tree_client.h View 1 2 3 5 chunks +1 line, -9 lines 0 comments Download
M ui/aura/mus/window_tree_client.cc View 1 2 3 12 chunks +10 lines, -28 lines 0 comments Download
M ui/aura/mus/window_tree_client_unittest.cc View 1 2 3 4 5 6 chunks +6 lines, -11 lines 0 comments Download
M ui/aura/mus/window_tree_host_mus.cc View 1 2 2 chunks +1 line, -6 lines 0 comments Download
M ui/aura/test/mus/window_tree_client_private.cc View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 44 (30 generated)
Peng
Hi Fady, PTAL. Thanks.
1 month, 1 week ago (2017-05-16 17:55:54 UTC) #2
Fady Samuel
https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/client_compositor_frame_sink.cc File services/ui/public/cpp/client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/client_compositor_frame_sink.cc#newcode87 services/ui/public/cpp/client_compositor_frame_sink.cc:87: if (!enable_surface_synchronization_) Why did you make this change? https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port_mus.cc ...
1 month, 1 week ago (2017-05-16 18:13:47 UTC) #5
Peng
https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/client_compositor_frame_sink.cc File services/ui/public/cpp/client_compositor_frame_sink.cc (right): https://codereview.chromium.org/2886873002/diff/20001/services/ui/public/cpp/client_compositor_frame_sink.cc#newcode87 services/ui/public/cpp/client_compositor_frame_sink.cc:87: if (!enable_surface_synchronization_) On 2017/05/16 18:13:47, Fady Samuel wrote: > ...
1 month, 1 week ago (2017-05-16 18:44:55 UTC) #6
Fady Samuel
https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port_mus.cc File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port_mus.cc#newcode271 ui/aura/mus/window_port_mus.cc:271: frame_sink_id_ = frame_sink_id; On 2017/05/16 18:44:55, Peng wrote: > ...
1 month, 1 week ago (2017-05-16 20:20:24 UTC) #7
Peng
https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port_mus.cc File ui/aura/mus/window_port_mus.cc (right): https://codereview.chromium.org/2886873002/diff/20001/ui/aura/mus/window_port_mus.cc#newcode271 ui/aura/mus/window_port_mus.cc:271: frame_sink_id_ = frame_sink_id; On 2017/05/16 20:20:23, Fady Samuel wrote: ...
1 month, 1 week ago (2017-05-16 21:26:37 UTC) #9
Fady Samuel
Thanks for cleaning this up. LGTM
1 month ago (2017-05-17 17:44:21 UTC) #20
Peng
Hi Scott, PTAL. Thanks.
1 month ago (2017-05-17 20:23:24 UTC) #25
sky
https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/interfaces/window_manager.mojom File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/interfaces/window_manager.mojom#newcode275 services/ui/public/interfaces/window_manager.mojom:275: // Returns true if all accelerators were added successfully. ...
1 month ago (2017-05-18 03:18:09 UTC) #29
Peng
https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/interfaces/window_manager.mojom File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2886873002/diff/120001/services/ui/public/interfaces/window_manager.mojom#newcode275 services/ui/public/interfaces/window_manager.mojom:275: // Returns true if all accelerators were added successfully. ...
1 month ago (2017-05-18 13:34:56 UTC) #30
Peng
+tsepez@chromium.org for *.mojom Hi Tom, PTAL. Thanks.
1 month ago (2017-05-18 13:39:02 UTC) #32
sky
LGTM
1 month ago (2017-05-18 15:54:23 UTC) #37
Tom Sepez
lgtm RS on deleting an argument.
1 month ago (2017-05-18 15:56:10 UTC) #38
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/2886873002/140001
1 month ago (2017-05-18 16:00:51 UTC) #41
commit-bot: I haz the power
1 month ago (2017-05-18 16:40:17 UTC) #44
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/77a8c758cc4731844a0308f23ccd...
Sign in to reply to this message.

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