Generalize the API of WakeLockContext mojo interface.

Generalize the API of WakeLockContext mojo interface.

Generalize the API of WakeLockContext mojo interface, so client can pass
type, reason and description as they want to create the WakeLockService.

BUG=689410
Review-Url: https://codereview.chromium.org/2867303003
Cr-Commit-Position: refs/heads/master@{#470801}
Committed: https://chromium.googlesource.com/chromium/src/+/f435c1e84c2fb1cdfe7b68739cfd05ff8c49dee4

[ke.he, 2017-05-09 06:14:11]

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

[commit-bot: I haz the power, 2017-05-09 06:14:24]

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-09 06:15:52]

Description was changed from

==========
Generalize the API of WakeLockContext mojo interface.

1) Generalize the API of WakeLockContext mojo interface, so client can pass
   type, reason and description as they want to create the WakeLockService.
2) Add traits and typemap files for wake_lock_context.mojom.

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

to

==========
Generalize the API of WakeLockContext mojo interface.

1) Generalize the API of WakeLockContext mojo interface, so client can pass
   type, reason and description as they want to create the WakeLockService.
2) Add traits and typemap files for wake_lock_context.mojom.

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

[ke.he, 2017-05-09 06:16:27]

Description was changed from

==========
Generalize the API of WakeLockContext mojo interface.

1) Generalize the API of WakeLockContext mojo interface, so client can pass
   type, reason and description as they want to create the WakeLockService.
2) Add traits and typemap files for wake_lock_context.mojom.

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

to

==========
Generalize the API of WakeLockContext mojo interface.

1) Generalize the API of WakeLockContext mojo interface, so client can pass
   type, reason and description as they want to create the WakeLockService.
2) Add traits and typemap files for wake_lock_context.mojom.

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

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

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

[commit-bot: I haz the power, 2017-05-09 09:11:07]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-05-09 09:14:47]

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

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

Hi, Colin, I split out the //device stuff.
PTAL. Thanks!

[blundell, 2017-05-09 15:53:24]

Thanks!

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/public/int...
File device/wake_lock/public/interfaces/wake_lock_context.mojom (right):

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/public/int...
device/wake_lock/public/interfaces/wake_lock_context.mojom:9: // TODO(heke):
Switch PowerSaveBlocker to use WakeLockType and WakeLockReason
I think that we could just do the mapping from mojom type to PowerSaveBlocker
type in wake_lock_service_impl.cc with this TODO there, and incrementally
convert clients of PowerSaveBlocker to pass the mojom types as we convert them
to be clients of WakeLock. That way once there are no more clients of
PowerSaveBlocker we can just convert PowerSaveBlocker itself to use the mojom
types and eliminate the mapping from wake_lock_service_impl.cc: We don't then
need to go back and convert all clients in a second pass. WDYT?

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/wake_lock_...
File device/wake_lock/wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/wake_lock_...
device/wake_lock/wake_lock_service_impl.cc:95: gfx::NativeView native_view =
native_view_getter_.Run(context_id_);
Hmm. This code currently only executes at some usage sites of PowerSaveBlocker.
However, with this change, we would be executing it unconditionally. What is the
right guard to avoid this change in behavior? Is it that this code should only
execute if the type is PreventDisplaySleep?

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

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

[commit-bot: I haz the power, 2017-05-10 04:34: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-10 06:15:45]

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

[commit-bot: I haz the power, 2017-05-10 06:15:46]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-05-10 07:00:20]

Colin, Thanks very much!
PTAL.

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/public/int...
File device/wake_lock/public/interfaces/wake_lock_context.mojom (right):

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/public/int...
device/wake_lock/public/interfaces/wake_lock_context.mojom:9: // TODO(heke):
Switch PowerSaveBlocker to use WakeLockType and WakeLockReason
On 2017/05/09 15:53:24, blundell wrote:
> I think that we could just do the mapping from mojom type to PowerSaveBlocker
> type in wake_lock_service_impl.cc with this TODO there, and incrementally
> convert clients of PowerSaveBlocker to pass the mojom types as we convert them
> to be clients of WakeLock. That way once there are no more clients of
> PowerSaveBlocker we can just convert PowerSaveBlocker itself to use the mojom
> types and eliminate the mapping from wake_lock_service_impl.cc: We don't then
> need to go back and convert all clients in a second pass. WDYT?

Good idea, in this way we don't need the typemap either. I'll let the
MediaWebContentsObsever pass the mojom types first.

Done.

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/wake_lock_...
File device/wake_lock/wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2867303003/diff/1/device/wake_lock/wake_lock_...
device/wake_lock/wake_lock_service_impl.cc:95: gfx::NativeView native_view =
native_view_getter_.Run(context_id_);
On 2017/05/09 15:53:24, blundell wrote:
> Hmm. This code currently only executes at some usage sites of
PowerSaveBlocker.
> However, with this change, we would be executing it unconditionally. What is
the
> right guard to avoid this change in behavior? Is it that this code should only
> execute if the type is PreventDisplaySleep?

