|
|
Description[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)
This is a copy & paste from DesktopSessionAgent
(https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon
process to report process statistics data to network process.
BUG=650926
Review-Url: https://codereview.chromium.org/2950993003
Cr-Commit-Position: refs/heads/master@{#483539}
Committed: https://chromium.googlesource.com/chromium/src/+/dda3acb809c43e2d633c335778ff4a1522927e59
Patch Set 1 #
Total comments: 4
Patch Set 2 : Stop process stats when network process crashes #
Total comments: 16
Patch Set 3 : Resolve review comments #Patch Set 4 : Resolve review comments #
Messages
Total messages: 48 (34 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 DaemonProcess (daemon process) This is a copy & paste from DesktopSessionAgent (https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon process to report process statistics data to network process. BUG=650926 ========== to ========== [Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process) This is a copy & paste from DesktopSessionAgent (https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon process to report process statistics data to network process. BUG=650926 ==========
zijiehe@chromium.org changed reviewers: + joedow@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/2950993003/diff/1/remoting/host/daemon_process.h File remoting/host/daemon_process.h (right): https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces... remoting/host/daemon_process.h:207: // Daemon and Network processes manages multiple desktop sessions. Some of Process stats data is tied to a specific ClientSession (which lives in the network process), it be initiated from the desktop session right? My mental model was that the Network process (specifically the ClientSession instance) would send the initial start/stop IPC messages to the other processes. Individual desktop sessions would start/stop on their own as they receive IPC messages and are created / destroyed. We shouldn't need ref-counting in the Daemon process to track them.
https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h File remoting/host/daemon_process.h (right): https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces... remoting/host/daemon_process.h:207: // Daemon and Network processes manages multiple desktop sessions. Some of On 2017/06/21 15:30:50, joedow wrote: > Process stats data is tied to a specific ClientSession (which lives in the > network process), it be initiated from the desktop session right? > > My mental model was that the Network process (specifically the ClientSession > instance) would send the initial start/stop IPC messages to the other processes. > > Individual desktop sessions would start/stop on their own as they receive IPC > messages and are created / destroyed. We shouldn't need ref-counting in the > Daemon process to track them. > Oops., the top line should say: Process stats data is tied to a specific ClientSession (which lives in the network process), it shouldn't be initiated from the desktop session right?
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #2 (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 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 #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h File remoting/host/daemon_process.h (right): https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces... remoting/host/daemon_process.h:207: // Daemon and Network processes manages multiple desktop sessions. Some of On 2017/06/21 15:32:04, joedow wrote: > On 2017/06/21 15:30:50, joedow wrote: > > Process stats data is tied to a specific ClientSession (which lives in the > > network process), it be initiated from the desktop session right? > > > > My mental model was that the Network process (specifically the ClientSession > > instance) would send the initial start/stop IPC messages to the other > processes. > > > > Individual desktop sessions would start/stop on their own as they receive IPC > > messages and are created / destroyed. We shouldn't need ref-counting in the > > Daemon process to track them. > > > > Oops., the top line should say: > Process stats data is tied to a specific ClientSession (which lives in the > network process), it shouldn't be initiated from the desktop session right? After some more detailed investigations, I believe current solution is still reasonable. The ref-counter is just a two-line change, but it matches existing design of the host instance in network process. If it was a 20-line change, it would be questionable whether we should support this non-supported feature. But a two-line change seems like a no-brainer to me: why should not we avoid the confusion for free? From the analysis perspective, daemon process is far more important than the RDP process. Daemon process handles all functions RDP process handles, plus configuration update, host status monitoring, connect / disconnect client sessions, etc. More important is that daemon process is running on every machine. But RDP process is running when curtain mode is on, which, I doubt, is for Googlers only. I have no concern to also let RDP process report its resource usage though.
https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h File remoting/host/daemon_process.h (right): https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces... remoting/host/daemon_process.h:207: // Daemon and Network processes manages multiple desktop sessions. Some of On 2017/06/21 21:42:56, Hzj_jie wrote: > On 2017/06/21 15:32:04, joedow wrote: > > On 2017/06/21 15:30:50, joedow wrote: > > > Process stats data is tied to a specific ClientSession (which lives in the > > > network process), it be initiated from the desktop session right? > > > > > > My mental model was that the Network process (specifically the ClientSession > > > instance) would send the initial start/stop IPC messages to the other > > processes. > > > > > > Individual desktop sessions would start/stop on their own as they receive > IPC > > > messages and are created / destroyed. We shouldn't need ref-counting in the > > > Daemon process to track them. > > > > > > > Oops., the top line should say: > > Process stats data is tied to a specific ClientSession (which lives in the > > network process), it shouldn't be initiated from the desktop session right? > > After some more detailed investigations, I believe current solution is still > reasonable. > The ref-counter is just a two-line change, but it matches existing design of the > host instance in network process. If it was a 20-line change, it would be > questionable whether we should support this non-supported feature. But a > two-line change seems like a no-brainer to me: why should not we avoid the > confusion for free? > From the analysis perspective, daemon process is far more important than the RDP > process. Daemon process handles all functions RDP process handles, plus > configuration update, host status monitoring, connect / disconnect client > sessions, etc. More important is that daemon process is running on every > machine. But RDP process is running when curtain mode is on, which, I doubt, is > for Googlers only. > I have no concern to also let RDP process report its resource usage though. My concerns are: 1.) The ref count code isn't going to be exercised since we don't support multiple connections 2.) LOC doesn't really matter, if code is never run then it doesn't provide value (whether it is 2 lines or 20) 3.) I don't think the Daemon process or RDP session is interesting enough to monitor. The question I posed is whether anyone can think of a scenario that we require the data, so far I have not been able to. My proposal is to only report Network and Desktop process info. - This is future proof with respect to concurrent connections - It is resilient in that the cleanup is guaranteed - desktop session is closed when session ends - Client session owns the Network process sender which is destroyed when the session is closed - It provide data for the processes we are most likely to want data on - You can rename the IPC messages to be more specific (remove the 'any' sender/recipient term and replace with the actual process name. This makes the IPC message code more readable If we are able to conceive a scenario where having the Daemon or RDP process data is useful then adding more code is fine. I can't think of anything that isn't a stretch.
On 2017/06/21 22:00:20, joedow wrote: > https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_process.h > File remoting/host/daemon_process.h (right): > > https://codereview.chromium.org/2950993003/diff/1/remoting/host/daemon_proces... > remoting/host/daemon_process.h:207: // Daemon and Network processes manages > multiple desktop sessions. Some of > On 2017/06/21 21:42:56, Hzj_jie wrote: > > On 2017/06/21 15:32:04, joedow wrote: > > > On 2017/06/21 15:30:50, joedow wrote: > > > > Process stats data is tied to a specific ClientSession (which lives in the > > > > network process), it be initiated from the desktop session right? > > > > > > > > My mental model was that the Network process (specifically the > ClientSession > > > > instance) would send the initial start/stop IPC messages to the other > > > processes. > > > > > > > > Individual desktop sessions would start/stop on their own as they receive > > IPC > > > > messages and are created / destroyed. We shouldn't need ref-counting in > the > > > > Daemon process to track them. > > > > > > > > > > Oops., the top line should say: > > > Process stats data is tied to a specific ClientSession (which lives in the > > > network process), it shouldn't be initiated from the desktop session right? > > > > After some more detailed investigations, I believe current solution is still > > reasonable. > > The ref-counter is just a two-line change, but it matches existing design of > the > > host instance in network process. If it was a 20-line change, it would be > > questionable whether we should support this non-supported feature. But a > > two-line change seems like a no-brainer to me: why should not we avoid the > > confusion for free? > > From the analysis perspective, daemon process is far more important than the > RDP > > process. Daemon process handles all functions RDP process handles, plus > > configuration update, host status monitoring, connect / disconnect client > > sessions, etc. More important is that daemon process is running on every > > machine. But RDP process is running when curtain mode is on, which, I doubt, > is > > for Googlers only. > > I have no concern to also let RDP process report its resource usage though. > > My concerns are: > 1.) The ref count code isn't going to be exercised since we don't support > multiple connections > 2.) LOC doesn't really matter, if code is never run then it doesn't provide > value (whether it is 2 lines or 20) I think my main point is "matching existing design". The StartProcessStatsReport requests come from IpcDesktopEnvironment. And we do support multiple IpcDesktopEnvironment instances in ChromotingHost. Simply ignoring this existing design would make it more confusing. > 3.) I don't think the Daemon process or RDP session is interesting enough to > monitor. The question I posed is whether anyone can think of a scenario that we > require the data, so far I have not been able to. The other point is *daemon process is far more important than the RDP process*. I do not think that "we do not have it for a long time" equals to "we do not need it at all". And I also do not think we need to actively monitor the resource usage of daemon process individually, but include it in to the total resource usage is still reasonable: it's always running on end users' machines and it has more functionalities. > > My proposal is to only report Network and Desktop process info. > - This is future proof with respect to concurrent connections > - It is resilient in that the cleanup is guaranteed > - desktop session is closed when session ends > - Client session owns the Network process sender which is destroyed when the > session is closed > - It provide data for the processes we are most likely to want data on > - You can rename the IPC messages to be more specific (remove the 'any' > sender/recipient term and replace with the actual process name. This makes the > IPC message code more readable > > If we are able to conceive a scenario where having the Daemon or RDP process > data is useful then adding more code is fine. I can't think of anything that > isn't a stretch.
A few comments on this approach if you decide not to go the centralized monitor process (using process IDs) route. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:148: void DaemonProcess::OnWorkerProcessStopped(int exit_code) { You can remove the exit_code param since it isn't being used. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:389: DCHECK(process_stats_request_count_ > 0); If you want to keep the request_count, I'd prefer that we DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment that the functionality needs to be tested before enabling if we support multiple, concurrent sessions in the future. The reason is that since we can't test the code, I'd prefer to indicate that explicitly instead of assuming it will work later. The other option is to remove the process_stats_request_count_ completely and DCHECK on whether the stats_sender_ is valid. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:392: DCHECK_EQ(process_stats_request_count_ == 1, !stats_sender_); I don't think you should allow calling start multiple times with different intervals, that just complicates the code. I don't think the ability to change the interval dynamically is useful, either way the originating process can send a stop IPC and then restart the timers with the new interval. Removing this extra condition means you can simplify the code too as process_stats_request_count_ is guaranteed to be 1: DCHECK(!stats_sender_); https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0); DCHECK_EQ(process_stats_request_count_, 0) https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process_unittest.cc (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process_unittest.cc:363: } If you feel strongly about keeping the counter, then please add tests to validate it (multiple start calls). Also add a test for dynamic interval changes too if you keep that functionality. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr... File remoting/host/worker_process_ipc_delegate.h (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr... remoting/host/worker_process_ipc_delegate.h:33: // Notifies that the worker process stops for any reason. Wouldn't we know this because the IPC channel gets disconnected? I'm not sure that this method is necessary. Seeing the additional changes makes me think that using the Daemon process (or network process) with process IDs is a much simpler approach.
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.
I think it's still reasonable to use this solution before finishing the investigation of other approaches. I can hard to say that fetching process states from daemon process is a very cheap change which is worthy to fully ignore all existing work. We may still face other problems during the implementations. For example, - What if process stats are required from multiple sessions (well, at least we can enable the feature for one ME2ME session and one IT2ME session)? Should we cache the result for a while or calling native API each time? - How should we ensure the platform independence? Current solution uses a platform neutral API in base which is supported on each platform. (Though I know technically speaking Daemon Process is used by Windows only.) So before we figured out all the issues we may face, I still think it's worthy to make current solution work. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:148: void DaemonProcess::OnWorkerProcessStopped(int exit_code) { On 2017/06/23 18:03:07, joedow wrote: > You can remove the exit_code param since it isn't being used. From the perspective of WorkerProcessLauncher, it's pretty reasonable to export the exit_code in OnWorkerProcessStopped() callback. The same parameter exists in OnPermanentError() callback. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:389: DCHECK(process_stats_request_count_ > 0); On 2017/06/23 18:03:07, joedow wrote: > If you want to keep the request_count, I'd prefer that we > DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment that > the functionality needs to be tested before enabling if we support multiple, > concurrent sessions in the future. > > The reason is that since we can't test the code, I'd prefer to indicate that > explicitly instead of assuming it will work later. > > The other option is to remove the process_stats_request_count_ completely and > DCHECK on whether the stats_sender_ is valid. Test cases are added to cover the scenarios. I think it's pretty fine to assume DaemonProcess can work as expected now :) https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:392: DCHECK_EQ(process_stats_request_count_ == 1, !stats_sender_); On 2017/06/23 18:03:07, joedow wrote: > I don't think you should allow calling start multiple times with different > intervals, that just complicates the code. I don't think the ability to change > the interval dynamically is useful, either way the originating process can send > a stop IPC and then restart the timers with the new interval. > > Removing this extra condition means you can simplify the code too as > process_stats_request_count_ is guaranteed to be 1: > > DCHECK(!stats_sender_); Here the trouble is different sessions may request different interval. I have added test case to cover it anyway. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0); On 2017/06/23 18:03:07, joedow wrote: > DCHECK_EQ(process_stats_request_count_, 0) Done. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process_unittest.cc (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process_unittest.cc:363: } On 2017/06/23 18:03:07, joedow wrote: > If you feel strongly about keeping the counter, then please add tests to > validate it (multiple start calls). Also add a test for dynamic interval > changes too if you keep that functionality. Done. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr... File remoting/host/worker_process_ipc_delegate.h (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr... remoting/host/worker_process_ipc_delegate.h:33: // Notifies that the worker process stops for any reason. On 2017/06/23 18:03:08, joedow wrote: > Wouldn't we know this because the IPC channel gets disconnected? I'm not sure > that this method is necessary. > > Seeing the additional changes makes me think that using the Daemon process (or > network process) with process IDs is a much simpler approach. I do not think the disconnecting event has been sent to WorkerProcessIpcDelegate / DaemonProcess. DaemonProcess does not own the IPC channel to the worker process AFAIK.
On 2017/06/24 00:51:41, Hzj_jie wrote: > I think it's still reasonable to use this solution before finishing the > investigation of other approaches. I can hard to say that fetching process > states from daemon process is a very cheap change which is worthy to fully > ignore all existing work. We may still face other problems during the > implementations. > For example, > - What if process stats are required from multiple sessions (well, at least we > can enable the feature for one ME2ME session and one IT2ME session)? Should we > cache the result for a while or calling native API each time? > - How should we ensure the platform independence? Current solution uses a > platform neutral API in base which is supported on each platform. (Though I know > technically speaking Daemon Process is used by Windows only.) > > So before we figured out all the issues we may face, I still think it's worthy > to make current solution work. > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > File remoting/host/daemon_process.cc (right): > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > remoting/host/daemon_process.cc:148: void > DaemonProcess::OnWorkerProcessStopped(int exit_code) { > On 2017/06/23 18:03:07, joedow wrote: > > You can remove the exit_code param since it isn't being used. > > From the perspective of WorkerProcessLauncher, it's pretty reasonable to export > the exit_code in OnWorkerProcessStopped() callback. The same parameter exists in > OnPermanentError() callback. > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > remoting/host/daemon_process.cc:389: DCHECK(process_stats_request_count_ > 0); > On 2017/06/23 18:03:07, joedow wrote: > > If you want to keep the request_count, I'd prefer that we > > DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment that > > the functionality needs to be tested before enabling if we support multiple, > > concurrent sessions in the future. > > > > The reason is that since we can't test the code, I'd prefer to indicate that > > explicitly instead of assuming it will work later. > > > > The other option is to remove the process_stats_request_count_ completely and > > DCHECK on whether the stats_sender_ is valid. > > Test cases are added to cover the scenarios. > I think it's pretty fine to assume DaemonProcess can work as expected now :) > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > remoting/host/daemon_process.cc:392: DCHECK_EQ(process_stats_request_count_ == > 1, !stats_sender_); > On 2017/06/23 18:03:07, joedow wrote: > > I don't think you should allow calling start multiple times with different > > intervals, that just complicates the code. I don't think the ability to > change > > the interval dynamically is useful, either way the originating process can > send > > a stop IPC and then restart the timers with the new interval. > > > > Removing this extra condition means you can simplify the code too as > > process_stats_request_count_ is guaranteed to be 1: > > > > DCHECK(!stats_sender_); > > Here the trouble is different sessions may request different interval. I have > added test case to cover it anyway. > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0); > On 2017/06/23 18:03:07, joedow wrote: > > DCHECK_EQ(process_stats_request_count_, 0) > > Done. > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > File remoting/host/daemon_process_unittest.cc (right): > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... > remoting/host/daemon_process_unittest.cc:363: } > On 2017/06/23 18:03:07, joedow wrote: > > If you feel strongly about keeping the counter, then please add tests to > > validate it (multiple start calls). Also add a test for dynamic interval > > changes too if you keep that functionality. > > Done. > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr... > File remoting/host/worker_process_ipc_delegate.h (right): > > https://codereview.chromium.org/2950993003/diff/60001/remoting/host/worker_pr... > remoting/host/worker_process_ipc_delegate.h:33: // Notifies that the worker > process stops for any reason. > On 2017/06/23 18:03:08, joedow wrote: > > Wouldn't we know this because the IPC channel gets disconnected? I'm not sure > > that this method is necessary. > > > > Seeing the additional changes makes me think that using the Daemon process (or > > network process) with process IDs is a much simpler approach. > > I do not think the disconnecting event has been sent to WorkerProcessIpcDelegate > / DaemonProcess. DaemonProcess does not own the IPC channel to the worker > process AFAIK. Any comments to this change?
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:148: void DaemonProcess::OnWorkerProcessStopped(int exit_code) { On 2017/06/24 00:51:40, Hzj_jie wrote: > On 2017/06/23 18:03:07, joedow wrote: > > You can remove the exit_code param since it isn't being used. > > From the perspective of WorkerProcessLauncher, it's pretty reasonable to export > the exit_code in OnWorkerProcessStopped() callback. The same parameter exists in > OnPermanentError() callback. I think we should prefer a cleaner function signature unless you can think of a usage of the exit_code. It is only used in a DCHECK in the permanent error method (which is OK) but is completely unused here. If we decide we need it at some point in the future, we can add it back. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0); On 2017/06/24 00:51:40, Hzj_jie wrote: > On 2017/06/23 18:03:07, joedow wrote: > > DCHECK_EQ(process_stats_request_count_, 0) > > Done. I don't see this change in the latest patchset. If you want to keep the condition, then you can use DCHECK_GE.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #4 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... File remoting/host/daemon_process.cc (right): https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:148: void DaemonProcess::OnWorkerProcessStopped(int exit_code) { On 2017/06/29 17:24:03, joedow wrote: > On 2017/06/24 00:51:40, Hzj_jie wrote: > > On 2017/06/23 18:03:07, joedow wrote: > > > You can remove the exit_code param since it isn't being used. > > > > From the perspective of WorkerProcessLauncher, it's pretty reasonable to > export > > the exit_code in OnWorkerProcessStopped() callback. The same parameter exists > in > > OnPermanentError() callback. > > I think we should prefer a cleaner function signature unless you can think of a > usage of the exit_code. It is only used in a DCHECK in the permanent error > method (which is OK) but is completely unused here. If we decide we need it at > some point in the future, we can add it back. Removed. https://codereview.chromium.org/2950993003/diff/60001/remoting/host/daemon_pr... remoting/host/daemon_process.cc:403: DCHECK(process_stats_request_count_ >= 0); On 2017/06/29 17:24:03, joedow wrote: > On 2017/06/24 00:51:40, Hzj_jie wrote: > > On 2017/06/23 18:03:07, joedow wrote: > > > DCHECK_EQ(process_stats_request_count_, 0) > > > > Done. > > I don't see this change in the latest patchset. > > If you want to keep the condition, then you can use DCHECK_GE. Sorry.
lgtm
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...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498778028052410, "parent_rev": "09d842cd48364ad7cb992ae0b84415a6bdeb148e", "commit_rev": "dda3acb809c43e2d633c335778ff4a1522927e59"}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process) This is a copy & paste from DesktopSessionAgent (https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon process to report process statistics data to network process. BUG=650926 ========== to ========== [Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process) This is a copy & paste from DesktopSessionAgent (https://codereview.chromium.org/2933203002/) to DaemonProcess to allow daemon process to report process statistics data to network process. BUG=650926 Review-Url: https://codereview.chromium.org/2950993003 Cr-Commit-Position: refs/heads/master@{#483539} Committed: https://chromium.googlesource.com/chromium/src/+/dda3acb809c43e2d633c335778ff... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/dda3acb809c43e2d633c335778ff... |