Device Service: Decouple Wake Lock from //content

Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).
(2) Adds a WakeLockContext Mojo interface. This interface can be used to
forward requests to bind wake locks from untrusted clients that are made within
the same (trusted-client specific) context.
(3) Adds a WakeLockContextProvider Mojo interface. This interface allows
for getting a WakeLockContext that is associated with a specific context ID.
The WakeLockContext impl uses that context ID on Android to obtain the
NativeView for its context.

Note that the lifetime model of WakeLockContext is somewhat complex: It must
stay alive as long as either
(1) Its Mojo connection is still valid (as the client might make future
    GetWakeLock() calls) OR
(2) There are still live WakeLock instances that it has instantiated (since
    they call into it when they receive Mojo requests from *their* clients).

Consequently, WakeLockContext monitors the state of the connections described
in (1) and (2), dying only when *all* of those connections go away. As a final
subtlety, WakeLockServiceImpl calls back into WakeLockContext from its
destructor, which executes immediately *after* WakeLockContext receives the
callback that that WakeLockServiceImpl instance has experienced a connection
error. WakeLockContext thus posts a task to delete itself rather than deleting
itself synchronously.

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2734943003
Cr-Commit-Position: refs/heads/master@{#459719}
Committed: https://chromium.googlesource.com/chromium/src/+/e75a8f91ce3a7a09d0da15f1c1fc83a79a668b8e

[blundell, 2017-03-07 17:12:35]

Description was changed from

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds a WakeLockServiceContext Mojo interface. A trusted client can
connect to this interface and forward requests to bind wake locks from
untrusted clients that are made within the same (trusted-client specific)
context.
(2) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).

BUG=689403
==========

to

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds a WakeLockServiceContext Mojo interface. A trusted client can
connect to this interface and forward requests to bind wake locks from
untrusted clients that are made within the same (trusted-client specific)
context.
(2) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).

BUG=689403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[blundell, 2017-03-07 17:15:38]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 17:16: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-03-07 17:53:44]

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

[commit-bot: I haz the power, 2017-03-07 17:53:45]

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

[blundell, 2017-03-07 18:34:20]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 18:35:12]

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-03-07 20:29:45]

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

[commit-bot: I haz the power, 2017-03-07 20:29:46]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-07 20:41:14]

blundell@chromium.org changed reviewers:
+ rockot@chromium.org

[blundell, 2017-03-07 20:41:15]

As I started putting this together, I realized that the natural context
granularity for WakeLock is actually per-tab, rather than per-frame (for
example, browser process clients of the underlying PowerSaveBlocker are always
within the context of a WebContents, but they're not always within the context
of a specific RenderFrameHost).

This is not 100% polished (see the comment I left), but it's sufficient to
discuss the high-level approach.

https://codereview.chromium.org/2734943003/diff/20001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/2734943003/diff/20001/content/public/browser/...
content/public/browser/web_contents.h:192: CONTENT_EXPORT static WebContents*
FromId(int id);
If we stick with this approach, I'll modify this to avoid adding this into the
WebContents interface (where it presumably would not be at all desired) and
instead will make a WakeLockServiceContextHost class that is owned by
WebContents and does the mapping.

[Ken Rockot(use gerrit already), 2017-03-10 16:23:25]

This pattern makes me super sad, but it highlights a real problem we need to
solve; so I would be OK with landing this pending cleanup, WebContents changes,
etc

https://codereview.chromium.org/2734943003/diff/20001/device/wake_lock/public...
File device/wake_lock/public/interfaces/wake_lock_service_context.mojom (right):

https://codereview.chromium.org/2734943003/diff/20001/device/wake_lock/public...
device/wake_lock/public/interfaces/wake_lock_service_context.mojom:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2734943003/diff/20001/device/wake_lock/public...
device/wake_lock/public/interfaces/wake_lock_service_context.mojom:9: interface
WakeLockServiceContext {
If we're going to do this, as discussed offline I'd add a separate acquisition
interface so that a WakeLockContext pipe is always bound to a context ID:

interface WakeLockContextProvider {
  GetContext(int32 context_id, WakeLockContext& context);
}

interface WakeLockContext {
  GetWakeLock(WakeLock& wake_lock);

  HasWakeLockForTests() => (bool result);  // Also... Maybe [Sync]?
};

** I have omitted "Service" from all these names because we should also do that.

https://codereview.chromium.org/2734943003/diff/20001/services/device/manifes...
File services/device/manifest.json (right):

https://codereview.chromium.org/2734943003/diff/20001/services/device/manifes...
services/device/manifest.json:10: "device:wake_lock_service_context": [
"device::mojom::WakeLockServiceContext" ]
nit: We really don't need 1:1 correspondence between capabilities and
interfaces. This could just be the "device:wake_lock" capability.

[blundell, 2017-03-14 15:05:12]

Description was changed from

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds a WakeLockServiceContext Mojo interface. A trusted client can
connect to this interface and forward requests to bind wake locks from
untrusted clients that are made within the same (trusted-client specific)
context.
(2) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).

