Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2933203002:
[Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) (Closed)
Description[Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process)
Three new commands have been added to chromoting_messages to
- Ask DesktopSessionAgent (desktop process) or Daemon to start reporting
process resource usage through IPC channel back to network process.
- Ask Desktop process or Daemon to stop reporting.
- Report process usage to network process.
So network process can decide whether it needs this information or not at
runtime.
BUG=650926
Review-Url: https://codereview.chromium.org/2933203002
Cr-Commit-Position: refs/heads/master@{#480652}
Committed: https://chromium.googlesource.com/chromium/src/+/36763d8f9b73a039095f0dd5db88ee2e44f17d41
Patch Set 1 #
Total comments: 14
Patch Set 2 : Resolve review comments #Patch Set 3 : Resolve review comments #
Total comments: 14
Patch Set 4 : Resolve review comments #
Total comments: 4
Patch Set 5 : Resolve review comments #
Total comments: 4
Patch Set 6 : Resolve review comments #
Total comments: 4
Patch Set 7 : Resolve review comments #
Messages
Total messages: 64 (43 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 ========== [Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) A new command has been added to chromoting_messages to ask DesktopSessionAgent (desktop process) to start reporting process resource usage through IPC channel back to network process. So network process can decide whether it needs this information or not at runtime. I have not seen a very clear requirement of a "StopProcessStatsReport" command, which is not added into this change. Let me know if you can imagine a scenario. BUG=650926 ========== to ========== [Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) A new command has been added to chromoting_messages to ask DesktopSessionAgent (desktop process) to start reporting process resource usage through IPC channel back to network process. So network process can decide whether it needs this information or not at runtime. I have not seen a very clear requirement of a "StopProcessStatsReport" command, which is not added into this change. Let me know if you can imagine a scenario. BUG=650926 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:270: // Starts to report process resource usage. Do you need a message to stop sending process stats? The lifetime of the P2P channel is not necessarily tied to the duration of the session so if a client kills the P2P channel, do you intend to keep sending stats to the network process? https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:271: IPC_MESSAGE_CONTROL1(ChromotingNetworkMsg_StartProcessStatsReport, Other messages use a ProductNameSendNameReceiverNameMsg_MessageType format, are you intending to pass the same message to all processes? If so, it would be good to update the name to show that: e.g. ChromotingNetworkAnyMsg_StartProcessStatsReport https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) Can the interval be compiled into the sender (so they all have the same interval) or is there a benefit to passing it in the IPC message? https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent.cc:579: I think it is worth a DCHECK here if you do not intend to receive this message multiple times (you would need to handle 'StopSending' messages as well but I think that is a good idea anyway). Since this is in an IPC handler, you will still want to protect against the message being sent twice in release builds so the if(!stats_sender_) check is ok to leave in with the DCHECK. https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent.h:153: // used. Only the first execution will take effect. Unless there is a good reason to pass a variable length value at runtime, I think this could be simpler to just compile a shared value in. https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent_unittest.cc (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent_unittest.cc:86: TEST(DesktopSessionAgentTest, StartProcessStatsReport) { I think this test is doing too much. Can you move the non-test related logic into a setup method. Whitespace and comments would make this a lot more readable as well.
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: This issue passed the CQ dry run.
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 ========== [Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) A new command has been added to chromoting_messages to ask DesktopSessionAgent (desktop process) to start reporting process resource usage through IPC channel back to network process. So network process can decide whether it needs this information or not at runtime. I have not seen a very clear requirement of a "StopProcessStatsReport" command, which is not added into this change. Let me know if you can imagine a scenario. BUG=650926 ========== to ========== [Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) Three new commands have been added to chromoting_messages to - Ask DesktopSessionAgent (desktop process) or Daemon to start reporting process resource usage through IPC channel back to network process. - Ask Desktop process or Daemon to stop reporting. - Report process usage to network process. So network process can decide whether it needs this information or not at runtime. BUG=650926 ==========
https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:270: // Starts to report process resource usage. On 2017/06/13 20:12:11, joedow wrote: > Do you need a message to stop sending process stats? The lifetime of the P2P > channel is not necessarily tied to the duration of the session so if a client > kills the P2P channel, do you intend to keep sending stats to the network > process? Done. https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:271: IPC_MESSAGE_CONTROL1(ChromotingNetworkMsg_StartProcessStatsReport, On 2017/06/13 20:12:12, joedow wrote: > Other messages use a ProductNameSendNameReceiverNameMsg_MessageType format, are > you intending to pass the same message to all processes? If so, it would be > good to update the name to show that: e.g. > ChromotingNetworkAnyMsg_StartProcessStatsReport I have not seen an "Any" in this file. Yes, this message is expected to be sent from network process to both daemon and desktop processes. Updated. https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) On 2017/06/13 20:12:12, joedow wrote: > Can the interval be compiled into the sender (so they all have the same > interval) or is there a benefit to passing it in the IPC message? It benefits the tests. But it's also reasonable to control it by network process just in case we have a shorter interval for experiments, but a longer interval to detect any unexpected error. https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent.cc:579: On 2017/06/13 20:12:12, joedow wrote: > I think it is worth a DCHECK here if you do not intend to receive this message > multiple times (you would need to handle 'StopSending' messages as well but I > think that is a good idea anyway). > > Since this is in an IPC handler, you will still want to protect against the > message being sent twice in release builds so the if(!stats_sender_) check is ok > to leave in with the DCHECK. Done. https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent.h:153: // used. Only the first execution will take effect. On 2017/06/13 20:12:12, joedow wrote: > Unless there is a good reason to pass a variable length value at runtime, I > think this could be simpler to just compile a shared value in. Explained in chromoting_messages.h. https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... File remoting/host/desktop_session_agent_unittest.cc (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/desktop_sessi... remoting/host/desktop_session_agent_unittest.cc:86: TEST(DesktopSessionAgentTest, StartProcessStatsReport) { On 2017/06/13 20:12:12, joedow wrote: > I think this test is doing too much. Can you move the non-test related logic > into a setup method. > > Whitespace and comments would make this a lot more readable as well. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) On 2017/06/14 23:12:23, Hzj_jie wrote: > On 2017/06/13 20:12:12, joedow wrote: > > Can the interval be compiled into the sender (so they all have the same > > interval) or is there a benefit to passing it in the IPC message? > > It benefits the tests. But it's also reasonable to control it by network process > just in case we have a shorter interval for experiments, but a longer interval > to detect any unexpected error. I'm not sure that we will need to change it but I don't feel too passionate about it. If you do decide this is mostly for testing, just add a SetIntervalForTest() to the classes which need it an then clean this up. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent.cc:584: interval_ms = 2000; Why 2000? Why not 1 second? A comment on why this is the default value would be good. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent.cc:584: interval_ms = 2000; Also, if this is the default value to be used for all process (not just DesktopSessionAgent) then you probably want it in a central location. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent.h:156: // Stops to report process statistic data to network process. s/Stops to report/Stops sending https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... File remoting/host/desktop_session_agent_unittest.cc (right): https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:29: class MockDelegate : public DesktopSessionAgent::Delegate { Sorry I missed this in the first iteration, but this isn't a Mock, FakeDelegate or TestDelegate is a more accurate name. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:110: ASSERT_FALSE(static_cast<bool>(task_runner_)); I might be missing something, but you assign nullptr to task_runner_ and agent_ above, how would these two ASSERTS ever fire? https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:111: ASSERT_FALSE(static_cast<bool>(agent_)); If you do need to keep the ASSERTS, I think ASSERT_EQ(agent_, nullptr) is more readable than trying to boolify the ptrs. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:139: run_loop_.Run(); Can you add a test where you send a stop MSG?
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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Unfortunately the build has been broken by some others' change. But this CL has been updated for reviewing. https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/2933203002/diff/1/remoting/host/chromoting_me... remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) On 2017/06/15 22:50:44, joedow wrote: > On 2017/06/14 23:12:23, Hzj_jie wrote: > > On 2017/06/13 20:12:12, joedow wrote: > > > Can the interval be compiled into the sender (so they all have the same > > > interval) or is there a benefit to passing it in the IPC message? > > > > It benefits the tests. But it's also reasonable to control it by network > process > > just in case we have a shorter interval for experiments, but a longer interval > > to detect any unexpected error. > > I'm not sure that we will need to change it but I don't feel too passionate > about it. If you do decide this is mostly for testing, just add a > SetIntervalForTest() to the classes which need it an then clean this up. If you do not have a strong opinion, I would keep it as-is. It benefits both tests and future user-cases. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent.cc:584: interval_ms = 2000; On 2017/06/15 22:50:44, joedow wrote: > Why 2000? Why not 1 second? > A comment on why this is the default value would be good. > Done. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent.cc:584: interval_ms = 2000; On 2017/06/15 22:50:44, joedow wrote: > Also, if this is the default value to be used for all process (not just > DesktopSessionAgent) then you probably want it in a central location. Done. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent.h:156: // Stops to report process statistic data to network process. On 2017/06/15 22:50:44, joedow wrote: > s/Stops to report/Stops sending Done. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... File remoting/host/desktop_session_agent_unittest.cc (right): https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:29: class MockDelegate : public DesktopSessionAgent::Delegate { On 2017/06/15 22:50:44, joedow wrote: > Sorry I missed this in the first iteration, but this isn't a Mock, FakeDelegate > or TestDelegate is a more accurate name. It looks like I still cannot choose the right name between Mock and Fake. :( https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:110: ASSERT_FALSE(static_cast<bool>(task_runner_)); On 2017/06/15 22:50:44, joedow wrote: > I might be missing something, but you assign nullptr to task_runner_ and agent_ > above, how would these two ASSERTS ever fire? No, it's my fault. They should be removed. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:111: ASSERT_FALSE(static_cast<bool>(agent_)); On 2017/06/15 22:50:44, joedow wrote: > If you do need to keep the ASSERTS, I think ASSERT_EQ(agent_, nullptr) is more > readable than trying to boolify the ptrs. Done. https://codereview.chromium.org/2933203002/diff/40001/remoting/host/desktop_s... remoting/host/desktop_session_agent_unittest.cc:139: run_loop_.Run(); On 2017/06/15 22:50:44, joedow wrote: > Can you add a test where you send a stop MSG? Done.
lgtm https://codereview.chromium.org/2933203002/diff/60001/remoting/base/constants.h File remoting/base/constants.h (right): https://codereview.chromium.org/2933203002/diff/60001/remoting/base/constants... remoting/base/constants.h:26: // This value is consistent with the update interval of Windows task manager. You might remove the second line, it is pertinent to Windows but if we enable process stats for another platform (mac/linux) then the Windows tie-in isn't as useful. https://codereview.chromium.org/2933203002/diff/60001/remoting/host/desktop_s... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/2933203002/diff/60001/remoting/host/desktop_s... remoting/host/desktop_session_agent.h:152: // |interval_ms| is less than or equal to 0, a default value of 2000 will be Can you remove 2000 from here? Since the value is defined in a header, it could change at any time and this comment may not be updated. I think it is safe to say that a default value, non-zero will be used.
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...
https://codereview.chromium.org/2933203002/diff/60001/remoting/base/constants.h File remoting/base/constants.h (right): https://codereview.chromium.org/2933203002/diff/60001/remoting/base/constants... remoting/base/constants.h:26: // This value is consistent with the update interval of Windows task manager. On 2017/06/16 22:23:16, joedow wrote: > You might remove the second line, it is pertinent to Windows but if we enable > process stats for another platform (mac/linux) then the Windows tie-in isn't as > useful. It answers your question in the last iteration: why it's 2000, not 1000. I have not objective to remove it as long as it's not an unreasonable value. https://codereview.chromium.org/2933203002/diff/60001/remoting/host/desktop_s... File remoting/host/desktop_session_agent.h (right): https://codereview.chromium.org/2933203002/diff/60001/remoting/host/desktop_s... remoting/host/desktop_session_agent.h:152: // |interval_ms| is less than or equal to 0, a default value of 2000 will be On 2017/06/16 22:23:16, joedow wrote: > Can you remove 2000 from here? Since the value is defined in a header, it could > change at any time and this comment may not be updated. I think it is safe to > say that a default value, non-zero will be used. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2933203002/#ps80001 (title: "Resolve review comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zijiehe@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zijiehe@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zijiehe@chromium.org changed reviewers: + dcheng@chromium.org
Daniel, would you please have a look at this change? I added a new IPC message to ask child processes to send resource usages.
Also, it doesn't seem like any non-test code is sending this yet. Is that in a followup CL? https://codereview.chromium.org/2933203002/diff/80001/remoting/base/constants.h File remoting/base/constants.h (right): https://codereview.chromium.org/2933203002/diff/80001/remoting/base/constants... remoting/base/constants.h:26: const int kDefaultProcessStatsIntervalMs = 2000; Can we take advantage of TimeDelta's constexpr constructor and just use TimeDelta directly here? https://codereview.chromium.org/2933203002/diff/80001/remoting/host/chromotin... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/2933203002/diff/80001/remoting/host/chromotin... remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) And just pass a TimeDelta here as well.
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...
https://codereview.chromium.org/2933203002/diff/80001/remoting/base/constants.h File remoting/base/constants.h (right): https://codereview.chromium.org/2933203002/diff/80001/remoting/base/constants... remoting/base/constants.h:26: const int kDefaultProcessStatsIntervalMs = 2000; On 2017/06/19 06:22:58, dcheng wrote: > Can we take advantage of TimeDelta's constexpr constructor and just use > TimeDelta directly here? Done. https://codereview.chromium.org/2933203002/diff/80001/remoting/host/chromotin... File remoting/host/chromoting_messages.h (right): https://codereview.chromium.org/2933203002/diff/80001/remoting/host/chromotin... remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) On 2017/06/19 06:22:58, dcheng wrote: > And just pass a TimeDelta here as well. I haven't realized we have the convenience to pass a base::TimeDelta through IPC directly. Updated.
On 2017/06/19 18:46:43, Hzj_jie wrote: > https://codereview.chromium.org/2933203002/diff/80001/remoting/base/constants.h > File remoting/base/constants.h (right): > > https://codereview.chromium.org/2933203002/diff/80001/remoting/base/constants... > remoting/base/constants.h:26: const int kDefaultProcessStatsIntervalMs = 2000; > On 2017/06/19 06:22:58, dcheng wrote: > > Can we take advantage of TimeDelta's constexpr constructor and just use > > TimeDelta directly here? > > Done. > > https://codereview.chromium.org/2933203002/diff/80001/remoting/host/chromotin... > File remoting/host/chromoting_messages.h (right): > > https://codereview.chromium.org/2933203002/diff/80001/remoting/host/chromotin... > remoting/host/chromoting_messages.h:272: int32_t /* interval_ms */) > On 2017/06/19 06:22:58, dcheng wrote: > > And just pass a TimeDelta here as well. > > I haven't realized we have the convenience to pass a base::TimeDelta through IPC > directly. Updated. Required changes are too many for one single code review. So yes, the new IPC message will be used in a coming change. I will CC the followup changes to you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ipc lgtm https://codereview.chromium.org/2933203002/diff/100001/remoting/base/constant... File remoting/base/constants.cc (right): https://codereview.chromium.org/2933203002/diff/100001/remoting/base/constant... remoting/base/constants.cc:19: const base::TimeDelta kDefaultProcessStatsInterval = Nit: mark this as constexpr so we guarantee no static initializer. (Yes, you can mark something const in the header and constexpr in the .cc... fun) https://codereview.chromium.org/2933203002/diff/100001/remoting/host/desktop_... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2933203002/diff/100001/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:234: DCHECK(!stats_sender_); Btw, do we consider the network process less privileged than the other chromoting processes? (Basically, do we trust the people sending us IPCs to correctly manage this state?)
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...
https://codereview.chromium.org/2933203002/diff/100001/remoting/base/constant... File remoting/base/constants.cc (right): https://codereview.chromium.org/2933203002/diff/100001/remoting/base/constant... remoting/base/constants.cc:19: const base::TimeDelta kDefaultProcessStatsInterval = On 2017/06/19 20:11:26, dcheng wrote: > Nit: mark this as constexpr so we guarantee no static initializer. > > (Yes, you can mark something const in the header and constexpr in the .cc... > fun) Since we are using constexpr, I think we can entirely remove the change to this file. https://codereview.chromium.org/2933203002/diff/100001/remoting/host/desktop_... File remoting/host/desktop_session_agent.cc (right): https://codereview.chromium.org/2933203002/diff/100001/remoting/host/desktop_... remoting/host/desktop_session_agent.cc:234: DCHECK(!stats_sender_); On 2017/06/19 20:11:26, dcheng wrote: > Btw, do we consider the network process less privileged than the other > chromoting processes? > > (Basically, do we trust the people sending us IPCs to correctly manage this > state?) If I understand your question correctly. The |stats_sender_| will always be reset in Stop() function. So this DCHECK() ensures Stop() is called before destructor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2933203002/#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": 1497915955091090,
"parent_rev": "3455ca4cb81a7f51f7d89a92aef918bd22f15be7", "commit_rev":
"36763d8f9b73a039095f0dd5db88ee2e44f17d41"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) Three new commands have been added to chromoting_messages to - Ask DesktopSessionAgent (desktop process) or Daemon to start reporting process resource usage through IPC channel back to network process. - Ask Desktop process or Daemon to stop reporting. - Report process usage to network process. So network process can decide whether it needs this information or not at runtime. BUG=650926 ========== to ========== [Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process) Three new commands have been added to chromoting_messages to - Ask DesktopSessionAgent (desktop process) or Daemon to start reporting process resource usage through IPC channel back to network process. - Ask Desktop process or Daemon to stop reporting. - Report process usage to network process. So network process can decide whether it needs this information or not at runtime. BUG=650926 Review-Url: https://codereview.chromium.org/2933203002 Cr-Commit-Position: refs/heads/master@{#480652} Committed: https://chromium.googlesource.com/chromium/src/+/36763d8f9b73a039095f0dd5db88... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/36763d8f9b73a039095f0dd5db88... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
