Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2586133002:
[Chromoting] Implement HostExperimentSessionPlugin (Closed)
Description[Chromoting] Implement HostExperimentSessionPlugin
HostExperimentSessionPlugin is a host side SessionPlugin to send host attributes
to, and receive configuration from the client side.
A host can attach this plugin to the Session and use its configuration to
initialize a HostSessionOptions.
This is part of host experiment framework.
BUG=650926
Review-Url: https://codereview.chromium.org/2586133002
Cr-Commit-Position: refs/heads/master@{#441850}
Committed: https://chromium.googlesource.com/chromium/src/+/8e659c584c519c09430f536788b6ac45400eeccc
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update implementation to match SessionPlugin #
Total comments: 6
Patch Set 3 : Resolve review comments #
Messages
Total messages: 46 (40 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== HostExperimentSessionPlugin BUG= ========== to ========== [Chromoting] Implement HostExperimentSessionPlugin This is part of host experiment framework. BUG=650926 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/DEPS File remoting/protocol/DEPS (right): https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/DEPS#... remoting/protocol/DEPS:9: "+remoting/host", Protocol code should not depend on host, this is by design. https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/host_... File remoting/protocol/host_experiment_session_plugin.cc (right): https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/host_... remoting/protocol/host_experiment_session_plugin.cc:8: #include "remoting/host/host_attributes.h" remoting/protocol should not depend on remoting/host Maybe move this file to remoting/host? Alternatively you can pass Attributes set in the constructor https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/host_... remoting/protocol/host_experiment_session_plugin.cc:35: if (!attachments) { Why would this function be called with empty attachments?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Chromoting] Implement HostExperimentSessionPlugin This is part of host experiment framework. BUG=650926 ========== to ========== [Chromoting] Implement HostExperimentSessionPlugin HostExperimentSessionPlugin is a host side SessionPlugin to send host attributes to, and receive configuration from the client side. A host can attach this plugin to the Session and use its configuration to initialize a HostSessionOptions. This is part of host experiment framework. BUG=650926 ==========
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org
https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/DEPS File remoting/protocol/DEPS (right): https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/DEPS#... remoting/protocol/DEPS:9: "+remoting/host", On 2016/12/23 00:23:19, Sergey Ulanov wrote: > Protocol code should not depend on host, this is by design. Done. https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/host_... File remoting/protocol/host_experiment_session_plugin.cc (right): https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/host_... remoting/protocol/host_experiment_session_plugin.cc:8: #include "remoting/host/host_attributes.h" On 2016/12/23 00:23:20, Sergey Ulanov wrote: > remoting/protocol should not depend on remoting/host > Maybe move this file to remoting/host? > Alternatively you can pass Attributes set in the constructor This file pair has been moved to remoting/host. https://codereview.chromium.org/2586133002/diff/40001/remoting/protocol/host_... remoting/protocol/host_experiment_session_plugin.cc:35: if (!attachments) { On 2016/12/23 00:23:19, Sergey Ulanov wrote: > Why would this function be called with empty attachments? Done.
lgtm after my nits are addressed https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... File remoting/host/host_experiment_session_plugin.cc (right): https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... remoting/host/host_experiment_session_plugin.cc:28: if (!configuration_.empty()) { I suggest adding a separate flag to indicate that the configuration has been received. The value we get in <host-configuration> may be empty https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... File remoting/host/host_experiment_session_plugin.h (right): https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... remoting/host/host_experiment_session_plugin.h:17: // receives experiment settings. This plugin is for host only. Don't need the last sentence. It's implied by location of this file and also class name. https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... remoting/host/host_experiment_session_plugin.h:22: std::unique_ptr<buzz::XmlElement> GetNextMessage() override; // protocol::SessionPlugin implementation.
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... File remoting/host/host_experiment_session_plugin.cc (right): https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... remoting/host/host_experiment_session_plugin.cc:28: if (!configuration_.empty()) { On 2017/01/05 20:45:06, Sergey Ulanov wrote: > I suggest adding a separate flag to indicate that the configuration has been > received. The value we get in <host-configuration> may be empty Done. https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... File remoting/host/host_experiment_session_plugin.h (right): https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... remoting/host/host_experiment_session_plugin.h:17: // receives experiment settings. This plugin is for host only. On 2017/01/05 20:45:06, Sergey Ulanov wrote: > Don't need the last sentence. It's implied by location of this file and also > class name. Done. https://codereview.chromium.org/2586133002/diff/80001/remoting/host/host_expe... remoting/host/host_experiment_session_plugin.h:22: std::unique_ptr<buzz::XmlElement> GetNextMessage() override; On 2017/01/05 20:45:06, Sergey Ulanov wrote: > // protocol::SessionPlugin implementation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2586133002/#ps120001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483666950350960,
"parent_rev": "5d47305072d3bf73cc0ad4998f0c9970f2b0b8c8", "commit_rev":
"8e659c584c519c09430f536788b6ac45400eeccc"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Implement HostExperimentSessionPlugin HostExperimentSessionPlugin is a host side SessionPlugin to send host attributes to, and receive configuration from the client side. A host can attach this plugin to the Session and use its configuration to initialize a HostSessionOptions. This is part of host experiment framework. BUG=650926 ========== to ========== [Chromoting] Implement HostExperimentSessionPlugin HostExperimentSessionPlugin is a host side SessionPlugin to send host attributes to, and receive configuration from the client side. A host can attach this plugin to the Session and use its configuration to initialize a HostSessionOptions. This is part of host experiment framework. BUG=650926 Review-Url: https://codereview.chromium.org/2586133002 Cr-Commit-Position: refs/heads/master@{#441850} Committed: https://chromium.googlesource.com/chromium/src/+/8e659c584c519c09430f536788b6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8e659c584c519c09430f536788b6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