BUG=689403
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds a WakeLockServiceContext Mojo interface. A trusted client can
connect to this interface and forward requests to bind wake locks from
untrusted clients that are made within the same (trusted-client specific)
context.
(2) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[blundell, 2017-03-14 17:16:26]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 17:16:46]

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-03-14 17:19:18]

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

[commit-bot: I haz the power, 2017-03-14 17:19:19]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[blundell, 2017-03-14 19:49:55]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 19:50:27]

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-03-14 20:44:01]

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

[commit-bot: I haz the power, 2017-03-14 20:44:02]

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_...)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[ke.he, 2017-03-15 08:09:37]

ke.he@intel.com changed reviewers:
+ ke.he@intel.com

[ke.he, 2017-03-15 08:09:38]

https://codereview.chromium.org/2734943003/diff/100001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.cc (right):

https://codereview.chromium.org/2734943003/diff/100001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:32: :
id_(g_unique_id.GetNext()), web_contents_(web_contents) {
Glad to see the ID is moved out of webcontents. So for those clients of
PowerSaveBlocker who are not in webcontents context, they can create their own
WakeLockContextHost, just pass a nullptr to parameter |web_contents|, right?

https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:68:
device::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
Currently it only creates one PowerSaveBlocker instance per
WakeLockServiceContext, the default type is "kPowerSaveBlockPreventDisplaySleep"
and cannot be changed.

To serve all clients in browser process, I plan to create 2 instances for each
type(kPowerSaveBlockPreventDisplaySleep and kPowerSaveBlockPreventAppSuspension)
in lazy way per WakeLockServiceContext.

I found the "Reason" parameter only meaningful for ChromeOS platform. I think it
is safe that Clients who want same "Type" but different "Reason" to share one
PowerSaveBlocker instance.

[ke.he, 2017-03-15 09:33:42]

https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:68:
device::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
See below link where it creates 2 PowerSaveBlocker instances separately:
https://cs.chromium.org/chromium/src/content/browser/media/media_web_contents...

[blundell, 2017-03-15 11:06:11]

Hi Ke He,

Thanks for the comments! I think that your reasoning broadly makes sense, but
we'll likely iterate as we zero in on the details once this lands. BTW, I'm
going to have to delete the patchset you commented on in order to condense the
patchsets and enable easy review :\.

https://codereview.chromium.org/2734943003/diff/20001/device/wake_lock/public...
File device/wake_lock/public/interfaces/wake_lock_service_context.mojom (right):

https://codereview.chromium.org/2734943003/diff/20001/device/wake_lock/public...
device/wake_lock/public/interfaces/wake_lock_service_context.mojom:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2017/03/10 16:23:25, Ken Rockot wrote:
> 2017

Done.

https://codereview.chromium.org/2734943003/diff/20001/device/wake_lock/public...
device/wake_lock/public/interfaces/wake_lock_service_context.mojom:9: interface
WakeLockServiceContext {
On 2017/03/10 16:23:25, Ken Rockot wrote:
> If we're going to do this, as discussed offline I'd add a separate acquisition
> interface so that a WakeLockContext pipe is always bound to a context ID:
> 
> interface WakeLockContextProvider {
>   GetContext(int32 context_id, WakeLockContext& context);
> }

Done.

> 
> interface WakeLockContext {
>   GetWakeLock(WakeLock& wake_lock);
> 
>   HasWakeLockForTests() => (bool result);  // Also... Maybe [Sync]?
> };

I initially made it Sync, but was scared off by all the comments here:
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/sync_call_restr....
As it works fine async, there didn't seem to be a "very compelling reason" here.

> 
> ** I have omitted "Service" from all these names because we should also do
that.

I did that for the new interface. I didn't do it for the existing ones to avoid
adding churn to this CL; instead I filed crbug.com/701414 to follow up once the
dust settles here.

https://codereview.chromium.org/2734943003/diff/20001/services/device/manifes...
File services/device/manifest.json (right):

https://codereview.chromium.org/2734943003/diff/20001/services/device/manifes...
services/device/manifest.json:10: "device:wake_lock_service_context": [
"device::mojom::WakeLockServiceContext" ]
On 2017/03/10 16:23:25, Ken Rockot wrote:
> nit: We really don't need 1:1 correspondence between capabilities and
> interfaces. This could just be the "device:wake_lock" capability.

Done.

https://codereview.chromium.org/2734943003/diff/100001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.cc (right):

https://codereview.chromium.org/2734943003/diff/100001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:32: :
id_(g_unique_id.GetNext()), web_contents_(web_contents) {
On 2017/03/15 08:09:38, ke.he wrote:
> Glad to see the ID is moved out of webcontents. So for those clients of
> PowerSaveBlocker who are not in webcontents context, they can create their own
> WakeLockContextHost, just pass a nullptr to parameter |web_contents|, right? 

We'll have to figure out the right design. It might make sense to different
entrypoint at the mojom level.

https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/100001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:68:
device::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
On 2017/03/15 08:09:38, ke.he wrote:
> Currently it only creates one PowerSaveBlocker instance per
> WakeLockServiceContext, the default type is
"kPowerSaveBlockPreventDisplaySleep"
> and cannot be changed.
> 
> To serve all clients in browser process, I plan to create 2 instances for each
> type(kPowerSaveBlockPreventDisplaySleep and
kPowerSaveBlockPreventAppSuspension)
> in lazy way per WakeLockServiceContext.

I think this makes sense. We'll be able to analyze in more detail when we get to
the point of code there.

> 
> I found the "Reason" parameter only meaningful for ChromeOS platform. I think
it
> is safe that Clients who want same "Type" but different "Reason" to share one
> PowerSaveBlocker instance.

Interesting. We'll have to hash this out when we can analyze in detail.

[blundell, 2017-03-15 11:10:03]

Patchset #4 (id:60001) has been deleted

[blundell, 2017-03-15 11:10:16]

Patchset #4 (id:80001) has been deleted

[blundell, 2017-03-15 11:10:26]

Patchset #4 (id:100001) has been deleted

[blundell, 2017-03-15 11:12:46]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 11:13:00]

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

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

[commit-bot: I haz the power, 2017-03-15 11:15:39]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[blundell, 2017-03-15 11:15:47]

Description was changed from

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds a WakeLockServiceContext Mojo interface. A trusted client can
connect to this interface and forward requests to bind wake locks from
untrusted clients that are made within the same (trusted-client specific)
context.
(2) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).
(2) Adds a WakeLockContext Mojo interface. This interface can be used to
forward requests to bind wake locks from untrusted clients that are made within
the same (trusted-client specific) context.
(3) Adds a WakeLockContextProvider Mojo interface. This interface allows
for getting a WakeLockContext that is associated with a specific context ID.
The WakeLockContext impl uses that context ID on Android to obtain the
NativeView for its context.

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[blundell, 2017-03-15 11:29:42]

Patchset #4 (id:120001) has been deleted

[blundell, 2017-03-15 11:53:33]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 11:53:47]

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-03-15 13:14:45]

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

