Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(491)

Issue 22477006: Added JsonMessage to the control channel. (Closed)

Created:
7 years, 4 months ago by Jamie
Modified:
7 years, 4 months ago
Reviewers:
Wez
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

Added JsonMessage to the control channel. This adds the client plumbing needed to get an arbitrary JSON message from client to host, or vice versa. This will allow us to extend the protocol at short notice, without needing to wait for client plugin changes to be promoted to Stable. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217127

Patch Set 1 #

Total comments: 8

Patch Set 2 : Added comment. #

Patch Set 3 : Added comment. #

Patch Set 4 : Renamed variables and methods to make the protocol encoding-agnostic. #

Patch Set 5 : Updated ChromotingJniInstance. #

Patch Set 6 : Updated protocol mocks. #

Total comments: 2

Patch Set 7 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -2 lines) Patch
M remoting/client/chromoting_client.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/client/chromoting_client.cc View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M remoting/client/client_user_interface.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M remoting/client/jni/chromoting_jni_instance.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/client/jni/chromoting_jni_instance.cc View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M remoting/client/plugin/chromoting_instance.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M remoting/client/plugin/chromoting_instance.cc View 1 2 3 4 chunks +27 lines, -1 line 0 comments Download
M remoting/host/client_session.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/host/client_session.cc View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M remoting/proto/control.proto View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
M remoting/proto/internal.proto View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M remoting/protocol/client_control_dispatcher.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M remoting/protocol/client_control_dispatcher.cc View 1 2 3 2 chunks +9 lines, -0 lines 0 comments Download
M remoting/protocol/client_stub.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M remoting/protocol/host_control_dispatcher.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/protocol/host_control_dispatcher.cc View 1 2 3 2 chunks +9 lines, -0 lines 0 comments Download
M remoting/protocol/host_stub.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M remoting/protocol/protocol_mock_objects.h View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download
M remoting/webapp/client_plugin.js View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M remoting/webapp/client_plugin_async.js View 1 2 3 2 chunks +21 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Jamie
ptal
7 years, 4 months ago (2013-08-09 21:14:21 UTC) #1
Wez
On 2013/08/09 21:14:21, Jamie wrote: > ptal Does this need to be JSON message as ...
7 years, 4 months ago (2013-08-09 21:17:58 UTC) #2
Wez
https://codereview.chromium.org/22477006/diff/1/remoting/client/plugin/chromoting_instance.cc File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/22477006/diff/1/remoting/client/plugin/chromoting_instance.cc#newcode442 remoting/client/plugin/chromoting_instance.cc:442: SendClientJson(type, json); Why not leave it up to the ...
7 years, 4 months ago (2013-08-09 21:25:48 UTC) #3
Jamie
You're correct that it doesn't need to be JSON. I originally had the "type" field ...
7 years, 4 months ago (2013-08-09 21:39:06 UTC) #4
Jamie
Changes as discussed off-line. PTAL.
7 years, 4 months ago (2013-08-10 01:30:18 UTC) #5
Wez
lgtm https://codereview.chromium.org/22477006/diff/48001/remoting/client/chromoting_client.h File remoting/client/chromoting_client.h (right): https://codereview.chromium.org/22477006/diff/48001/remoting/client/chromoting_client.h#newcode70 remoting/client/chromoting_client.h:70: virtual void DeliverHostMessage( nit: After we talked I ...
7 years, 4 months ago (2013-08-12 20:48:01 UTC) #6
Jamie
fyi https://codereview.chromium.org/22477006/diff/48001/remoting/client/chromoting_client.h File remoting/client/chromoting_client.h (right): https://codereview.chromium.org/22477006/diff/48001/remoting/client/chromoting_client.h#newcode70 remoting/client/chromoting_client.h:70: virtual void DeliverHostMessage( On 2013/08/12 20:48:01, Wez wrote: ...
7 years, 4 months ago (2013-08-12 20:58:49 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/22477006/94001
7 years, 4 months ago (2013-08-12 21:03:42 UTC) #8
commit-bot: I haz the power
7 years, 4 months ago (2013-08-13 00:13:51 UTC) #9
Message was sent while issue was closed.
Change committed as 217127

Powered by Google App Engine
This is Rietveld 408576698