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

Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689413
Review-Url: https://codereview.chromium.org/2886603003
Cr-Commit-Position: refs/heads/master@{#473395}
Committed: https://chromium.googlesource.com/chromium/src/+/1671f06ee7b3e7ba18d4a8171d3e0c64d95a44b0

[ke.he, 2017-05-16 10:02:01]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 10:02:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 11:45:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 11:45:16]

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_...)

[ke.he, 2017-05-17 02:04:09]

ke.he@intel.com changed reviewers:
+ emircan@chromium.org

[ke.he, 2017-05-17 02:04:10]

Hi, Emircan@, Could you PTAL on this? Thanks.

[ke.he, 2017-05-17 07:35:12]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 07:35:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 08:37:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 08:37:10]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-05-17 17:14:30]

Description was changed from

==========
Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689410
==========

to

==========
Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689410
==========

[emircan, 2017-05-17 17:14:30]

emircan@chromium.org changed reviewers:
+ chfremer@chromium.org

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

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:169: device::mojom::WakeLockService*
GetWakeLock();
nit: Should this method be named GetWakeLockService()?

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:257: bool has_wake_lock_for_testing_;
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.

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

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:111: bool HasWakeLockForTesting()
const { return has_wake_lock_for_testing_; }
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 04:17:09]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 04:18:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

chfremer@, emircan@, Thank you very much!
CL is updated, PTAL.

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:111: bool HasWakeLockForTesting()
const { return has_wake_lock_for_testing_; }
On 2017/05/18 00:09:33, emircan wrote:
> 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.

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

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:169: device::mojom::WakeLockService*
GetWakeLock();
On 2017/05/17 22:51:15, chfremer wrote:
> nit: Should this method be named GetWakeLockService()?

Done.

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:257: bool has_wake_lock_for_testing_;
On 2017/05/17 22:51:15, chfremer wrote:
> 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.

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

[commit-bot: I haz the power, 2017-05-18 06:04:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 06:04:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:257: bool has_wake_lock_for_testing_;
On 2017/05/18 05:34:15, ke.he wrote:
> On 2017/05/17 22:51:15, chfremer wrote:
> > 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.
> 
> Inject an instance of "WakeLockService" IIUC. Thanks!
> Done.

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_;
};

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:121: device::mojom::WakeLockServicePtr
wake_lock_;
I recommend naming this |wake_lock_service_| to avoid readers assuming it is the
actual lock.

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals_unittest.cc (right):

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals_unittest.cc:58: void
HasWakeLockForTests(
Probably no longer needed?

[ke.he, 2017-05-19 03:34:22]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

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

Thanks! CL is updated, PTAL.

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/20001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:257: bool has_wake_lock_for_testing_;
On 2017/05/18 17:48:31, chfremer wrote:
> On 2017/05/18 05:34:15, ke.he wrote:
> > On 2017/05/17 22:51:15, chfremer wrote:
> > > 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.
> > 
> > Inject an instance of "WakeLockService" IIUC. Thanks!
> > Done.
> 
> 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_;
> };

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

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals.h (right):

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals.h:121: device::mojom::WakeLockServicePtr
wake_lock_;
On 2017/05/18 17:48:31, chfremer wrote:
> I recommend naming this |wake_lock_service_| to avoid readers assuming it is
the
> actual lock.

Done.

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_internals_unittest.cc (right):

https://codereview.chromium.org/2886603003/diff/40001/content/browser/webrtc/...
content/browser/webrtc/webrtc_internals_unittest.cc:58: void
HasWakeLockForTests(
On 2017/05/18 17:48:31, chfremer wrote:
> Probably no longer needed?

They are purl virtual functions in the base class, so let's keep them:)

[commit-bot: I haz the power, 2017-05-19 03:34:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ke.he, 2017-05-19 04:10:00]

Description was changed from

==========
Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689410
==========

to

==========
Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689413
==========

[commit-bot: I haz the power, 2017-05-19 06:11:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 06:11:32]

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_...)

[chfremer, 2017-05-19 16:39:48]

PatchSet #4 lgtm

Just double-checking, since I couldn't tell from your reply: You chose to not go
with the dependency injection proposal, right (which is okay)?

[emircan, 2017-05-19 17:05:10]

PS#4 lgtm

[ke.he, 2017-05-20 00:27:37]

On 2017/05/19 16:39:48, chfremer wrote:
> PatchSet #4 lgtm
> 
> Just double-checking, since I couldn't tell from your reply: You chose to not
go
> with the dependency injection proposal, right (which is okay)?

Yes, I think both solutions are ok so I didn't go with the dependency injection
which would change much more code:)

Thanks.

[ke.he, 2017-05-20 00:28:15]

The CQ bit was checked by ke.he@intel.com

[commit-bot: I haz the power, 2017-05-20 00:28:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-20 00:34:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495240095433220,
"parent_rev": "dfe378426b93e089fbcbf605284f842092d333ac", "commit_rev":
"1671f06ee7b3e7ba18d4a8171d3e0c64d95a44b0"}

[commit-bot: I haz the power, 2017-05-20 00:35:11]

Description was changed from

==========
Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689413
==========

to

==========
Convert //content/browser/webrtc 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 the Wake Lock Mojo
interface instead.

This CL converts clients of PowerSaveBlocker in //content/browser/webrtc

BUG=689413

Review-Url: https://codereview.chromium.org/2886603003
Cr-Commit-Position: refs/heads/master@{#473395}
Committed:
https://chromium.googlesource.com/chromium/src/+/1671f06ee7b3e7ba18d4a8171d3e...
==========

[commit-bot: I haz the power, 2017-05-20 00:35:12]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1671f06ee7b3e7ba18d4a8171d3e...