Convert CastTransportHostFilter to be client of wake lock service.

Convert CastTransportHostFilter to be client of wake lock 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.

BUG=689415
Review-Url: https://codereview.chromium.org/2891293002
Cr-Commit-Position: refs/heads/master@{#476172}
Committed: https://chromium.googlesource.com/chromium/src/+/b4fe4703234db95a75f7b94d1e97044d38488156

[ke.he, 2017-05-19 07:24:46]

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

[commit-bot: I haz the power, 2017-05-19 07:25:35]

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-19 09:24:27]

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

[commit-bot: I haz the power, 2017-05-19 09:24:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ke.he, 2017-05-19 10:03:31]

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

[commit-bot: I haz the power, 2017-05-19 10:03:52]

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 11:05:15]

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

[ke.he, 2017-05-19 11:05:16]

Hi, Sergey, PTAL, thanks.

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
chrome/browser/media/cast_transport_host_filter.cc:398:
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
It must runs on IO thread.

[commit-bot: I haz the power, 2017-05-19 11:51:47]

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

[commit-bot: I haz the power, 2017-05-19 11:51:47]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-05-19 19:34:11]

sergeyu@chromium.org changed reviewers:
+ hubbe@chromium.org

[Sergey Ulanov, 2017-05-19 19:34:11]

Looks mostly good, but I'm not familiar with this file. +hubbe@ for the second
pair for eyes.

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
chrome/browser/media/cast_transport_host_filter.cc:159: has_wake_lock_ = true;
I don't think we need this flag. RequestWakeLock() has no effect if the lock is
active, so you could just always call it here. Also you could use
!id_map.IsEmpty() to detect if the wake lock is already active.

[ke.he, 2017-05-24 05:43:08]

Hi, Sergey, Thanks! Sorry to reply your comments lately.

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
chrome/browser/media/cast_transport_host_filter.cc:159: has_wake_lock_ = true;
On 2017/05/19 19:34:11, Sergey Ulanov wrote:
> I don't think we need this flag. RequestWakeLock() has no effect if the lock
is
> active, so you could just always call it here. Also you could use
> !id_map.IsEmpty() to detect if the wake lock is already active.

Although sending extra RequestWakeLock() would be handled well in remote service
side, I still feel it is better to make sure not to send extra
RequestWakeLock(). And the "DVLOG(1)" would be output multiple times without the
flag. WDYT?

I'm OK to go with removing the flag if you think above operations are
acceptable.

[Sergey Ulanov, 2017-05-24 19:17:48]

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
chrome/browser/media/cast_transport_host_filter.cc:159: has_wake_lock_ = true;
On 2017/05/24 05:43:08, ke.he wrote:
> On 2017/05/19 19:34:11, Sergey Ulanov wrote:
> > I don't think we need this flag. RequestWakeLock() has no effect if the lock
> is
> > active, so you could just always call it here. Also you could use
> > !id_map.IsEmpty() to detect if the wake lock is already active.
> 
> Although sending extra RequestWakeLock() would be handled well in remote
service
> side, I still feel it is better to make sure not to send extra
> RequestWakeLock(). And the "DVLOG(1)" would be output multiple times without
the
> flag. WDYT?
> 
> I'm OK to go with removing the flag if you think above operations are
> acceptable.
> 

You can still avoid calling RequestWakeLock() again without the flag.

if (id_map_.IsEmpty()) {
  GetWakeLockService()->RequestWakeLock();
}


This will give you the same behavior as with the flag: Request wake_lock when a
first stream is started and release it when the last stream is stopped.

[ke.he, 2017-05-25 07:21:35]

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

[commit-bot: I haz the power, 2017-05-25 07:21:56]

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-25 07:24:12]

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

[commit-bot: I haz the power, 2017-05-25 07:24:13]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[ke.he, 2017-05-25 09:48:17]

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

[commit-bot: I haz the power, 2017-05-25 09:48:31]

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-25 10:50:13]

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

[commit-bot: I haz the power, 2017-05-25 10:50:13]

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-25 15:04:19]

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

[commit-bot: I haz the power, 2017-05-25 15:04:55]

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-25 16:40:01]

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

[commit-bot: I haz the power, 2017-05-25 16:40:02]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-05-26 02:55:51]

