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

content/browser/webrtc/webrtc_internals.h

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_WEBRTC_WEBRTC_INTERNALS_H_
 #define CONTENT_BROWSER_WEBRTC_WEBRTC_INTERNALS_H_
 
 #include <memory>
 #include <queue>
 #include <string>
 
 #include "base/containers/hash_tables.h"
 #include "base/gtest_prod_util.h"
 #include "base/lazy_instance.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/process/process.h"
 #include "base/threading/thread_checker.h"
 #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;
 class WebRTCInternalsUIObserver;
 
 // This is a singleton class running in the browser UI thread.
 // It collects peer connection infomation from the renderers,
 // forwards the data to WebRTCInternalsUIObserver and
 // sends data collecting commands to the renderers.
 class CONTENT_EXPORT WebRTCInternals : public RenderProcessHostObserver,
@@ skipping 65 matching lines @@
   const base::FilePath& GetAudioDebugRecordingsFilePath() const;
 
   // Enables or disables diagnostic event log.
   void EnableEventLogRecordings(content::WebContents* web_contents);
   void DisableEventLogRecordings();
 
   bool IsEventLogRecordingsEnabled() const;
   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
   // class and do per-instance testing without having to use the global
   // instance.
   // The default ctor sets |aggregate_updates_ms| to 500ms.
   WebRTCInternals();
   WebRTCInternals(int aggregate_updates_ms, bool should_block_power_saving);
   ~WebRTCInternals() override;
 
@@ skipping 33 matching lines @@
   // Enables event log recordings on all render process hosts using
   // |event_log_recordings_file_path_|.
   void EnableEventLogRecordingsOnAllRenderProcessHosts();
 #endif
 
   // Updates the number of open PeerConnections. Called when a PeerConnection
   // is stopped or removed.
   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
   // a page uses a lot of peerconnection instances with many event
   // notifications.
   void ProcessPendingUpdates();
 
   base::DictionaryValue* FindRecord(base::ProcessId pid,
                                     int lid,
                                     size_t* index = nullptr);
@@ skipping 30 matching lines @@
 
   // Diagnostic audio recording state.
   bool audio_debug_recordings_;
   base::FilePath audio_debug_recordings_file_path_;
 
   // Diagnostic event log recording state.
   bool event_log_recordings_;
   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.
   base::hash_set<int> render_process_id_set_;
 
   // Used to bulk up updates that we send to javascript.
   // The class owns the value/dictionary and command name of an update.
   // For each update, a PendingUpdate is stored in the |pending_updates_| queue
   // and deleted as soon as the update has been delivered.
   // The class is moveble and not copyable to avoid copying while still allowing
@@ skipping 13 matching lines @@
    private:
     base::ThreadChecker thread_checker_;
     const char* command_;
     std::unique_ptr<base::Value> value_;
     DISALLOW_COPY_AND_ASSIGN(PendingUpdate);
   };
 
   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_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_WEBRTC_WEBRTC_INTERNALS_H_