Fix CompositorResizeLock to do something.

Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. This maintains the property that the
first to grab a lock chooses the timeout for all other locks grabbed
during that time, however Mac code which wants no timeouts now
doesn't cause other parts of the system to also get no timeout unless
the mac lock is currently active without a timeout.

R=piman@chromium.org
BUG=704928
NOTRY=true

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

[danakj, 2017-03-23 18:05:46]

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

[commit-bot: I haz the power, 2017-03-23 18:06:28]

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-23 18:16:26]

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

[commit-bot: I haz the power, 2017-03-23 18:16:27]

Dry run: Try jobs failed on following builders:
  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_...)

[danakj, 2017-03-23 18:44:23]

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

[commit-bot: I haz the power, 2017-03-23 18:44:52]

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-23 19:13:34]

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

[commit-bot: I haz the power, 2017-03-23 19:13:35]

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

[danakj, 2017-03-23 22:22:59]

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

[commit-bot: I haz the power, 2017-03-23 22:23:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-23 22:41:05]

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

[commit-bot: I haz the power, 2017-03-23 22:41:40]

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-23 22:47:50]

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

[commit-bot: I haz the power, 2017-03-23 22:47:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2017-03-23 22:48:09]

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

[commit-bot: I haz the power, 2017-03-23 22:48:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-23 22:52:15]

Description was changed from

==========
Fix ResizeLock to do something.

This makes the base class abstract instead of calling virtual methods
from the constructor that skip the subclass. Before this, only deferred
locks would actually do anything.

R=piman@chromium.org
BUG=
==========

to

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout.

R=piman@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2017-03-23 22:54:35]

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

[commit-bot: I haz the power, 2017-03-23 22:54:36]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2017-03-23 23:00:49]

Description was changed from

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout.

R=piman@chromium.org
BUG=
==========

to

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. Now mac can just request locks with
a 0 timeout, without having effects on other Compositor clients in
the system.

R=piman@chromium.org
BUG=
==========

[danakj, 2017-03-23 23:02:43]

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

[commit-bot: I haz the power, 2017-03-23 23:03:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-23 23:05:42]

chrome_vox tests fail (via dcheck) because they are causing a commit somehow
during a compositorlock. ericrk said he recalls them resizing inside an
animation or something. I will investigate that tomorrow.

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

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

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

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

[piman, 2017-03-24 04:05:31]

https://codereview.chromium.org/2773433003/diff/100001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2773433003/diff/100001/ui/compositor/composit...
ui/compositor/compositor.h:308: CompositorLockClient* client,
2 things.
1- I assume we want the client to outlive the CompositorLock. However, because
the CompositorLock is refcounted, and arbitrary unrelated things may want to
grab one, which will extend the lifetime of the CompositorLock, unbeknownst to
|client|.
2- relatedly, the second caller that tries to grab it will get a CompositorLock
that will not call into |client|

IOW I think the client needs to be put in an ObserverList (so that we can have
more than one and so that they can remove themselves).

[danakj, 2017-03-24 14:49:26]

https://codereview.chromium.org/2773433003/diff/100001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2773433003/diff/100001/ui/compositor/composit...
ui/compositor/compositor.h:308: CompositorLockClient* client,
On 2017/03/24 04:05:31, piman wrote:
> 2 things.
> 1- I assume we want the client to outlive the CompositorLock. However, because
> the CompositorLock is refcounted, and arbitrary unrelated things may want to
> grab one, which will extend the lifetime of the CompositorLock, unbeknownst to
> |client|.
> 2- relatedly, the second caller that tries to grab it will get a
CompositorLock
> that will not call into |client|
> 
> IOW I think the client needs to be put in an ObserverList (so that we can have
> more than one and so that they can remove themselves).

Thank you, good point.

[danakj, 2017-03-24 16:54:38]

Description was changed from

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. Now mac can just request locks with
a 0 timeout, without having effects on other Compositor clients in
the system.

R=piman@chromium.org
BUG=
==========

to

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. This maintains the property that the
first to grab a lock chooses the timeout for all other locks grabbed
during that time, however Mac code which wants no timeouts now
doesn't cause other parts of the system to also get no timeout unless
the mac lock is currently active without a timeout.