Thanks!  PTAL.

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/1/chrome/browser/media/cast_t...
chrome/browser/media/cast_transport_host_filter.cc:159: has_wake_lock_ = true;
On 2017/05/24 19:17:48, Sergey Ulanov wrote:
> On 2017/05/24 05:43:08, ke.he wrote:
> > On 2017/05/19 19:34:11, Sergey Ulanov wrote:
> > > I don't think we need this flag. RequestWakeLock() has no effect if the
lock
> > is
> > > active, so you could just always call it here. Also you could use
> > > !id_map.IsEmpty() to detect if the wake lock is already active.
> > 
> > Although sending extra RequestWakeLock() would be handled well in remote
> service
> > side, I still feel it is better to make sure not to send extra
> > RequestWakeLock(). And the "DVLOG(1)" would be output multiple times without
> the
> > flag. WDYT?
> > 
> > I'm OK to go with removing the flag if you think above operations are
> > acceptable.
> > 
> 
> You can still avoid calling RequestWakeLock() again without the flag.
> 
> if (id_map_.IsEmpty()) {
>   GetWakeLockService()->RequestWakeLock();
> }
> 
> 
> This will give you the same behavior as with the flag: Request wake_lock when
a
> first stream is started and release it when the last stream is stopped.

Done. Thanks!

[Sergey Ulanov, 2017-05-26 05:39:03]

lgtm

[ke.he, 2017-05-26 05:57:03]

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

[commit-bot: I haz the power, 2017-05-26 05:57:38]

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-26 06:07:32]

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

[commit-bot: I haz the power, 2017-05-26 06:07:33]

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

[ke.he, 2017-05-27 03:23:38]

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

[ke.he, 2017-05-27 03:23:39]

Hi, Ken, PTAL on //chrome/browser/media/DEPS. Thanks!

[Ken Rockot(use gerrit already), 2017-05-27 05:23:32]

DEPS LGTM for device-related additions. Will need a content owner too

[Ken Rockot(use gerrit already), 2017-05-27 05:24:14]

Wait, not LGTM. I didn't pay close enough attention. No, you cannot include
stuff from content/browser from within chrome/. No exceptions.

[Ken Rockot(use gerrit already), 2017-05-27 05:27:26]

lgtm

https://codereview.chromium.org/2891293002/diff/40001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/40001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:397:
service_manager::Connector* connector =
You don't need to use ServiceManagerContext for this (and you can't, anyway).

A Connector can always be cloned, and can always bind other ConnectorRequests to
the same impl.

What you should do is create a new Connector here on the IO thread:

  service_manager::mojom::ConnectorRequest connector_request;
  auto connector = service_manager::Connector::Create(&connector_request);

and then pass |connector_request| to the main thread where you can:

  content::ServiceManagerConnection::GetForProcess()
      ->GetConnector()
      ->BindConnectorRequest(std::move(connector_request));

[Ken Rockot(use gerrit already), 2017-05-27 05:28:09]

On 2017/05/27 at 05:27:26, Ken Rockot(use gerrit already) wrote:
> lgtm
> 
>
https://codereview.chromium.org/2891293002/diff/40001/chrome/browser/media/ca...
> File chrome/browser/media/cast_transport_host_filter.cc (right):
> 
>
https://codereview.chromium.org/2891293002/diff/40001/chrome/browser/media/ca...
> chrome/browser/media/cast_transport_host_filter.cc:397:
service_manager::Connector* connector =
> You don't need to use ServiceManagerContext for this (and you can't, anyway).
> 
> A Connector can always be cloned, and can always bind other ConnectorRequests
to the same impl.
> 
> What you should do is create a new Connector here on the IO thread:
> 
>   service_manager::mojom::ConnectorRequest connector_request;
>   auto connector = service_manager::Connector::Create(&connector_request);
> 
> and then pass |connector_request| to the main thread where you can:
> 
>   content::ServiceManagerConnection::GetForProcess()
>       ->GetConnector()
>       ->BindConnectorRequest(std::move(connector_request));

(that LG was not intentional... but i'll leave it alone. please make the change
above :))

[ke.he, 2017-05-31 06:35:03]

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

[commit-bot: I haz the power, 2017-05-31 06:35:26]

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-31 07:40:29]

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

[commit-bot: I haz the power, 2017-05-31 07:40:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ke.he, 2017-05-31 08:26:15]

Patchset #5 (id:80001) has been deleted

[ke.he, 2017-05-31 08:31:01]

Patchset #5 (id:100001) has been deleted

[ke.he, 2017-05-31 08:37:25]

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

[commit-bot: I haz the power, 2017-05-31 08:37: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-31 09:49:44]

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

[commit-bot: I haz the power, 2017-05-31 09:49:45]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-05-31 11:12:03]

Ken and Sergey, Thanks very much!
PTAL again:)

https://codereview.chromium.org/2891293002/diff/40001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/2891293002/diff/40001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:397:
service_manager::Connector* connector =
On 2017/05/27 05:27:26, Ken Rockot(use gerrit already) wrote:
> You don't need to use ServiceManagerContext for this (and you can't, anyway).
> 
> A Connector can always be cloned, and can always bind other ConnectorRequests
to
> the same impl.
> 
> What you should do is create a new Connector here on the IO thread:
> 
>   service_manager::mojom::ConnectorRequest connector_request;
>   auto connector = service_manager::Connector::Create(&connector_request);
> 
> and then pass |connector_request| to the main thread where you can:
> 
>   content::ServiceManagerConnection::GetForProcess()
>       ->GetConnector()
>       ->BindConnectorRequest(std::move(connector_request));

