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

Issue 1655433002: Remove done notifications from incoming message handlers. (Closed)

Created:
4 years, 7 months ago by Sergey Ulanov
Modified:
4 years, 7 months ago
Reviewers:
Jamie, nicholss
CC:
chromium-reviews, chromoting-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove done notifications from incoming message handlers. Previously MessageReader and ProtobufMessageParse were passing done callbacks to the messages handlers. These callbacks were necessary to pace the reader, particularly when video renderer is slow and cannot keep up with the rate of the incoming messages. It's no longer necessary because we have explicit ACK messages for video packets. Committed: https://crrev.com/e61b68d97a31c00367a0e98d9d88c74774026482 Cr-Commit-Position: refs/heads/master@{#372509}

Patch Set 1 : #

Total comments: 6

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -273 lines) Patch
M remoting/protocol/audio_reader.h View 1 chunk +3 lines, -0 lines 0 comments Download
M remoting/protocol/audio_reader.cc View 1 chunk +8 lines, -5 lines 0 comments Download
M remoting/protocol/channel_multiplexer.h View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/protocol/channel_multiplexer.cc View 7 chunks +9 lines, -21 lines 0 comments Download
M remoting/protocol/client_control_dispatcher.h View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/protocol/client_control_dispatcher.cc View 3 chunks +3 lines, -8 lines 0 comments Download
M remoting/protocol/client_video_dispatcher.h View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/protocol/client_video_dispatcher.cc View 1 chunk +4 lines, -9 lines 0 comments Download
M remoting/protocol/client_video_dispatcher_unittest.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M remoting/protocol/host_control_dispatcher.h View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/protocol/host_control_dispatcher.cc View 1 chunk +1 line, -3 lines 0 comments Download
M remoting/protocol/host_event_dispatcher.h View 1 chunk +1 line, -2 lines 0 comments Download
M remoting/protocol/host_event_dispatcher.cc View 2 chunks +1 line, -5 lines 0 comments Download
M remoting/protocol/host_video_dispatcher.h View 1 chunk +1 line, -1 line 0 comments Download
M remoting/protocol/host_video_dispatcher.cc View 1 chunk +3 lines, -8 lines 0 comments Download
M remoting/protocol/message_reader.h View 1 2 chunks +4 lines, -10 lines 0 comments Download
M remoting/protocol/message_reader.cc View 1 3 chunks +6 lines, -29 lines 0 comments Download
M remoting/protocol/message_reader_unittest.cc View 9 chunks +17 lines, -155 lines 0 comments Download
M remoting/protocol/protobuf_message_parser.h View 3 chunks +3 lines, -5 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 25 (14 generated)
Sergey Ulanov
4 years, 7 months ago (2016-01-29 20:53:47 UTC) #4
Jamie
LGTM. https://codereview.chromium.org/1655433002/diff/20001/remoting/protocol/audio_reader.cc File remoting/protocol/audio_reader.cc (right): https://codereview.chromium.org/1655433002/diff/20001/remoting/protocol/audio_reader.cc#newcode26 remoting/protocol/audio_reader.cc:26: base::Bind(&base::DoNothing)); Is it possible to remove the callback ...
4 years, 7 months ago (2016-01-29 23:48:12 UTC) #5
Sergey Ulanov
https://codereview.chromium.org/1655433002/diff/20001/remoting/protocol/audio_reader.cc File remoting/protocol/audio_reader.cc (right): https://codereview.chromium.org/1655433002/diff/20001/remoting/protocol/audio_reader.cc#newcode26 remoting/protocol/audio_reader.cc:26: base::Bind(&base::DoNothing)); On 2016/01/29 23:48:12, Jamie wrote: > Is it ...
4 years, 7 months ago (2016-01-30 00:14:47 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1655433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1655433002/40001
4 years, 7 months ago (2016-01-30 00:16:07 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/124073) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 7 months ago (2016-01-30 00:19:22 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1655433002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1655433002/60001
4 years, 7 months ago (2016-01-30 01:09:36 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1655433002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1655433002/80001
4 years, 7 months ago (2016-01-30 01:11:33 UTC) #18
commit-bot: I haz the power
Committed patchset #4 (id:80001)
4 years, 7 months ago (2016-01-30 01:52:43 UTC) #20
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/e61b68d97a31c00367a0e98d9d88c74774026482 Cr-Commit-Position: refs/heads/master@{#372509}
4 years, 7 months ago (2016-01-30 01:53:44 UTC) #22
nicholss
On 2016/01/30 01:53:44, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as ...
4 years, 7 months ago (2016-02-01 16:53:54 UTC) #23
nicholss
4 years, 7 months ago (2016-02-01 16:54:13 UTC) #25
Message was sent while issue was closed.

          

Powered by Google App Engine
This is Rietveld 408576698