[commit-bot: I haz the power, 2017-03-15 13:14:45]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-15 15:25:10]

Hi Ken,

This is ready for re-review now. My responses to your comments are on the
earlier mail I sent responding to Ke He (I forgot that that would of course send
out my responses to *all* comments).

I unfortunately got a rebase mixed in with the response to the review in an
inextricable way :P. Apologies for that.

[blundell, 2017-03-15 15:26:30]

blundell@chromium.org changed reviewers:
+ dcheng@chromium.org

[blundell, 2017-03-15 15:26:31]

Daniel, could you do a review of //device/wake_lock/public/interfaces? I'm
interested to get your thoughts (you'll probably have to look over the whole CL
to get the necessary context).

[Ken Rockot(use gerrit already), 2017-03-16 17:48:40]

Looks good (well, as good as this general approach *can* look) overall.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
File device/wake_lock/public/interfaces/README.mojom (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
device/wake_lock/public/interfaces/README.mojom:1: Provides the ability to block
the device / display from sleeping.
README.mojom?

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:105:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
I haven't understood why this is necessary, i.e. why we must outlive
WakeLockServiceImpl. Can you elaborate? Would be really nice if we could at
least use StrongBindingSet.

[dcheng, 2017-03-17 06:55:26]

It seems reasonable overall. I am a little sad that we need to plumb through a
context ID at all though, but I guess this is unavoidable?

(I ask because where possible, I've encouraged people to use message pipes for
identity rather than integer IDs)

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_browsertest.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_browsertest.cc:76: base::Closure
quit_closure = run_loop.QuitClosure();
Nit: this appears unused.

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:21:
base::LazyInstance<std::map<int, WakeLockContextHost*>>::DestructorAtExit
Does this need to be DestructorAtExit?

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:36: if
(ServiceManagerConnection::GetForProcess()) {
Out of curiosity, why is this check needed at all?

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:48:
g_id_to_context_host.Get()[id_] = nullptr;
Nit: erase()?

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.h (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.h:21:
WakeLockContextHost(WebContents* web_contents);
Nit: explicit

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
File device/wake_lock/public/interfaces/README.mojom (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
device/wake_lock/public/interfaces/README.mojom:1: Provides the ability to block
the device / display from sleeping.
Nit: should this be README.md?

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_context_provider.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_context_provider.cc:17: file_task_runner,
native_view_getter),
Nit: std::move (and #include <utility>)

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_context_provider.cc:24: :
file_task_runner_(file_task_runner),
Nit: std::move

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_context_provider.cc:32: new
WakeLockServiceContext(std::move(request), context_id, file_task_runner_,
May be worth a comment here (or in the class header documentation) to briefly
explain the lifetime model of this object. For example, it seems like if
GetWakeLock is never called, this object will leak.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:22: WakeLockContextCallback
native_view_getter)
Nit: either move or pass by const ref (pass by const ref is the general
convention atm, but there's a vague plan to migrate most things to OnceCallback,
which will imply the former)

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:24:
file_task_runner_(file_task_runner),
Nit: std::move

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:97: DCHECK(num_connections_ > 0);
Nit: DCHECK_GT

https://codereview.chromium.org/2734943003/diff/140001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/services/device/device...
services/device/device_service.cc:31: WakeLockContextCallback
wake_lock_context_callback) {
Nit: std::move() the callback if you want to pass by value. Alternatively,
follow general convention and pass by const ref.

https://codereview.chromium.org/2734943003/diff/140001/services/device/device...
services/device/device_service.cc:38: std::move(io_task_runner),
Nit: #include <utility> for std::move

[blundell, 2017-03-17 10:02:00]

Description was changed from

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).
(2) Adds a WakeLockContext Mojo interface. This interface can be used to
forward requests to bind wake locks from untrusted clients that are made within
the same (trusted-client specific) context.
(3) Adds a WakeLockContextProvider Mojo interface. This interface allows
for getting a WakeLockContext that is associated with a specific context ID.
The WakeLockContext impl uses that context ID on Android to obtain the
NativeView for its context.

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).
(2) Adds a WakeLockContext Mojo interface. This interface can be used to
forward requests to bind wake locks from untrusted clients that are made within
the same (trusted-client specific) context.
(3) Adds a WakeLockContextProvider Mojo interface. This interface allows
for getting a WakeLockContext that is associated with a specific context ID.
The WakeLockContext impl uses that context ID on Android to obtain the
NativeView for its context.

Note that the lifetime model of WakeLockContext is somewhat complex: It must
stay alive as long as either
(1) Its Mojo connection is still valid (as the client might make future
    GetWakeLock() calls) OR
(2) There are still live WakeLock instances that it has instantiated (since
    they call into it when they receive Mojo requests from *their* clients).

Consequently, WakeLockContext monitors the state of the connections described
in (1) and (2), dying only when *all* of those connections go away. As a final
subtlety, WakeLockServiceImpl calls back into WakeLockContext from its
destructor, which executes immediately *after* WakeLockContext receives the
callback that that WakeLockServiceImpl instance has experienced a connection
error. WakeLockContext thus posts a task to delete itself rather than deleting
itself synchronously.

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[blundell, 2017-03-17 11:03:31]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 11:03:51]

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-03-17 11:06:29]

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