Oh, Thanks pointing out this! I'm sorry.
I found there are two clients executing this InitDisplaySleepBlocker(), one is
MediaWebContentsObserver::video_power_save_blocker_, another is
RenderFrameDevToolsAgentHost::power_save_blocker_. Both of them are requesting
|kPowerSaveBlockPreventDisplaySleep| type. So yes this code should only execute
for PreventDisplaySleep type.

Done.

[ke.he, 2017-05-10 09:45:17]

By the way, the work of "adding interface GetUnassociatedWakeLockContext()" and
"converting the first non-webcontents-context client" are almost locally
finished.

Should I split them in 2 CLs or merge them together? Thanks:)

[blundell, 2017-05-10 10:05:11]

lgtm, thanks!

https://codereview.chromium.org/2867303003/diff/20001/device/wake_lock/wake_l...
File device/wake_lock/wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2867303003/diff/20001/device/wake_lock/wake_l...
device/wake_lock/wake_lock_service_impl.cc:99:
static_cast<PowerSaveBlocker::PowerSaveBlockerType>(type_);
hmm, I think it would be safer to explicitly do the conversion case-by-case even
thought that's slightly tedious.

[ke.he, 2017-05-10 12:49:06]

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

[commit-bot: I haz the power, 2017-05-10 12:49:36]

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-10 13:12:03]

static_cast was removed. thanks:)

https://codereview.chromium.org/2867303003/diff/20001/device/wake_lock/wake_l...
File device/wake_lock/wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2867303003/diff/20001/device/wake_lock/wake_l...
device/wake_lock/wake_lock_service_impl.cc:99:
static_cast<PowerSaveBlocker::PowerSaveBlockerType>(type_);
On 2017/05/10 10:05:11, blundell wrote:
> hmm, I think it would be safer to explicitly do the conversion case-by-case
even
> thought that's slightly tedious.

Done.

[commit-bot: I haz the power, 2017-05-10 14:03:51]

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

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

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-05-10 14:21:13]

Colin, PTAL again as I removed static_cast, thanks.

[ke.he, 2017-05-10 14:25:32]

ke.he@intel.com changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[ke.he, 2017-05-10 14:25:33]

Jam@, PTAL on the //content. Tom, PTAL on the mojom.
thanks all!

[blundell, 2017-05-10 14:43:47]

still lgtm, thanks!

https://codereview.chromium.org/2867303003/diff/40001/device/wake_lock/wake_l...
File device/wake_lock/wake_lock_service_impl.cc (right):

https://codereview.chromium.org/2867303003/diff/40001/device/wake_lock/wake_l...
device/wake_lock/wake_lock_service_impl.cc:128: GetType(type_),
GetReason(reason_), *description_, main_task_runner_,
really tiny nit: Maybe ToPowerSaveBlockerType() and ToPowerSaveBlockerReason()?

[jam, 2017-05-10 16:13:56]

lgtm

[Tom Sepez, 2017-05-10 16:36:10]

lgtm

[ke.he, 2017-05-10 23:00:33]

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

[commit-bot: I haz the power, 2017-05-10 23:01:46]

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-11 00:07:41]

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

[commit-bot: I haz the power, 2017-05-11 00:07:43]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ke.he, 2017-05-11 01:53:37]

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

[commit-bot: I haz the power, 2017-05-11 01:54:35]

CQ is trying da patch.

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

[ke.he, 2017-05-11 02:36:28]

Description was changed from

==========
Generalize the API of WakeLockContext mojo interface.

1) Generalize the API of WakeLockContext mojo interface, so client can pass
   type, reason and description as they want to create the WakeLockService.
2) Add traits and typemap files for wake_lock_context.mojom.

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

to

==========
Generalize the API of WakeLockContext mojo interface.

Generalize the API of WakeLockContext mojo interface, so client can pass
type, reason and description as they want to create the WakeLockService.

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

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

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

[ke.he, 2017-05-11 03:58:17]

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

[commit-bot: I haz the power, 2017-05-11 04:00:03]

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-11 04:09:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494475097144470,
"parent_rev": "081e4c81bb3c3a10b7e6921954dbe1e234e469c1", "commit_rev":
"f435c1e84c2fb1cdfe7b68739cfd05ff8c49dee4"}

[commit-bot: I haz the power, 2017-05-11 04:09:32]

Description was changed from

==========
Generalize the API of WakeLockContext mojo interface.

Generalize the API of WakeLockContext mojo interface, so client can pass
type, reason and description as they want to create the WakeLockService.

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

to

==========
Generalize the API of WakeLockContext mojo interface.

Generalize the API of WakeLockContext mojo interface, so client can pass
type, reason and description as they want to create the WakeLockService.

BUG=689410

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

[commit-bot: I haz the power, 2017-05-11 04:09:34]

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