R=piman@chromium.org
BUG=704928
==========

[danakj, 2017-03-24 16:54:56]

Back to the chrome_vox thing after lunch.

https://codereview.chromium.org/2773433003/diff/100001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2773433003/diff/100001/ui/compositor/composit...
ui/compositor/compositor.h:308: CompositorLockClient* client,
On 2017/03/24 14:49:26, danakj wrote:
> On 2017/03/24 04:05:31, piman wrote:
> > 2 things.
> > 1- I assume we want the client to outlive the CompositorLock. However,
because
> > the CompositorLock is refcounted, and arbitrary unrelated things may want to
> > grab one, which will extend the lifetime of the CompositorLock, unbeknownst
to
> > |client|.
> > 2- relatedly, the second caller that tries to grab it will get a
> CompositorLock
> > that will not call into |client|
> > 
> > IOW I think the client needs to be put in an ObserverList (so that we can
have
> > more than one and so that they can remove themselves).
> 
> Thank you, good point.

I've made CompositorLock a unique_ptr instead, but the timeout is shared and
implemented in ui::Compositor. Added a test to verify this.

[danakj, 2017-03-24 16:55:35]

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

[commit-bot: I haz the power, 2017-03-24 16:56:05]

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-24 17:00:34]

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

[commit-bot: I haz the power, 2017-03-24 17:00:37]

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

[danakj, 2017-03-24 17:33:35]

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

[commit-bot: I haz the power, 2017-03-24 17:34:02]

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-24 17:52:26]

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

[commit-bot: I haz the power, 2017-03-24 17:52:28]

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

[piman, 2017-03-24 18:42:34]

LGTM+nit, there's test failures though (didn't look in details)

https://codereview.chromium.org/2773433003/diff/140001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2773433003/diff/140001/ui/compositor/composit...
ui/compositor/compositor.h:418: base::WeakPtrFactory<Compositor>
lock_timeout_weak_ptr_factory_;
nit: do we need 2 separate WeakPtrFactory? I don't think we explicitly
invalidate either one (did I miss it?), so the weakptrs would have the same
lifetime coming from either, right?

[danakj, 2017-03-24 18:44:33]

https://codereview.chromium.org/2773433003/diff/140001/ui/compositor/composit...
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/2773433003/diff/140001/ui/compositor/composit...
ui/compositor/compositor.h:418: base::WeakPtrFactory<Compositor>
lock_timeout_weak_ptr_factory_;
On 2017/03/24 18:42:34, piman wrote:
> nit: do we need 2 separate WeakPtrFactory? I don't think we explicitly
> invalidate either one (did I miss it?), so the weakptrs would have the same
> lifetime coming from either, right?

Oh, I wanted to invalidate when the last lock is removed. But the list would
just be empty so I don't think we need a weakptr at all actually. We expect all
locks to be gone when Compositor goes away.

[piman, 2017-03-24 18:46:03]

On 2017/03/24 18:44:33, danakj wrote:
>
https://codereview.chromium.org/2773433003/diff/140001/ui/compositor/composit...
> File ui/compositor/compositor.h (right):
> 
>
https://codereview.chromium.org/2773433003/diff/140001/ui/compositor/composit...
> ui/compositor/compositor.h:418: base::WeakPtrFactory<Compositor>
> lock_timeout_weak_ptr_factory_;
> On 2017/03/24 18:42:34, piman wrote:
> > nit: do we need 2 separate WeakPtrFactory? I don't think we explicitly
> > invalidate either one (did I miss it?), so the weakptrs would have the same
> > lifetime coming from either, right?
> 
> Oh, I wanted to invalidate when the last lock is removed. But the list would
> just be empty so I don't think we need a weakptr at all actually. We expect
all
> locks to be gone when Compositor goes away.

SG, we can add a DCHECK for that then.

[danakj, 2017-03-24 18:51:07]

On Fri, Mar 24, 2017 at 2:46 PM, <piman@chromium.org> wrote:

> On 2017/03/24 18:44:33, danakj wrote:
> >
> https://codereview.chromium.org/2773433003/diff/140001/ui/
> compositor/compositor.h
> > File ui/compositor/compositor.h (right):
> >
> >
> https://codereview.chromium.org/2773433003/diff/140001/ui/
> compositor/compositor.h#newcode418
> > ui/compositor/compositor.h:418: base::WeakPtrFactory<Compositor>
> > lock_timeout_weak_ptr_factory_;
> > On 2017/03/24 18:42:34, piman wrote:
> > > nit: do we need 2 separate WeakPtrFactory? I don't think we explicitly
> > > invalidate either one (did I miss it?), so the weakptrs would have the
> same
> > > lifetime coming from either, right?
> >
> > Oh, I wanted to invalidate when the last lock is removed. But the list
> would
> > just be empty so I don't think we need a weakptr at all actually. We
> expect
> all
> > locks to be gone when Compositor goes away.
>
> SG, we can add a DCHECK for that then.
>

Yep yep, on it.

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

[danakj, 2017-03-24 18:59:06]

On 2017/03/24 18:51:07, danakj wrote:
> On Fri, Mar 24, 2017 at 2:46 PM, <mailto:piman@chromium.org> wrote:
> 
> > On 2017/03/24 18:44:33, danakj wrote:
> > >
> > https://codereview.chromium.org/2773433003/diff/140001/ui/
> > compositor/compositor.h
> > > File ui/compositor/compositor.h (right):
> > >
> > >
> > https://codereview.chromium.org/2773433003/diff/140001/ui/
> > compositor/compositor.h#newcode418
> > > ui/compositor/compositor.h:418: base::WeakPtrFactory<Compositor>
> > > lock_timeout_weak_ptr_factory_;
> > > On 2017/03/24 18:42:34, piman wrote:
> > > > nit: do we need 2 separate WeakPtrFactory? I don't think we explicitly
> > > > invalidate either one (did I miss it?), so the weakptrs would have the
> > same
> > > > lifetime coming from either, right?
> > >
> > > Oh, I wanted to invalidate when the last lock is removed. But the list
> > would
> > > just be empty so I don't think we need a weakptr at all actually. We
> > expect
> > all
> > > locks to be gone when Compositor goes away.
> >
> > SG, we can add a DCHECK for that then.
> >
> 
> Yep yep, on it.

OH I remembered why I need the weakptrfactory. I'll add a test.

Add lock with timeout 10
Remove lock
Add lock with timeout 50
Wait 10
The old timeout would cancel the 50 one cuz it's still posted.

[danakj, 2017-03-24 19:04:07]

On 2017/03/24 18:59:06, danakj wrote:
> On 2017/03/24 18:51:07, danakj wrote:
> > On Fri, Mar 24, 2017 at 2:46 PM, <mailto:piman@chromium.org> wrote:
> > 
> > > On 2017/03/24 18:44:33, danakj wrote:
> > > >
> > > https://codereview.chromium.org/2773433003/diff/140001/ui/
> > > compositor/compositor.h
> > > > File ui/compositor/compositor.h (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2773433003/diff/140001/ui/
> > > compositor/compositor.h#newcode418
> > > > ui/compositor/compositor.h:418: base::WeakPtrFactory<Compositor>
> > > > lock_timeout_weak_ptr_factory_;
> > > > On 2017/03/24 18:42:34, piman wrote:
> > > > > nit: do we need 2 separate WeakPtrFactory? I don't think we explicitly
> > > > > invalidate either one (did I miss it?), so the weakptrs would have the
> > > same
> > > > > lifetime coming from either, right?
> > > >
> > > > Oh, I wanted to invalidate when the last lock is removed. But the list
> > > would
> > > > just be empty so I don't think we need a weakptr at all actually. We
> > > expect
> > > all
> > > > locks to be gone when Compositor goes away.
> > >
> > > SG, we can add a DCHECK for that then.
> > >
> > 
> > Yep yep, on it.
> 
> OH I remembered why I need the weakptrfactory. I'll add a test.
> 
> Add lock with timeout 10
> Remove lock
> Add lock with timeout 50
> Wait 10
> The old timeout would cancel the 50 one cuz it's still posted.

I also lied about locks being ended before the Compositor is destroyed. That's a
crash I'm seeing in a test. The old code would CancelCompositorLock in
~Compositor() so that they could outlive but would be detached. I'll add another
unit test for this.

[danakj, 2017-03-24 19:29:09]

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

[danakj, 2017-03-24 19:29:26]

Ok latest patchset fixes the timeout bleeding, and ensures locks can outlive the
compositor, with a unittest that does this. Bots *should* be green at this point
cuz I've rebased on top of the defer commits fix earlier.

[commit-bot: I haz the power, 2017-03-24 19:29:50]

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-24 19:49:35]

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