[commit-bot: I haz the power, 2017-03-17 11:06:30]

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

[blundell, 2017-03-17 11:08:49]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 11:09:12]

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-03-17 12:24:18]

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

[commit-bot: I haz the power, 2017-03-17 12:24:19]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-17 12:28:23]

Thanks for the reviews!

Daniel: I agree with the philosophy of avoiding "context IDs" and the like
whenever possible - the scoping of the pipe should *be* the context. In this
case, it seems unavoidable as we need a token that the Wake Lock impl can pass
back to //content in C++ to identify the corresponding //content-side class.

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_browsertest.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_browsertest.cc:76: base::Closure
quit_closure = run_loop.QuitClosure();
On 2017/03/17 06:55:25, dcheng wrote:
> Nit: this appears unused.

Done.

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:21:
base::LazyInstance<std::map<int, WakeLockContextHost*>>::DestructorAtExit
On 2017/03/17 06:55:25, dcheng wrote:
> Does this need to be DestructorAtExit?

I cargo-culted from here:
https://cs.chromium.org/chromium/src/content/browser/browser_context.cc?dr&q=...

Changed to Leaky, as I can't think of a reason the destructor needs to run here.

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:36: if
(ServiceManagerConnection::GetForProcess()) {
On 2017/03/17 06:55:25, dcheng wrote:
> Out of curiosity, why is this check needed at all?

In unittests the service manager connection is not initialized. Added a comment
to this effect.

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:48:
g_id_to_context_host.Get()[id_] = nullptr;
On 2017/03/17 06:55:25, dcheng wrote:
> Nit: erase()?

Done.

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.h (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.h:21:
WakeLockContextHost(WebContents* web_contents);
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
File device/wake_lock/public/interfaces/README.mojom (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
device/wake_lock/public/interfaces/README.mojom:1: Provides the ability to block
the device / display from sleeping.
On 2017/03/16 17:48:40, Ken Rockot wrote:
> README.mojom?

Who is README.mojom? My name is README.md.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/publi...
device/wake_lock/public/interfaces/README.mojom:1: Provides the ability to block
the device / display from sleeping.
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: should this be README.md?

Who is README.mojom? My name is README.md.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_context_provider.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_context_provider.cc:17: file_task_runner,
native_view_getter),
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: std::move (and #include <utility>)

Done.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_context_provider.cc:24: :
file_task_runner_(file_task_runner),
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: std::move

Done.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_context_provider.cc:32: new
WakeLockServiceContext(std::move(request), context_id, file_task_runner_,
On 2017/03/17 06:55:26, dcheng wrote:
> May be worth a comment here (or in the class header documentation) to briefly
> explain the lifetime model of this object. For example, it seems like if
> GetWakeLock is never called, this object will leak.

Done. (If GetWakeLock is never called, this object will die when the other side
drops its connection.)

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:22: WakeLockContextCallback
native_view_getter)
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: either move or pass by const ref (pass by const ref is the general
> convention atm, but there's a vague plan to migrate most things to
OnceCallback,
> which will imply the former)

Done.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:24:
file_task_runner_(file_task_runner),
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: std::move

Done.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:97: DCHECK(num_connections_ > 0);
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: DCHECK_GT

Done.

https://codereview.chromium.org/2734943003/diff/140001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:105:
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
On 2017/03/16 17:48:40, Ken Rockot wrote:
> I haven't understood why this is necessary, i.e. why we must outlive
> WakeLockServiceImpl. Can you elaborate?

WakeLockServiceImpl calls back into this class on its destructor, so either this
class needs to outlive WakeLockServiceImpl or the latter needs to have a
WeakPtr. I opted for the former because I like to avoid WeakPtr when possible.

Would be really nice if we could at
> least use StrongBindingSet.

Done.

https://codereview.chromium.org/2734943003/diff/140001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/services/device/device...
services/device/device_service.cc:31: WakeLockContextCallback
wake_lock_context_callback) {
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: std::move() the callback if you want to pass by value. Alternatively,
> follow general convention and pass by const ref.

Done.

https://codereview.chromium.org/2734943003/diff/140001/services/device/device...
services/device/device_service.cc:38: std::move(io_task_runner),
On 2017/03/17 06:55:26, dcheng wrote:
> Nit: #include <utility> for std::move

Done.

https://codereview.chromium.org/2734943003/diff/180001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/180001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:95:
context_binding_encountered_error_ = true;
Ken, AFAICT there isn't a way to check a Binding to see if it has encountered an
error. Is that correct? Is there a rationale for not exposing a
Binding::encountered_error() method?

[blundell, 2017-03-21 08:44:06]

Ken/Daniel: ping.

[dcheng, 2017-03-22 10:09:55]

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:36: if
(ServiceManagerConnection::GetForProcess()) {
On 2017/03/17 12:28:21, blundell wrote:
> On 2017/03/17 06:55:25, dcheng wrote:
> > Out of curiosity, why is this check needed at all?
> 
> In unittests the service manager connection is not initialized. Added a
comment
> to this effect.

Out of curiosity, is there any chance to add some thing like
TestBrowserThreadBundle so that it would Just Work in tests?

(I ask this because I've been surprised by the RenderThreadImpl::current() is
null in tests behavior in the past; it just so happened that the 'null in tests'
check written into non-test code ended up masking other issues)

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:18: mojom::WakeLockContextRequest
request,
Sorry, one more lifetime question...

Is it possible that |request| is already disconnected at this point? In that
case, would the connection error handler still be called?

[blundell, 2017-03-22 13:03:40]

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
File content/browser/wake_lock/wake_lock_context_host.cc (right):

https://codereview.chromium.org/2734943003/diff/140001/content/browser/wake_l...
content/browser/wake_lock/wake_lock_context_host.cc:36: if
(ServiceManagerConnection::GetForProcess()) {
On 2017/03/22 10:09:55, dcheng wrote:
> On 2017/03/17 12:28:21, blundell wrote:
> > On 2017/03/17 06:55:25, dcheng wrote:
> > > Out of curiosity, why is this check needed at all?
> > 
> > In unittests the service manager connection is not initialized. Added a
> comment
> > to this effect.
> 
> Out of curiosity, is there any chance to add some thing like
> TestBrowserThreadBundle so that it would Just Work in tests?
> 
> (I ask this because I've been surprised by the RenderThreadImpl::current() is
> null in tests behavior in the past; it just so happened that the 'null in
tests'
> check written into non-test code ended up masking other issues)

Filed crbug.com/704098 to track this.

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:18: mojom::WakeLockContextRequest
request,
On 2017/03/22 10:09:55, dcheng wrote:
> Sorry, one more lifetime question...
> 
> Is it possible that |request| is already disconnected at this point? In that
> case, would the connection error handler still be called?

Any messages sent from the other end of the pipe bound to |request| are queued
until this end of the pipe is bound. I imagine that the other end of the pipe
disconnecting is treated as simply the last message to be sent from that end. I
got a little lost trying to actually track this down in the code. Ken can verify
I'm sure.

In any case, I think that this must work because otherwise StrongBinding would
be broken:

https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/strong_binding....

[Ken Rockot(use gerrit already), 2017-03-22 18:25:05]

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:18: mojom::WakeLockContextRequest
request,
On 2017/03/22 at 13:03:40, blundell wrote:
> On 2017/03/22 10:09:55, dcheng wrote:
> > Sorry, one more lifetime question...
> > 
> > Is it possible that |request| is already disconnected at this point? In that
> > case, would the connection error handler still be called?
> 
> Any messages sent from the other end of the pipe bound to |request| are queued
until this end of the pipe is bound. I imagine that the other end of the pipe
disconnecting is treated as simply the last message to be sent from that end. I
got a little lost trying to actually track this down in the code. Ken can verify
I'm sure.
> 
> In any case, I think that this must work because otherwise StrongBinding would
be broken:
> 
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/strong_binding....

+1 to all of that. A pipe being disconnected is not equivalent to it being
unreadable. We only signal a binding error once it becomes unreadable, i.e.
disconnected AND no remaining unread messages.

[Ken Rockot(use gerrit already), 2017-03-22 20:01:57]

(I was supposed to LGTM with that last comment. Oops. LGTM.)

[dcheng, 2017-03-22 20:07:32]

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
File device/wake_lock/wake_lock_service_context.cc (right):

https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
device/wake_lock/wake_lock_service_context.cc:18: mojom::WakeLockContextRequest
request,
On 2017/03/22 18:25:05, Ken Rockot wrote:
> On 2017/03/22 at 13:03:40, blundell wrote:
> > On 2017/03/22 10:09:55, dcheng wrote:
> > > Sorry, one more lifetime question...
> > > 
> > > Is it possible that |request| is already disconnected at this point? In
that
> > > case, would the connection error handler still be called?
> > 
> > Any messages sent from the other end of the pipe bound to |request| are
queued
> until this end of the pipe is bound. I imagine that the other end of the pipe
> disconnecting is treated as simply the last message to be sent from that end.
I
> got a little lost trying to actually track this down in the code. Ken can
verify
> I'm sure.
> > 
> > In any case, I think that this must work because otherwise StrongBinding
would
> be broken:
> > 
> >
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/strong_binding....
> 
> +1 to all of that. A pipe being disconnected is not equivalent to it being
> unreadable. We only signal a binding error once it becomes unreadable, i.e.
> disconnected AND no remaining unread messages.

OK, so the transition to the disconnected state doesn't happen /until/ we try to
read from the pipe. So even if the pipe is broken while the message is in
flight, this is OK: we'll get the connection error when we try to read from the
pipe.

Is this summary correct?

[Ken Rockot(use gerrit already), 2017-03-22 20:20:45]

On 2017/03/22 at 20:07:32, dcheng wrote:
>
https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
> File device/wake_lock/wake_lock_service_context.cc (right):
> 
>
https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
> device/wake_lock/wake_lock_service_context.cc:18:
mojom::WakeLockContextRequest request,
> On 2017/03/22 18:25:05, Ken Rockot wrote:
> > On 2017/03/22 at 13:03:40, blundell wrote:
> > > On 2017/03/22 10:09:55, dcheng wrote:
> > > > Sorry, one more lifetime question...
> > > > 
> > > > Is it possible that |request| is already disconnected at this point? In
that
> > > > case, would the connection error handler still be called?
> > > 
> > > Any messages sent from the other end of the pipe bound to |request| are
queued
> > until this end of the pipe is bound. I imagine that the other end of the
pipe
> > disconnecting is treated as simply the last message to be sent from that
end. I
> > got a little lost trying to actually track this down in the code. Ken can
verify
> > I'm sure.
> > > 
> > > In any case, I think that this must work because otherwise StrongBinding
would
> > be broken:
> > > 
> > >
> >
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/strong_binding....
> > 
> > +1 to all of that. A pipe being disconnected is not equivalent to it being
> > unreadable. We only signal a binding error once it becomes unreadable, i.e.
> > disconnected AND no remaining unread messages.
> 
> OK, so the transition to the disconnected state doesn't happen /until/ we try
to read from the pipe. So even if the pipe is broken while the message is in
flight, this is OK: we'll get the connection error when we try to read from the
pipe.
> 
> Is this summary correct?

No, not entirely.

Once a message is written to a pipe, we guarantee delivery as long as the
receiver is alive. You can do something like:

CreatePipe(&a, &b);
WriteMessage(a, "hi")
Close(a);
WaitForTenMinutes();
Read(b);

and you will still get the message out of |b|. Pipes have to work this way if we
want our APIs to not be terrible.

A pipe handle may at any time become aware that its peer was closed, but the
bindings logic we define does not care about this condition at all. The only
thing a binding cares about is whether the pipe is still readable, and a pipe is
readable as long as (psuedocode: !peer_closed() || has_unread_messages()) is
true.

[dcheng, 2017-03-22 20:27:54]

On 2017/03/22 20:20:45, Ken Rockot wrote:
> On 2017/03/22 at 20:07:32, dcheng wrote:
> >
>
https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
> > File device/wake_lock/wake_lock_service_context.cc (right):
> > 
> >
>
https://codereview.chromium.org/2734943003/diff/200001/device/wake_lock/wake_...
> > device/wake_lock/wake_lock_service_context.cc:18:
> mojom::WakeLockContextRequest request,
> > On 2017/03/22 18:25:05, Ken Rockot wrote:
> > > On 2017/03/22 at 13:03:40, blundell wrote:
> > > > On 2017/03/22 10:09:55, dcheng wrote:
> > > > > Sorry, one more lifetime question...
> > > > > 
> > > > > Is it possible that |request| is already disconnected at this point?
In
> that
> > > > > case, would the connection error handler still be called?
> > > > 
> > > > Any messages sent from the other end of the pipe bound to |request| are
> queued
> > > until this end of the pipe is bound. I imagine that the other end of the
> pipe
> > > disconnecting is treated as simply the last message to be sent from that
> end. I
> > > got a little lost trying to actually track this down in the code. Ken can
> verify
> > > I'm sure.
> > > > 
> > > > In any case, I think that this must work because otherwise StrongBinding
> would
> > > be broken:
> > > > 
> > > >
> > >
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/strong_binding....
> > > 
> > > +1 to all of that. A pipe being disconnected is not equivalent to it being
> > > unreadable. We only signal a binding error once it becomes unreadable,
i.e.
> > > disconnected AND no remaining unread messages.
> > 
> > OK, so the transition to the disconnected state doesn't happen /until/ we
try
> to read from the pipe. So even if the pipe is broken while the message is in
> flight, this is OK: we'll get the connection error when we try to read from
the
> pipe.
> > 
> > Is this summary correct?
> 
> No, not entirely.
> 
> Once a message is written to a pipe, we guarantee delivery as long as the
> receiver is alive. You can do something like:
> 
> CreatePipe(&a, &b);
> WriteMessage(a, "hi")
> Close(a);
> WaitForTenMinutes();
> Read(b);
> 
> and you will still get the message out of |b|. Pipes have to work this way if
we
> want our APIs to not be terrible.
> 
> A pipe handle may at any time become aware that its peer was closed, but the
> bindings logic we define does not care about this condition at all. The only
> thing a binding cares about is whether the pipe is still readable, and a pipe
is
> readable as long as (psuedocode: !peer_closed() || has_unread_messages()) is
> true.

Sorry, I realize my example was a bit unclear. What I'm saying is:

1. The sender sends the InterfaceRequest to the receiver in a Mojo message.
2. While that Mojo message is still in flight, the sender's end of the pipe for
the in-flight InterfaceRequest is closed.
3. The receiver gets the message and the InterfaceRequest contained within.
4. The receiver binds the InterfaceRequest to a Binding and sets a connection
error handler.
5. At some point, the receiver will start trying to read messages off the
InterfaceRequest pipe.
  - If nothing was ever written (the scenario I Was wondering about), we get a
connection error
    at this point, and the connection error handler is invoked (even though it
was 'disconnected'
    before the receiver ever got the request)
  - Otherwise, we read the pending messages off the pipe and signal a connection
error when
    they're all gone.

In both circumstances, we are guaranteed to invoke the connection error handler.

[Ken Rockot(use gerrit already), 2017-03-22 20:29:11]

On Wed, Mar 22, 2017 at 1:27 PM, <dcheng@chromium.org> wrote:

> On 2017/03/22 20:20:45, Ken Rockot wrote:
> > On 2017/03/22 at 20:07:32, dcheng wrote:
> > >
> >
> https://codereview.chromium.org/2734943003/diff/200001/
> device/wake_lock/wake_lock_service_context.cc
> > > File device/wake_lock/wake_lock_service_context.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2734943003/diff/200001/
> device/wake_lock/wake_lock_service_context.cc#newcode18
> > > device/wake_lock/wake_lock_service_context.cc:18:
> > mojom::WakeLockContextRequest request,
> > > On 2017/03/22 18:25:05, Ken Rockot wrote:
> > > > On 2017/03/22 at 13:03:40, blundell wrote:
> > > > > On 2017/03/22 10:09:55, dcheng wrote:
> > > > > > Sorry, one more lifetime question...
> > > > > >
> > > > > > Is it possible that |request| is already disconnected at this
> point?
> In
> > that
> > > > > > case, would the connection error handler still be called?
> > > > >
> > > > > Any messages sent from the other end of the pipe bound to
> |request| are
> > queued
> > > > until this end of the pipe is bound. I imagine that the other end of
> the
> > pipe
> > > > disconnecting is treated as simply the last message to be sent from
> that
> > end. I
> > > > got a little lost trying to actually track this down in the code.
> Ken can
> > verify
> > > > I'm sure.
> > > > >
> > > > > In any case, I think that this must work because otherwise
> StrongBinding
> > would
> > > > be broken:
> > > > >
> > > > >
> > > >
> >
> https://cs.chromium.org/chromium/src/mojo/public/cpp/
> bindings/strong_binding.h?q=strong_bind+package:%5Echromium$&l=92
> > > >
> > > > +1 to all of that. A pipe being disconnected is not equivalent to it
> being
> > > > unreadable. We only signal a binding error once it becomes
> unreadable,
> i.e.
> > > > disconnected AND no remaining unread messages.
> > >
> > > OK, so the transition to the disconnected state doesn't happen /until/
> we
> try
> > to read from the pipe. So even if the pipe is broken while the message
> is in
> > flight, this is OK: we'll get the connection error when we try to read
> from
> the
> > pipe.
> > >
> > > Is this summary correct?
> >
> > No, not entirely.
> >
> > Once a message is written to a pipe, we guarantee delivery as long as the
> > receiver is alive. You can do something like:
> >
> > CreatePipe(&a, &b);
> > WriteMessage(a, "hi")
> > Close(a);
> > WaitForTenMinutes();
> > Read(b);
> >
> > and you will still get the message out of |b|. Pipes have to work this
> way if
> we
> > want our APIs to not be terrible.
> >
> > A pipe handle may at any time become aware that its peer was closed, but
> the
> > bindings logic we define does not care about this condition at all. The
> only
> > thing a binding cares about is whether the pipe is still readable, and a
> pipe
> is
> > readable as long as (psuedocode: !peer_closed() ||
> has_unread_messages()) is
> > true.
>
> Sorry, I realize my example was a bit unclear. What I'm saying is:
>
> 1. The sender sends the InterfaceRequest to the receiver in a Mojo message.
> 2. While that Mojo message is still in flight, the sender's end of the
> pipe for
> the in-flight InterfaceRequest is closed.
> 3. The receiver gets the message and the InterfaceRequest contained within.
> 4. The receiver binds the InterfaceRequest to a Binding and sets a
> connection
> error handler.
> 5. At some point, the receiver will start trying to read messages off the
> InterfaceRequest pipe.
> - If nothing was ever written (the scenario I Was wondering about), we get
> a
> connection error
> at this point, and the connection error handler is invoked (even though it
> was 'disconnected'
> before the receiver ever got the request)
> - Otherwise, we read the pending messages off the pipe and signal a
> connection
> error when
> they're all gone.
>
> In both circumstances, we are guaranteed to invoke the connection error
> handler.
>

Oh. Yes, exactly!


>
> https://codereview.chromium.org/2734943003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2017-03-22 20:30:01]

Thanks for helping me understand. LGTM =)

[blundell, 2017-03-23 09:38:32]

blundell@chromium.org changed reviewers:
+ jam@chromium.org

[blundell, 2017-03-23 09:38:33]

jam@: Can you review //content? Thanks!

[jam, 2017-03-24 14:28:57]

lgtm

[blundell, 2017-03-24 14:36:13]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2017-03-24 14:36: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-03-24 14:39:56]

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

[commit-bot: I haz the power, 2017-03-24 14:39:58]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[blundell, 2017-03-26 20:32:42]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 20:32:49]

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-03-26 22:28:52]

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

[commit-bot: I haz the power, 2017-03-26 22:28:53]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-27 08:07:04]

