Convert //content/browser/webrtc to be clients of WakeLock mojo service.

content/browser/webrtc/webrtc_internals.h

Index: content/browser/webrtc/webrtc_internals.h
diff --git a/content/browser/webrtc/webrtc_internals.h b/content/browser/webrtc/webrtc_internals.h
index cc5324fd3a9e00c6f6890ae11be145c2610504d4..f3a5177dd6a998a96d61ef2d5d7f35a344b11b1c 100644
--- a/content/browser/webrtc/webrtc_internals.h
+++ b/content/browser/webrtc/webrtc_internals.h
@@ -19,13 +19,10 @@
 #include "base/values.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/render_process_host_observer.h"
+#include "device/wake_lock/public/interfaces/wake_lock_service.mojom.h"
 #include "media/media_features.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 
-namespace device {
-class PowerSaveBlocker;
-}  // namespace device
-
 namespace content {
 
 class WebContents;
@@ -111,7 +108,7 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   const base::FilePath& GetEventLogFilePath() const;
 
   int num_open_connections() const { return num_open_connections_; }
-  bool IsPowerSavingBlocked() const { return !!power_save_blocker_; }
+  bool HasWakeLockForTesting() const { return has_wake_lock_for_testing_; }

[emircan, 2017/05/18 00:09:33]

I think adding *ForTesting() methods should be the last resort. Instead, can you
add WebRtcInternalsTest as a friend class? That way you can access the
internals.

[ke.he, 2017/05/18 05:34:15]

I'll remove the *ForTesting() methods, Thanks!

 
  protected:
   // Constructor/Destructor are protected to allow tests to derive from the
@@ -165,9 +162,11 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   void MaybeClosePeerConnection(base::DictionaryValue* record);
 
   // Called whenever a PeerConnection is created or stopped in order to
-  // impose/release a block on suspending the current application for power
+  // request/cancel a wake lock on suspending the current application for power
   // saving.
-  void CreateOrReleasePowerSaveBlocker();
+  void UpdateWakeLock();
+
+  device::mojom::WakeLockService* GetWakeLock();

[chfremer, 2017/05/17 22:51:15]

nit: Should this method be named GetWakeLockService()?

[ke.he, 2017/05/18 05:34:15]

Done.

 
   // Called on a timer to deliver updates to javascript.
   // We throttle and bulk together updates to avoid DOS like scenarios where
@@ -218,11 +217,10 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   bool selecting_event_log_;
   base::FilePath event_log_recordings_file_path_;
 
-  // While |num_open_connections_| is greater than zero, hold an instance of
-  // PowerSaveBlocker.  This prevents the application from being suspended while
-  // remoting.
+  // While |num_open_connections_| is greater than zero, request a wake lock
+  // service. This prevents the application from being suspended while remoting.
   int num_open_connections_;
-  std::unique_ptr<device::PowerSaveBlocker> power_save_blocker_;
+  device::mojom::WakeLockServicePtr wake_lock_;
   const bool should_block_power_saving_;
 
   // Set of render process hosts that |this| is registered as an observer on.
@@ -256,6 +254,8 @@ class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
   std::queue<PendingUpdate> pending_updates_;
   const int aggregate_updates_ms_;
 
+  bool has_wake_lock_for_testing_;

[chfremer, 2017/05/17 22:51:15]

As an idea for how the same thing could be achieved without the need for adding
test-only state or the logic and dependencies inside GetWakeLock() to this
already huge class:

We could inject an instance of a new interface "WakeLockProvider" into the 
constructor of this class. Then we could use different implementations of this 
interface for testing and for production. The production implementation would 
talk to the actual service, while the testing implementation would just set a 
boolean flag that can be inspected by the test code for verification.

[ke.he, 2017/05/18 05:34:15]

Inject an instance of "WakeLockService" IIUC. Thanks!
Done.

[chfremer, 2017/05/18 17:48:31]

Nice, the solution in PatchSet 3 no longer requires this flag in the class under
test. What I initially meant to express is to go even further by completely
separating out the knowledge of any Mojo/Service/Connection related logic into a
separate class.

Here is a sketch of what it would look like:

class WakeLockProvider {
 public:
  virtual ~WakeLockProvider() {}
  virtual void RequestLock() = 0;
  virtual void CancelLock() = 0;
};

class ServiceWakeLockProvider : public WakeLockProvider {
 public:
  void RequestLock() override {
    // Talk to service ...
  }
  void CancelLock() override {
    // Talk to service ...
  }

 private:
  void LazyConnectToWakeLockService() {
    // Logic from GetWakeLock() could go here
  }

  device::mojom::WakeLockServicePtr wake_lock_service_;
};

class MockWakeLockProvider : public WakeLockProvider {
  // Sets boolean flag
};

// The existing class
class WebRTCInternals {
  // ...

  // Dependency injection via constructor
  WebRTCInternals(std::unique_ptr<WakeLockProvider> wake_lock_provider) :
wake_lock_provider_(std::move(wake_lock_provider)) {}

  // ...

 private:
  // ...
  std::unique_ptr<WakeLockProvider> wake_lock_provider_;
};

[ke.he, 2017/05/19 03:34:50]

Now I understand what you mean, thanks very much for your explain:)

+
   // Weak factory for this object that we use for bulking up updates.
   base::WeakPtrFactory<WebRTCInternals> weak_factory_;
 };