[commit-bot: I haz the power, 2017-03-24 19:49:36]

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

[piman, 2017-03-24 19:57:45]

LGTM, but I worry about 1 possible race/flaky test

https://codereview.chromium.org/2773433003/diff/160001/ui/compositor/composit...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/2773433003/diff/160001/ui/compositor/composit...
ui/compositor/compositor_unittest.cc:219:
task_runner()->PostDelayedTask(FROM_HERE, run_loop.QuitClosure(), timeout1);
This one could in theory be racy causing a flake, if for some reason l.214 or
l.218 take >9ms to execute, causing this callback to run after the timeout2. Is
it possible to inject fake time, or is it hard?

[danakj, 2017-03-24 20:04:48]

https://codereview.chromium.org/2773433003/diff/160001/ui/compositor/composit...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/2773433003/diff/160001/ui/compositor/composit...
ui/compositor/compositor_unittest.cc:219:
task_runner()->PostDelayedTask(FROM_HERE, run_loop.QuitClosure(), timeout1);
On 2017/03/24 19:57:45, piman wrote:
> This one could in theory be racy causing a flake, if for some reason l.214 or
> l.218 take >9ms to execute, causing this callback to run after the timeout2.
Is
> it possible to inject fake time, or is it hard?

Good point, let me look around in base/test/

[danakj, 2017-03-24 21:39:24]

I have made mock time for all compositor_unittests.cc except one where it needs
a real messageloop cuz it is using the scheduler and stuff.

[danakj, 2017-03-24 21:47:59]

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

[commit-bot: I haz the power, 2017-03-24 21:48:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2017-03-24 21:54:28]

thank you!! LGTM.

[commit-bot: I haz the power, 2017-03-24 22:07:42]

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

[commit-bot: I haz the power, 2017-03-24 22:07:43]

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

[danakj, 2017-03-25 00:02:24]

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

[commit-bot: I haz the power, 2017-03-25 00:03:08]

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-25 00:19:06]

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

[commit-bot: I haz the power, 2017-03-25 00:19:07]

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

[danakj, 2017-03-28 22:11:51]

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

[commit-bot: I haz the power, 2017-03-28 22:12: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-28 22:22:49]

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

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

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

[danakj, 2017-03-28 23:16:13]

https://codereview.chromium.org/2773433003/diff/260001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2773433003/diff/260001/ui/compositor/composit...
ui/compositor/compositor.cc:556: active_locks_.push_back(lock.get());
Oh, this change makes OnCompositingLockStateChanged fire before IsLocked()
returns true. This turned up in a couple RWHVA unittests because I *also*
happened to make them use a CompositorLock tied to the real compositor, which
exposed this bug. Before they were using an independent CompositorLock that
didn't do anything.

I've added a compositor_unittest for the observer being called at the right
time. And also made RWHVA unittests use a disconnected lock again so that it
won't have a timeout. I wanted to try make it use a real lock but specify the
timeout but getting access to the Compositor* there is hard because of lifetimes
and clients and friends for unit tests... since I'm happy with the
compositor_unittests coverage I left it this way.

[danakj, 2017-03-28 23:16:39]

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

[commit-bot: I haz the power, 2017-03-28 23:17:15]

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-28 23:36:11]

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

[commit-bot: I haz the power, 2017-03-28 23:36:13]

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

[piman, 2017-03-29 00:08:06]

lgtm

[danakj, 2017-03-29 19:20:19]

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

[commit-bot: I haz the power, 2017-03-29 19:21:24]

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-29 19:25:03]

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

[commit-bot: I haz the power, 2017-03-29 19:25:04]

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

[danakj, 2017-03-29 20:00:48]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-03-29 20:00:49]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2773433003/#ps320001
(title: "resizelock: rebase")

[commit-bot: I haz the power, 2017-03-29 20:01:42]

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-29 20:23:52]

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