The CQ bit was checked by blundell@chromium.org

[blundell, 2017-03-27 08:07:04]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2734943003/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-27 08:07:16]

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-03-27 08:11:22]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490602024049610,
"parent_rev": "e93a5f83b296e8294f0726f1426dbc2dbf131d33", "commit_rev":
"e75a8f91ce3a7a09d0da15f1c1fc83a79a668b8e"}

[commit-bot: I haz the power, 2017-03-27 08:12:48]

Description was changed from

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).
(2) Adds a WakeLockContext Mojo interface. This interface can be used to
forward requests to bind wake locks from untrusted clients that are made within
the same (trusted-client specific) context.
(3) Adds a WakeLockContextProvider Mojo interface. This interface allows
for getting a WakeLockContext that is associated with a specific context ID.
The WakeLockContext impl uses that context ID on Android to obtain the
NativeView for its context.

Note that the lifetime model of WakeLockContext is somewhat complex: It must
stay alive as long as either
(1) Its Mojo connection is still valid (as the client might make future
    GetWakeLock() calls) OR
(2) There are still live WakeLock instances that it has instantiated (since
    they call into it when they receive Mojo requests from *their* clients).

Consequently, WakeLockContext monitors the state of the connections described
in (1) and (2), dying only when *all* of those connections go away. As a final
subtlety, WakeLockServiceImpl calls back into WakeLockContext from its
destructor, which executes immediately *after* WakeLockContext receives the
callback that that WakeLockServiceImpl instance has experienced a connection
error. WakeLockContext thus posts a task to delete itself rather than deleting
itself synchronously.

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Device Service: Decouple Wake Lock from //content

