|
|
Created:
3 years, 7 months ago by ke.he Modified:
3 years, 7 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chfremer+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, xjz+watch_chromium.org, mfoltz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert clients of PowerSaveBlocker to be clients of WakeLock mojo service.
Wake Lock is a Mojo interface that wraps PowerSaveBlocker, As part of the
creation of standalone Device Service, all browser-side clients of
PowerSaveBlocker should be converted to be clients of WakeLock mojo interface
instead.
This CL converts clients of PowerSaveBlocker in //content/browser/media/
BUG=689410
Review-Url: https://codereview.chromium.org/2881143003
Cr-Commit-Position: refs/heads/master@{#472967}
Committed: https://chromium.googlesource.com/chromium/src/+/bfff87457dc144fb7cc3d3a34d6c21c43369f867
Patch Set 1 #Patch Set 2 : Convert clients of PowerSaveBlocker to be clients of WakeLock mojo service. #Patch Set 3 : clone() should happen in UI threads. #
Total comments: 19
Patch Set 4 : Addressed Sergeyu@'s comments. #Patch Set 5 : wake lock bindinterface should only be executed once. #
Total comments: 4
Patch Set 6 : Revert patchset 5, add DCHECK(!wake_lock_). #Patch Set 7 : code rebase #
Messages
Total messages: 52 (44 generated)
The CQ bit was checked by ke.he@intel.com 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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ke.he@intel.com 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 ========== Convert clients of PowerSaveBlocker to be clients of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker, As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of WakeLock mojo interface instead. This CL converts clients of PowerSaveBlocker in //content/browser/media/ BUG=689410 ========== to ========== Convert clients of PowerSaveBlocker to be clients of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker, As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of WakeLock mojo interface instead. This CL converts clients of PowerSaveBlocker in //content/browser/media/ BUG=689410 ==========
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_...)
The CQ bit was checked by ke.he@intel.com 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.
ke.he@intel.com changed reviewers: + sergeyu@chromium.org
Hi, Sergeyu@, PTAL. thanks.
sergeyu@google.com changed reviewers: + sergeyu@google.com
https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/aura_window_capture_machine.cc (right): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/aura_window_capture_machine.cc:99: connector->BindInterface(device::mojom::kServiceName, Can this operation fail? https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/aura_window_capture_machine.cc:104: "DesktopCaptureDevice is running", mojo::MakeRequest(&wake_lock_)); Please change this string to "Desktop capturer is running" https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (right): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:69: std::unique_ptr<service_manager::Connector> GetConnector() { Maybe call it GetServiceConnector()? https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:115: // Requests a wake lock. I don't think this comment is useful - it just duplicates method name. Remove it? https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:117: const std::unique_ptr<service_manager::Connector>& connector); I don't think it needs to be const reference. Normally unique_ptr<> args are passed by value. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:200: // Gets a service_manager::connector first, then request a wake lock. s/connector/Connector/ https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:204: base::Unretained(this))); I don't think base::Unretained() is safe here. Add WeakPtrFactory in Core and use it here? https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:391: "DesktopCaptureDevice is running", mojo::MakeRequest(&wake_lock_)); Please change the description to "Desktop capturer is active".
sergeyu@chromium.org changed reviewers: - sergeyu@google.com
The CQ bit was checked by ke.he@intel.com 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_...)
The CQ bit was checked by ke.he@intel.com 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_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 ke.he@intel.com 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...
Hi, Sergey, thanks very much! Besides your comments, I also make changes to make sure the |wake_lock_| to be initialized once. PTAL. Thanks! https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/aura_window_capture_machine.cc (right): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/aura_window_capture_machine.cc:99: connector->BindInterface(device::mojom::kServiceName, On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > Can this operation fail? BindInterface() is a mojo call too. Even if the BindInterface() fails to create a connection to DeviceService, it is still safe to call wake_lock_provider_->GetWakeLockWithoutContext(), while in this case the DeviceService side won't really call GetWakeLockWithoutContext(). https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/aura_window_capture_machine.cc:104: "DesktopCaptureDevice is running", mojo::MakeRequest(&wake_lock_)); On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > Please change this string to "Desktop capturer is running" Done. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (right): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:69: std::unique_ptr<service_manager::Connector> GetConnector() { On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > Maybe call it GetServiceConnector()? Done. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:115: // Requests a wake lock. On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > I don't think this comment is useful - it just duplicates method name. Remove > it? Done. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:117: const std::unique_ptr<service_manager::Connector>& connector); On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > I don't think it needs to be const reference. Normally unique_ptr<> args are > passed by value. Done. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:200: // Gets a service_manager::connector first, then request a wake lock. On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > s/connector/Connector/ Done. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:204: base::Unretained(this))); On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > I don't think base::Unretained() is safe here. Add WeakPtrFactory in Core and > use it here? Oh! Yes, should use WeakPtr here. thanks! Done. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:391: "DesktopCaptureDevice is running", mojo::MakeRequest(&wake_lock_)); On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > Please change the description to "Desktop capturer is active". "is active" or "is running"? Done with "is running".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (left): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:143: // TODO(jiayl): Remove power_save_blocker_ when there is an API to keep the Please keep this TODO, it's still relevant https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (right): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:391: "DesktopCaptureDevice is running", mojo::MakeRequest(&wake_lock_)); On 2017/05/17 10:25:52, ke.he wrote: > On 2017/05/16 19:29:43, Do not use (sergeyu) wrote: > > Please change the description to "Desktop capturer is active". > > "is active" or "is running"? > Done with "is running". I'm fine with either. https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... File content/browser/media/capture/aura_window_capture_machine.cc (right): https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... content/browser/media/capture/aura_window_capture_machine.cc:92: if (!wake_lock_) { I don't think InternalStart() can be called more than once, so this can be replaced with DCHECK(!wake_lock_). https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (right): https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:202: if (!wake_lock_) { AllocateAndStart() is never called more than once, so I don't think you need this check.
The CQ bit was checked by ke.he@intel.com 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by ke.he@intel.com 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ke.he@intel.com 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...
Sergey, Thanks! CL is updated. PTAL. https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (left): https://codereview.chromium.org/2881143003/diff/40001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:143: // TODO(jiayl): Remove power_save_blocker_ when there is an API to keep the On 2017/05/17 18:47:55, Sergey Ulanov wrote: > Please keep this TODO, it's still relevant Done. and also in "aura_window_capture_machine.h" https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... File content/browser/media/capture/aura_window_capture_machine.cc (right): https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... content/browser/media/capture/aura_window_capture_machine.cc:92: if (!wake_lock_) { On 2017/05/17 18:47:55, Sergey Ulanov wrote: > I don't think InternalStart() can be called more than once, so this can be > replaced with DCHECK(!wake_lock_). Done. https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... File content/browser/media/capture/desktop_capture_device.cc (right): https://codereview.chromium.org/2881143003/diff/80001/content/browser/media/c... content/browser/media/capture/desktop_capture_device.cc:202: if (!wake_lock_) { On 2017/05/17 18:47:55, Sergey Ulanov wrote: > AllocateAndStart() is never called more than once, so I don't think you need > this check. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by ke.he@intel.com
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": 140001, "attempt_start_ts": 1495149194853380, "parent_rev": "2bbdd107bcc11f6b41469249dfc51bac7b25eb01", "commit_rev": "bfff87457dc144fb7cc3d3a34d6c21c43369f867"}
Message was sent while issue was closed.
Description was changed from ========== Convert clients of PowerSaveBlocker to be clients of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker, As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of WakeLock mojo interface instead. This CL converts clients of PowerSaveBlocker in //content/browser/media/ BUG=689410 ========== to ========== Convert clients of PowerSaveBlocker to be clients of WakeLock mojo service. Wake Lock is a Mojo interface that wraps PowerSaveBlocker, As part of the creation of standalone Device Service, all browser-side clients of PowerSaveBlocker should be converted to be clients of WakeLock mojo interface instead. This CL converts clients of PowerSaveBlocker in //content/browser/media/ BUG=689410 Review-Url: https://codereview.chromium.org/2881143003 Cr-Commit-Position: refs/heads/master@{#472967} Committed: https://chromium.googlesource.com/chromium/src/+/bfff87457dc144fb7cc3d3a34d6c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bfff87457dc144fb7cc3d3a34d6c... |