Done. Thanks!

[Ken Rockot(use gerrit already), 2017-05-31 11:50:50]

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/D...
chrome/browser/media/DEPS:2:
"+content/public/common/service_manager_connection.h",
this is not necessary - src/chrome already has full access to
content/public/common

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:18: #include
"content/public/common/service_manager_connection.h"
Please move this include to the cc file, ServiceManagerConnection's definition
is never used by this header

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:34: static void
BindConnectorRequest(
Please remove this from the class. It can be defined entirely within an
anonymous namespace in the cc

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:104: // While |id_map_| is
non-empty, request the WakeLockService to hold a wake
How about "While |id_map_| is is non-empty, we use |wake_lock_| to request and
hold a wake lock. If any wake lock is held upon destruction, it's implicitly
canceled when this object is destroyed."

IMHO no need to explain the implementation details of Mojo bindings or the
service here. It may be best to document the service behavior in the mojom
rather than at the site of each client's usage.

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:109:
device::mojom::WakeLockServicePtr wake_lock_service_;
super nit: can we please just call this |wake_lock_|?

I would like to someday rename WakeLockService to WakeLock, as we should rid
ourselves of the habit of calling interfaces "services"

[ke.he, 2017-05-31 12:41:57]

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

[commit-bot: I haz the power, 2017-05-31 12:42:08]

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-31 12:45:10]

Ken, Thanks very much. PTAL:)

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/DEPS
File chrome/browser/media/DEPS (right):

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/D...
chrome/browser/media/DEPS:2:
"+content/public/common/service_manager_connection.h",
On 2017/05/31 11:50:49, Ken Rockot(use gerrit already) wrote:
> this is not necessary - src/chrome already has full access to
> content/public/common

Done.

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:18: #include
"content/public/common/service_manager_connection.h"
On 2017/05/31 11:50:49, Ken Rockot(use gerrit already) wrote:
> Please move this include to the cc file, ServiceManagerConnection's definition
> is never used by this header

Done.

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:34: static void
BindConnectorRequest(
On 2017/05/31 11:50:49, Ken Rockot(use gerrit already) wrote:
> Please remove this from the class. It can be defined entirely within an
> anonymous namespace in the cc

Done.

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:104: // While |id_map_| is
non-empty, request the WakeLockService to hold a wake
On 2017/05/31 11:50:49, Ken Rockot(use gerrit already) wrote:
> How about "While |id_map_| is is non-empty, we use |wake_lock_| to request and
> hold a wake lock. If any wake lock is held upon destruction, it's implicitly
> canceled when this object is destroyed."
> 
> IMHO no need to explain the implementation details of Mojo bindings or the
> service here. It may be best to document the service behavior in the mojom
> rather than at the site of each client's usage.

Done.

https://codereview.chromium.org/2891293002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:109:
device::mojom::WakeLockServicePtr wake_lock_service_;
On 2017/05/31 11:50:49, Ken Rockot(use gerrit already) wrote:
> super nit: can we please just call this |wake_lock_|?

Done.
 
> I would like to someday rename WakeLockService to WakeLock, as we should rid
> ourselves of the habit of calling interfaces "services"

Yeah, I think we can do the rename when moving it to //services/device/, after
all clients of wakelock be converted.

[commit-bot: I haz the power, 2017-05-31 14:17:17]

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

[commit-bot: I haz the power, 2017-05-31 14:17:18]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-05-31 14:57:29]

lgtm

[ke.he, 2017-06-01 03:22:54]

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

[ke.he, 2017-06-01 03:22:54]

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/2891293002/#ps140001
(title: "Addressed Ken's review comments.")

[commit-bot: I haz the power, 2017-06-01 03:23:13]

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-06-01 03:28:00]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496287374002320,
"parent_rev": "1183b68ff2c6689a609c9abc9757b94e3a8953bb", "commit_rev":
"b4fe4703234db95a75f7b94d1e97044d38488156"}

[commit-bot: I haz the power, 2017-06-01 03:28:11]

Description was changed from

==========
Convert CastTransportHostFilter to be client of wake lock 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.

BUG=689415
==========

to

==========
Convert CastTransportHostFilter to be client of wake lock 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.

BUG=689415

Review-Url: https://codereview.chromium.org/2891293002
Cr-Commit-Position: refs/heads/master@{#476172}
Committed:
https://chromium.googlesource.com/chromium/src/+/b4fe4703234db95a75f7b94d1e97...
==========

[commit-bot: I haz the power, 2017-06-01 03:28:13]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/b4fe4703234db95a75f7b94d1e97...