Wake Lock presents challenges in decoupling from //content:

(1) It is naturally a per-WebContents interface, as it coalesces
requests made from frames within the same WebContents.
(2) On Android, it needs the NativeView of the WebContents in order to
block the screen from going to sleep.

This CL takes the following approach:
(1) Adds the notion of a context ID, which is set on WakeLockServiceContext
and which the Device Service can use to map contexts to NativeViews (via
a callback that is passed in at Device Service construction time by its
embedder).
(2) Adds a WakeLockContext Mojo interface. This interface can be used to
forward requests to bind wake locks from untrusted clients that are made within
the same (trusted-client specific) context.
(3) Adds a WakeLockContextProvider Mojo interface. This interface allows
for getting a WakeLockContext that is associated with a specific context ID.
The WakeLockContext impl uses that context ID on Android to obtain the
NativeView for its context.

Note that the lifetime model of WakeLockContext is somewhat complex: It must
stay alive as long as either
(1) Its Mojo connection is still valid (as the client might make future
    GetWakeLock() calls) OR
(2) There are still live WakeLock instances that it has instantiated (since
    they call into it when they receive Mojo requests from *their* clients).

Consequently, WakeLockContext monitors the state of the connections described
in (1) and (2), dying only when *all* of those connections go away. As a final
subtlety, WakeLockServiceImpl calls back into WakeLockContext from its
destructor, which executes immediately *after* WakeLockContext receives the
callback that that WakeLockServiceImpl instance has experienced a connection
error. WakeLockContext thus posts a task to delete itself rather than deleting
itself synchronously.

BUG=689403
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features." Restart Chrome. Go to
https://colinblundell.github.io/wake_lock.html and verify that the page
says "Wake Lock API found". Verify that if you wait ~40 seconds, the device
goes to sleep. Tap "Block sleep." Verify that if you wait longer than the
above period, the device does not go to sleep. Tap "Unblock sleep" and
verify again that the device goes to sleep after waiting.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2017-03-27 08:12:50]

Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/e75a8f91ce3a7a09d0da15f1c1fc...