Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2080723008:
[Chromoting] Use device::PowerSaveBlocker to block screen saver (Closed)
Description[Chromoting] Use device::PowerSaveBlocker to block screen saver
Now ScreenCapturers implement their own screen-saver blocking logic, which is
not part of ScreenCapturer functionality. This change adds ScreenSaverBlocker
into It2MeHost or RemotingMe2MeHost as a HostStatusObserver. The
ScreenSaverBlocker implementation uses device::PowerSaverBlocker to do the real
job, and monitors HostStatusMonitor's OnClientConnected and OnClientDisconnected
events to start and stop PowerSaverBlocker. After this change, we can eventually
remove screen-saver blocking logic from various ScreenCapturer implementations.
BUG=626839
Committed: https://crrev.com/726a66222237491b51c1538854ea20e5df0b89d8
Cr-Commit-Position: refs/heads/master@{#405864}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Resolve review comments #
Total comments: 5
Patch Set 3 : Resolve review comments #
Total comments: 14
Patch Set 4 : Resolve review comments #
Total comments: 4
Patch Set 5 : Resolve review comments #Patch Set 6 : Add unittest for HostPowerSaveBlocker #
Total comments: 21
Patch Set 7 : Resolve review comments #Patch Set 8 : Resolve review comments #Patch Set 9 : Resolve review comments #
Messages
Total messages: 68 (41 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds PowerSaveBlocker into DesktopCapturerProxy. So we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG= ========== to ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds PowerSaveBlocker into DesktopCapturerProxy. So we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG= ==========
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:74: caller_task_runner_, We have not sent file_task_runner to BasicDesktopEnvironment, so I choose network_task_runner (caller_task_runner_) instead. The IO blocking jobs are only injected in PowerSaveBlocker's constructor and destructor, i.e. DesktopCapturerProxy's constructor and destructor. Since there are no network requests during that time, the impact would be small.
https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:74: caller_task_runner_, On 2016/06/22 18:57:59, Hzj_jie wrote: > We have not sent file_task_runner to BasicDesktopEnvironment, so I choose > network_task_runner (caller_task_runner_) instead. The IO blocking jobs are only > injected in PowerSaveBlocker's constructor and destructor, i.e. > DesktopCapturerProxy's constructor and destructor. Since there are no network > requests during that time, the impact would be small. I don't think this is acceptable. We should never have any blocking tasks running on network thread. https://codereview.chromium.org/2080723008/diff/20001/remoting/host/desktop_c... File remoting/host/desktop_capturer_proxy.h (right): https://codereview.chromium.org/2080723008/diff/20001/remoting/host/desktop_c... remoting/host/desktop_capturer_proxy.h:61: device::PowerSaveBlocker power_save_blocker_; Power-blocking isn't related to screen capturing, so I don't think it belongs here. Put it in ClientSession?
Description was changed from ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds PowerSaveBlocker into DesktopCapturerProxy. So we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG= ========== to ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds PowerSaveBlocker into ClientSession. So we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. PowerSaveBlocker needs two task runners, a UI one and a blocking one, to execute platform specific tasks. To achieve the requirement, this change also does a tiny refactor. Now a ChromotingHostContext will be sent to ChromotingHost and ClientSession. ChromotingHost keeps a copy of the input ChromotingHostContext, ClientSession keeps one reference of the audio_task_runner, PowerSaveBlocker keeps references of ui_task_runner and file_task_runner. BUG= ==========
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:160001) has been deleted
https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:74: caller_task_runner_, On 2016/06/22 19:05:37, Sergey Ulanov wrote: > On 2016/06/22 18:57:59, Hzj_jie wrote: > > We have not sent file_task_runner to BasicDesktopEnvironment, so I choose > > network_task_runner (caller_task_runner_) instead. The IO blocking jobs are > only > > injected in PowerSaveBlocker's constructor and destructor, i.e. > > DesktopCapturerProxy's constructor and destructor. Since there are no network > > requests during that time, the impact would be small. > > I don't think this is acceptable. We should never have any blocking tasks > running on network thread. I think all the jobs in network thread are all IO jobs, thus type of network_task_runner is TYPE_IO. But I also found a DisallowBlockingOpertions has been applied to it, which looks weird. Can I assume only asynchronized jobs are executing on a network_task_runner? https://codereview.chromium.org/2080723008/diff/20001/remoting/host/desktop_c... File remoting/host/desktop_capturer_proxy.h (right): https://codereview.chromium.org/2080723008/diff/20001/remoting/host/desktop_c... remoting/host/desktop_capturer_proxy.h:61: device::PowerSaveBlocker power_save_blocker_; On 2016/06/22 19:05:37, Sergey Ulanov wrote: > Power-blocking isn't related to screen capturing, so I don't think it belongs > here. Put it in ClientSession? Done.
I think it's better to implement this functionality outside of ChromotingHost. E.g. there is no reason to pull power-save-blocker in tests. I suggest you add a separate HostStatusObserver, so ChoromotingHost will not need to have any dependencies on PowerSaveBlocker. So the new implementation would be initialized similar to HostStatusLogger. In this change you also pass ChromotingHostContext around. It would be best to put that kind of change in a separate CL, but I don't think you really need to do that if you implement the alternative design I suggested above. https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/2080723008/diff/20001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:74: caller_task_runner_, On 2016/06/24 19:14:29, Hzj_jie wrote: > On 2016/06/22 19:05:37, Sergey Ulanov wrote: > > On 2016/06/22 18:57:59, Hzj_jie wrote: > > > We have not sent file_task_runner to BasicDesktopEnvironment, so I choose > > > network_task_runner (caller_task_runner_) instead. The IO blocking jobs are > > only > > > injected in PowerSaveBlocker's constructor and destructor, i.e. > > > DesktopCapturerProxy's constructor and destructor. Since there are no > network > > > requests during that time, the impact would be small. > > > > I don't think this is acceptable. We should never have any blocking tasks > > running on network thread. > > I think all the jobs in network thread are all IO jobs, thus type of > network_task_runner is TYPE_IO. But I also found a DisallowBlockingOpertions has > been applied to it, which looks weird. Can I assume only asynchronized jobs are > executing on a network_task_runner? That's right, we only allow non-blocking IO on that thread. https://codereview.chromium.org/2080723008/diff/180001/remoting/DEPS File remoting/DEPS (right): https://codereview.chromium.org/2080723008/diff/180001/remoting/DEPS#newcode3 remoting/DEPS:3: "+device/power_save_blocker", put this in remoting/host/DEPS. Only host can depend on it https://codereview.chromium.org/2080723008/diff/180001/remoting/test/protocol... File remoting/test/protocol_perftest.cc (right): https://codereview.chromium.org/2080723008/diff/180001/remoting/test/protocol... remoting/test/protocol_perftest.cc:112: message_loop_.task_runner(), run_loop_.QuitClosure())) { Previously the host was running on a separate thread. This moves it to the current message loop, and I don't think we want to do that. https://codereview.chromium.org/2080723008/diff/180001/remoting/test/protocol... remoting/test/protocol_perftest.cc:171: context_->video_encode_task_runner().get(), FROM_HERE, We want to use separate threads for encoding and decoding. Otherwise host and client would interfere with each other, which affects test results. https://codereview.chromium.org/2080723008/diff/180001/remoting/test/protocol... remoting/test/protocol_perftest.cc:474: scoped_refptr<AutoThreadTaskRunner> host_thread_; this is a task_runner, so it shouldn't be called host_thread_.
Also, I'm sorry I realized that HostStatusObserver is useful here only after you spent your time implementing the current solution :-(
On 2016/06/27 18:41:27, Sergey Ulanov wrote: > Also, I'm sorry I realized that HostStatusObserver is useful here only after you > spent your time implementing the current solution :-( No matter :) But I need some time to implement the HostStatusObserver solution.
Patchset #3 (id:200001) has been deleted
https://codereview.chromium.org/2080723008/diff/180001/remoting/DEPS File remoting/DEPS (right): https://codereview.chromium.org/2080723008/diff/180001/remoting/DEPS#newcode3 remoting/DEPS:3: "+device/power_save_blocker", On 2016/06/27 18:20:24, Sergey Ulanov wrote: > put this in remoting/host/DEPS. Only host can depend on it Done.
Please update BUG= reference in the description https://codereview.chromium.org/2080723008/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2080723008/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:977: if (false && base::FeatureList::IsEnabled(kPostQuantumExperiment)) { Shouldn't be part of this CL Also Joe landed a fix, so you don't need this workaround anymore https://codereview.chromium.org/2080723008/diff/220001/remoting/host/it2me/it... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/it2me/it... remoting/host/it2me/it2me_host.cc:276: screen_saver_blocker_.reset( Do we want power-save blocker for It2Me host? I'm not sure it's useful in this scenario. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/remoting... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/remoting... remoting/host/remoting_me2me_host.cc:1478: new ScreenSaverBlocker(host_->AsWeakPtr(), context_->Copy())); Please don't use ChromotingHostContext::Copy(). Ideally we should remove that method. Context shouldn't be copyable. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/remoting... remoting/host/remoting_me2me_host.cc:1543: host_change_notification_listener_.reset(); Reset screen_saver_blocker_ here too. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... File remoting/host/screen_saver_blocker.cc (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... remoting/host/screen_saver_blocker.cc:33: device::PowerSaveBlocker::kReasonVideoPlayback, Why not kReasonOther? https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... File remoting/host/screen_saver_blocker.h (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... remoting/host/screen_saver_blocker.h:22: class ScreenSaverBlocker : public HostStatusObserver { I don't think this is the best name, as it blocks sleep as well, not only screen saver. Call it HostPowerSaveBlocker? https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... remoting/host/screen_saver_blocker.h:25: std::unique_ptr<ChromotingHostContext>&& context); Please pass unique_ptr by value instead of rvalue. But in this case we shouldn't be passing context ownership at all. Instead I suggest passing task runners for the UI and file threads.
Also please update CL description - it doesn't match the latest impl
Description was changed from ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds PowerSaveBlocker into ClientSession. So we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. PowerSaveBlocker needs two task runners, a UI one and a blocking one, to execute platform specific tasks. To achieve the requirement, this change also does a tiny refactor. Now a ChromotingHostContext will be sent to ChromotingHost and ClientSession. ChromotingHost keeps a copy of the input ChromotingHostContext, ClientSession keeps one reference of the audio_task_runner, PowerSaveBlocker keeps references of ui_task_runner and file_task_runner. BUG= ========== to ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds ScreenSaverBlocker into It2MeHost or RemotingMe2MeHost as a HostStatusObserver. The ScreenSaverBlocker implementation uses device::PowerSaverBlocker to do the real job, and monitors HostStatusMonitor's OnClientConnected and OnClientDisconnected events to start and stop PowerSaverBlocker. After this change, we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG=626839 ==========
https://codereview.chromium.org/2080723008/diff/220001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2080723008/diff/220001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:977: if (false && base::FeatureList::IsEnabled(kPostQuantumExperiment)) { On 2016/07/08 21:31:46, Sergey Ulanov wrote: > Shouldn't be part of this CL > Also Joe landed a fix, so you don't need this workaround anymore Sorry, this change has been started before Joe's fix. And I should revert this line before sending out for reviewing. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/it2me/it... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/it2me/it... remoting/host/it2me/it2me_host.cc:276: screen_saver_blocker_.reset( On 2016/07/08 21:31:46, Sergey Ulanov wrote: > Do we want power-save blocker for It2Me host? I'm not sure it's useful in this > scenario. Emmm, good point. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/remoting... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/remoting... remoting/host/remoting_me2me_host.cc:1478: new ScreenSaverBlocker(host_->AsWeakPtr(), context_->Copy())); On 2016/07/08 21:31:47, Sergey Ulanov wrote: > Please don't use ChromotingHostContext::Copy(). Ideally we should remove that > method. Context shouldn't be copyable. Done. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/remoting... remoting/host/remoting_me2me_host.cc:1543: host_change_notification_listener_.reset(); On 2016/07/08 21:31:47, Sergey Ulanov wrote: > Reset screen_saver_blocker_ here too. Done. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... File remoting/host/screen_saver_blocker.cc (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... remoting/host/screen_saver_blocker.cc:33: device::PowerSaveBlocker::kReasonVideoPlayback, On 2016/07/08 21:31:47, Sergey Ulanov wrote: > Why not kReasonOther? Done. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... File remoting/host/screen_saver_blocker.h (right): https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... remoting/host/screen_saver_blocker.h:22: class ScreenSaverBlocker : public HostStatusObserver { On 2016/07/08 21:31:47, Sergey Ulanov wrote: > I don't think this is the best name, as it blocks sleep as well, not only screen > saver. Call it HostPowerSaveBlocker? Indeed blocking screensaver implies the suspending should also be blocked. Since a hardware cannot display when it suspended. So PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep implies kPowerSaveBlockPreventAppSuspension. But I have no strong feeling about this naming. Updated. https://codereview.chromium.org/2080723008/diff/220001/remoting/host/screen_s... remoting/host/screen_saver_blocker.h:25: std::unique_ptr<ChromotingHostContext>&& context); On 2016/07/08 21:31:47, Sergey Ulanov wrote: > Please pass unique_ptr by value instead of rvalue. But in this case we shouldn't > be passing context ownership at all. Instead I suggest passing task runners for > the UI and file threads. Done.
looks good, but can you please add a simple unittest. https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.cc (right): https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... remoting/host/host_power_save_blocker.cc:17: const ChromotingHostContext& context) Please replace this parameter with two task runners. ChromotingHostContext is currently used only when setting up a host. https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.h (right): https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... remoting/host/host_power_save_blocker.h:20: class AutoThreadTaskRunner; sort these alphabetically
I have addressed your comments. But I cannot quite sure how to write a unittest for this class. Feel free to drop by if you have some hints. https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.cc (right): https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... remoting/host/host_power_save_blocker.cc:17: const ChromotingHostContext& context) On 2016/07/13 04:57:05, Sergey Ulanov wrote: > Please replace this parameter with two task runners. ChromotingHostContext is > currently used only when setting up a host. Done. https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.h (right): https://codereview.chromium.org/2080723008/diff/240001/remoting/host/host_pow... remoting/host/host_power_save_blocker.h:20: class AutoThreadTaskRunner; On 2016/07/13 04:57:05, Sergey Ulanov wrote: > sort these alphabetically Done.
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #6 (id:280001) 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/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.h (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker.h:35: const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner); I think these both should be SingleThreadTaskRunner. Why do you need SequencedTaskRunner for ui_task_runner? https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker_unittest.cc (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:24: private: All of this variables can be protected. There is no reason to hide them from the tests. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:25: base::Thread ui_thread_; On UI thread must be the main application thread. Create MessageLoopForUI for the main thread here https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:26: base::Thread block_thread_; blocking_io_thread_ or file_thread_? https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:36: DCHECK(ui_thread_.Start()); this won't start the thread in release builds. Never put an expression with side-effects inside DCHECK(). Give that this is a unittest, ASSERT_TRUE() would be more appropriate here. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:37: DCHECK(ui_thread_.WaitUntilThreadStarted()); Why? If you really need it then use StartAndWaitForTesting() https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:50: void HostPowerSaveBlockerTest::AssertDeactivated() { You can replaced these two methods with bool is_activated() and ASSERT in the test.
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/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.h (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker.h:35: const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner); On 2016/07/14 19:52:48, Sergey Ulanov wrote: > I think these both should be SingleThreadTaskRunner. Why do you need > SequencedTaskRunner for ui_task_runner? Follow the definition of PowerSaveBlocker. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker_unittest.cc (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:24: private: On 2016/07/14 19:52:48, Sergey Ulanov wrote: > All of this variables can be protected. There is no reason to hide them from the > tests. Done. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:25: base::Thread ui_thread_; On 2016/07/14 19:52:48, Sergey Ulanov wrote: > On UI thread must be the main application thread. Create MessageLoopForUI for > the main thread here Done. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:26: base::Thread block_thread_; On 2016/07/14 19:52:48, Sergey Ulanov wrote: > blocking_io_thread_ or file_thread_? Follow PowerSaveBlocker definition. It's a little bit confusion, but I believe it should be a blocking io thread. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:36: DCHECK(ui_thread_.Start()); On 2016/07/14 19:52:48, Sergey Ulanov wrote: > this won't start the thread in release builds. Never put an expression with > side-effects inside DCHECK(). Give that this is a unittest, ASSERT_TRUE() would > be more appropriate here. ASSERT_TRUE won't work in constructor (It returns an error message.) https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:37: DCHECK(ui_thread_.WaitUntilThreadStarted()); On 2016/07/14 19:52:48, Sergey Ulanov wrote: > Why? > If you really need it then use StartAndWaitForTesting() Done. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:50: void HostPowerSaveBlockerTest::AssertDeactivated() { On 2016/07/14 19:52:48, Sergey Ulanov wrote: > You can replaced these two methods with bool is_activated() and ASSERT in the > test. Done.
https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.h (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker.h:35: const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner); On 2016/07/14 23:20:03, Hzj_jie wrote: > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > I think these both should be SingleThreadTaskRunner. Why do you need > > SequencedTaskRunner for ui_task_runner? > > Follow the definition of PowerSaveBlocker. I'm not sure why they used SequencedTaskRunner there, but I don't see any reason to copy it. Please use SingleThreadTaskRunner for both task runners. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker_unittest.cc (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:26: base::Thread block_thread_; On 2016/07/14 23:20:03, Hzj_jie wrote: > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > blocking_io_thread_ or file_thread_? > > Follow PowerSaveBlocker definition. It's a little bit confusion, but I believe > it should be a blocking io thread. Call it blocking_thread_? block_thread_ is confusing. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:36: DCHECK(ui_thread_.Start()); On 2016/07/14 23:20:03, Hzj_jie wrote: > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > this won't start the thread in release builds. Never put an expression with > > side-effects inside DCHECK(). Give that this is a unittest, ASSERT_TRUE() > would > > be more appropriate here. > > ASSERT_TRUE won't work in constructor (It returns an error message.) It should work. It doesn't return any result. It will simply return from the constructor when assertion fails.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker.h (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker.h:35: const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner); On 2016/07/14 23:30:56, Sergey Ulanov wrote: > On 2016/07/14 23:20:03, Hzj_jie wrote: > > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > > I think these both should be SingleThreadTaskRunner. Why do you need > > > SequencedTaskRunner for ui_task_runner? > > > > Follow the definition of PowerSaveBlocker. > > I'm not sure why they used SequencedTaskRunner there, but I don't see any reason > to copy it. Please use SingleThreadTaskRunner for both task runners. Done. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker_unittest.cc (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:26: base::Thread block_thread_; On 2016/07/14 23:30:57, Sergey Ulanov wrote: > On 2016/07/14 23:20:03, Hzj_jie wrote: > > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > > blocking_io_thread_ or file_thread_? > > > > Follow PowerSaveBlocker definition. It's a little bit confusion, but I believe > > it should be a blocking io thread. > > Call it blocking_thread_? block_thread_ is confusing. Oh, sorry, I missed ing. https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:36: DCHECK(ui_thread_.Start()); On 2016/07/14 23:30:57, Sergey Ulanov wrote: > On 2016/07/14 23:20:03, Hzj_jie wrote: > > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > > this won't start the thread in release builds. Never put an expression with > > > side-effects inside DCHECK(). Give that this is a unittest, ASSERT_TRUE() > > would > > > be more appropriate here. > > > > ASSERT_TRUE won't work in constructor (It returns an error message.) > > It should work. It doesn't return any result. It will simply return from the > constructor when assertion fails. Emmm... Here is the error message, ../../remoting/host/host_power_save_blocker_unittest.cc:32:3: error: constructor 'HostPowerSaveBlockerTest' must not return void expression ASSERT_TRUE(blocking_thread_.StartWithOptions( ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro 'ASSERT_TRUE' GTEST_FATAL_FAILURE_) ^~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: expanded from macro 'GTEST_TEST_BOOLEAN_' fail(::testing::internal::GetBoolAssertionFailureMessage(\ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: expanded from macro 'GTEST_FATAL_FAILURE_' return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
LGTM after my comment is addressed https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... File remoting/host/host_power_save_blocker_unittest.cc (right): https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... remoting/host/host_power_save_blocker_unittest.cc:36: DCHECK(ui_thread_.Start()); On 2016/07/15 02:12:00, Hzj_jie wrote: > On 2016/07/14 23:30:57, Sergey Ulanov wrote: > > On 2016/07/14 23:20:03, Hzj_jie wrote: > > > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > > > this won't start the thread in release builds. Never put an expression > with > > > > side-effects inside DCHECK(). Give that this is a unittest, ASSERT_TRUE() > > > would > > > > be more appropriate here. > > > > > > ASSERT_TRUE won't work in constructor (It returns an error message.) > > > > It should work. It doesn't return any result. It will simply return from the > > constructor when assertion fails. > > Emmm... Here is the error message, > ../../remoting/host/host_power_save_blocker_unittest.cc:32:3: error: constructor > 'HostPowerSaveBlockerTest' must not return void expression > ASSERT_TRUE(blocking_thread_.StartWithOptions( > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro > 'ASSERT_TRUE' > GTEST_FATAL_FAILURE_) > ^~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: > expanded from macro 'GTEST_TEST_BOOLEAN_' > fail(::testing::internal::GetBoolAssertionFailureMessage(\ > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: > expanded from macro 'GTEST_FATAL_FAILURE_' > return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "must not return void expression" gah, C++, this is so dumb! Then I guess you can move initialization to SetUp() method, which actually returns void?
On 2016/07/15 06:15:33, Sergey Ulanov wrote: > LGTM after my comment is addressed > > https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... > File remoting/host/host_power_save_blocker_unittest.cc (right): > > https://codereview.chromium.org/2080723008/diff/300001/remoting/host/host_pow... > remoting/host/host_power_save_blocker_unittest.cc:36: > DCHECK(ui_thread_.Start()); > On 2016/07/15 02:12:00, Hzj_jie wrote: > > On 2016/07/14 23:30:57, Sergey Ulanov wrote: > > > On 2016/07/14 23:20:03, Hzj_jie wrote: > > > > On 2016/07/14 19:52:48, Sergey Ulanov wrote: > > > > > this won't start the thread in release builds. Never put an expression > > with > > > > > side-effects inside DCHECK(). Give that this is a unittest, > ASSERT_TRUE() > > > > would > > > > > be more appropriate here. > > > > > > > > ASSERT_TRUE won't work in constructor (It returns an error message.) > > > > > > It should work. It doesn't return any result. It will simply return from the > > > constructor when assertion fails. > > > > Emmm... Here is the error message, > > ../../remoting/host/host_power_save_blocker_unittest.cc:32:3: error: > constructor > > 'HostPowerSaveBlockerTest' must not return void expression > > ASSERT_TRUE(blocking_thread_.StartWithOptions( > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro > > 'ASSERT_TRUE' > > GTEST_FATAL_FAILURE_) > > ^~~~~~~~~~~~~~~~~~~~~ > > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: > > expanded from macro 'GTEST_TEST_BOOLEAN_' > > fail(::testing::internal::GetBoolAssertionFailureMessage(\ > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: > > expanded from macro 'GTEST_FATAL_FAILURE_' > > return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) > > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > "must not return void expression" gah, C++, this is so dumb! > > Then I guess you can move initialization to SetUp() method, which actually > returns void? That's a good idea. I will make the change.
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
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/2080723008/#ps360001 (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...)
zijiehe@chromium.org changed reviewers: + boliu@chromium.org
Hi, Bo, Would you please sign-off for this change? We will use PowerSaveBlocker in Chrome Remoting Desktop host to block host system to shutdown screen or suspend.
On 2016/07/15 18:11:09, Hzj_jie wrote: > Hi, Bo, > Would you please sign-off for this change? We will use PowerSaveBlocker in > Chrome Remoting Desktop host to block host system to shutdown screen or suspend. 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...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds ScreenSaverBlocker into It2MeHost or RemotingMe2MeHost as a HostStatusObserver. The ScreenSaverBlocker implementation uses device::PowerSaverBlocker to do the real job, and monitors HostStatusMonitor's OnClientConnected and OnClientDisconnected events to start and stop PowerSaverBlocker. After this change, we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG=626839 ========== to ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds ScreenSaverBlocker into It2MeHost or RemotingMe2MeHost as a HostStatusObserver. The ScreenSaverBlocker implementation uses device::PowerSaverBlocker to do the real job, and monitors HostStatusMonitor's OnClientConnected and OnClientDisconnected events to start and stop PowerSaverBlocker. After this change, we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG=626839 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:360001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds ScreenSaverBlocker into It2MeHost or RemotingMe2MeHost as a HostStatusObserver. The ScreenSaverBlocker implementation uses device::PowerSaverBlocker to do the real job, and monitors HostStatusMonitor's OnClientConnected and OnClientDisconnected events to start and stop PowerSaverBlocker. After this change, we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG=626839 ========== to ========== [Chromoting] Use device::PowerSaveBlocker to block screen saver Now ScreenCapturers implement their own screen-saver blocking logic, which is not part of ScreenCapturer functionality. This change adds ScreenSaverBlocker into It2MeHost or RemotingMe2MeHost as a HostStatusObserver. The ScreenSaverBlocker implementation uses device::PowerSaverBlocker to do the real job, and monitors HostStatusMonitor's OnClientConnected and OnClientDisconnected events to start and stop PowerSaverBlocker. After this change, we can eventually remove screen-saver blocking logic from various ScreenCapturer implementations. BUG=626839 Committed: https://crrev.com/726a66222237491b51c1538854ea20e5df0b89d8 Cr-Commit-Position: refs/heads/master@{#405864} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/726a66222237491b51c1538854ea20e5df0b89d8 Cr-Commit-Position: refs/heads/master@{#405864} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