[commit-bot: I haz the power, 2017-03-29 20:23:53]

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

[danakj, 2017-03-29 20:35:14]

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

[commit-bot: I haz the power, 2017-03-29 20:36:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-03-29 20:41:56]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-03-29 20:42:03]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-03-29 20:42:03]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2773433003/#ps340001
(title: "resizelock: observer-rebase")

[commit-bot: I haz the power, 2017-03-29 20:42:21]

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

[danakj, 2017-03-29 23:20:02]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-03-29 23:20:07]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-03-29 23:20:18]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-03-29 23:20:26]

Description was changed from

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. This maintains the property that the
first to grab a lock chooses the timeout for all other locks grabbed
during that time, however Mac code which wants no timeouts now
doesn't cause other parts of the system to also get no timeout unless
the mac lock is currently active without a timeout.

R=piman@chromium.org
BUG=704928
==========

to

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. This maintains the property that the
first to grab a lock chooses the timeout for all other locks grabbed
during that time, however Mac code which wants no timeouts now
doesn't cause other parts of the system to also get no timeout unless
the mac lock is currently active without a timeout.

R=piman@chromium.org
BUG=704928
NOTRY=true
==========

[danakj, 2017-03-29 23:20:35]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2017-03-29 23:21:47]

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-29 23:30:54]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1490829635593540,
"parent_rev": "824a4fafaea59bede0f34603abfd0f550db4992e", "commit_rev":
"ea89dd544163618531f6892ce94b1b06fd006c14"}

[commit-bot: I haz the power, 2017-03-29 23:31:43]

Description was changed from

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. This maintains the property that the
first to grab a lock chooses the timeout for all other locks grabbed
during that time, however Mac code which wants no timeouts now
doesn't cause other parts of the system to also get no timeout unless
the mac lock is currently active without a timeout.

R=piman@chromium.org
BUG=704928
NOTRY=true
==========

to

==========
Fix CompositorResizeLock to do something.

Currently CompositorResizeLock doesn't work unless deferred since the
base class tries to use a virtual method from its constructor which
doesn't know about the subclass yet.

This fixes that and makes the following changes in the process:

I noticed that there are no longer any unit tests that depend on
ResizeLock being an interface. So I made CompositorResizeLock the
concrete type.

I noticed that CompositorResizeLock was in located in files named
compositor_resize_lock_aura.{h,cc} but is not named for aura anymore,
and is in fact used in non-aura cases for mac. So I renamed the files,
and moved the aura pieces out to DelegatedFrameHostClientAura.

In order for DelegatedFrameHostClientAura to know when to release
events however, it needs to know when the resize lock ends. So I
introduced CompositorResizeLockClient, which tells when the lock is
over.

Importantly, the above is not the same as the CompositorLock ending.
That can be done by calling CompositorResizeLock::UnlockCompositor,
which does not end the resize lock, but does release the
CompositorLock inside it. So this doesn't inform the client at
that time until the resize lock actually ends. I've tried to make
this more clear with function names and comments.

In order to unit test everything I've pulled CompositorLock out
of compositor.h and removed the friend relationships between
Compositor and CompositorLock. To do so CompositorLock now has 2
interfaces:
- CompositorLockClient which it informs about timeouts. This is
so that CompositorResizeLock knows when its internal
CompositorLock is released by timeout.
- CompositorLockDelegate which performs actual unlocking. This is
the ui::Compositor outside of tests.

Lastly, CompositorResizeLock has a timeout, and CompositorLock has
a timeout. These happen to be the same number tho
CompositorResizeLock allows you to override it. I've combined these
into a single timeout (with the help of CompositorLockClient). And
will allow GetCompositorLock to request a specific timeout. With
that, Compositor no longer needs SetLocksWillTimeOut which has some
action-at-distance properties. This maintains the property that the
first to grab a lock chooses the timeout for all other locks grabbed
during that time, however Mac code which wants no timeouts now
doesn't cause other parts of the system to also get no timeout unless
the mac lock is currently active without a timeout.

R=piman@chromium.org
BUG=704928
NOTRY=true

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

[commit-bot: I haz the power, 2017-03-29 23:31:45]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/ea89dd544163618531f6892ce94b...