bluetooth: Create base class BluetoothDevice::CreateGattConnection impl.

bluetooth: Create base class BluetoothDevice::CreateGattConnection impl.

A common base class implementation of CreateGattConnection is implemented
here that will be appropriate for at least the Android and Mac platforms.

Part of a 4 patch series:
https://crrev.com/1287753002 Remove Disconnect callback
https://crrev.com/1284073002 Add adapter_ to BluetoothDevice
https://crrev.com/1292263002 BluetoothDevice::CreateGattConnection Impl <<<
https://crrev.com/1256313002 Android Impl & tests.

BUG=512643
Committed: https://crrev.com/9556087a448034de335fc651008947bfa73c1cdd
Cr-Commit-Position: refs/heads/master@{#349900}

[Jeffrey Yasskin, 2015-08-19 22:49:45]

jyasskin@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Jeffrey Yasskin, 2015-08-19 22:49:46]

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:308: DidFailToConnectGatt(ERROR_UNKNOWN);
Don't use "UNKNOWN" here (or ever, really). If you need to add a correct
enumerator, do that.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:312: CHECK(gatt_connection_reference_count_
<
Use base/numerics/safe_math.h instead of implementing your own checks. Or just
don't check: we don't check in places like IdMap either.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:437: //
In/DecrementGattConnectionReferenceCount.
"Inc/Decrement", I think.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:447: // DidDisconnectGatt immediately or
asynchronously as the connection state
"immediately or asynchronously" covers all the options, so you can probably skip
it.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:449: virtual void CreateGattConnectionImpl()
= 0;
If you intend to only call this from BluetoothDevice, it should be private.
Subclasses can still override private members.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:455: // Calls pending callbacks for
CreateGattConnection based on result of
Say something about how Did*Gatt() calls need to match up with
CreateGattConnectionImpl and DisconnectGatt calls. Are they 1-1? Can Did*Gatt()
calls come in spontaneously? Does one of these report the device going out of
range?

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_connection.cc (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:19: if (device)
If !device, IsConnected() should probably return false, even if the device is
later paired and connected, since this object doesn't own a connection
reference.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:27: std::string
BluetoothGattConnection::GetDeviceAddress() const {
Return a const std::string&?

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:33:
adapter_->GetDevice(device_address_)->IsGattConnected();
Do you need to check for adapter_->GetDevice() returning null? If not, comment
why.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:33:
adapter_->GetDevice(device_address_)->IsGattConnected();
This leads to an interesting situation where:
1) we have a Connection object
2) the device moves out of range leading to a disconnection
3) the device moves back into range
4) a *different webpage* reconnects.

I think at this point, our original Connection object now returns true from
IsConnected(), which doesn't seem correct. It should record that it's
disconnected until its owner reconnects it.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_connection.h (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:25:
BluetoothGattConnection(BluetoothAdapter* adapter,
Take this as a scoped_refptr<> to make it clear that you're keeping a reference.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:49: scoped_refptr<BluetoothAdapter>
adapter_;
I'm nervous about holding a reference to BluetoothAdapter from an object that's
notionally "inside" the adapter. How do we systematically prevent reference
cycles?

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:55: bool
already_decremented_connection_reference_on_device_ = false;
Call this something like "owns_reference_for_connection_" or even just
"connected_". The device could be connected without a particular Connection
handle being connected.

[scheib, 2015-09-13 02:40:28]

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:308: DidFailToConnectGatt(ERROR_UNKNOWN);
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> Don't use "UNKNOWN" here (or ever, really). If you need to add a correct
> enumerator, do that.

Ok, though this scenario is a situation where we don't know why we are failing.
I've added a VLOG to make that more clear, and moved to FAILED. If we don't want
UNKNOWN, we should just remove the enum. Though in this case I believe it is
most semantically correct. In the following patch this is called when Android
reports disconnection and a success error status.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:312: CHECK(gatt_connection_reference_count_
<
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> Use base/numerics/safe_math.h instead of implementing your own checks. Or just
> don't check: we don't check in places like IdMap either.

Done. IDMap does check: after increment and next set it checks for duplicate.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:437: //
In/DecrementGattConnectionReferenceCount.
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> "Inc/Decrement", I think.

Done.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:447: // DidDisconnectGatt immediately or
asynchronously as the connection state
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> "immediately or asynchronously" covers all the options, so you can probably
skip
> it.

Done. I was trying to communicate that these may be called safely at any time.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:449: virtual void CreateGattConnectionImpl()
= 0;
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> If you intend to only call this from BluetoothDevice, it should be private.
> Subclasses can still override private members.

Doesn't overriding for implementation then defeat the private scoping? I'm not
sure how this adds compile safety, but I do see that it will split related
methods to be non contiguous.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:455: // Calls pending callbacks for
CreateGattConnection based on result of
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> Say something about how Did*Gatt() calls need to match up with
> CreateGattConnectionImpl and DisconnectGatt calls. Are they 1-1? Can
Did*Gatt()
> calls come in spontaneously? Does one of these report the device going out of
> range?

Done. Regarding out of range, there isn't anything specific in this design that
addresses out of range - but if the platform indicates that the GATT connection
is disconnected then DidDisconnectGatt is the only expected API point to call.
BluetoothDevice doesn't otherwise make any statements about out of range.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_connection.cc (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:19: if (device)
On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> If !device, IsConnected() should probably return false, even if the device is
> later paired and connected, since this object doesn't own a connection
> reference.

Done.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:27: std::string
BluetoothGattConnection::GetDeviceAddress() const {
On 2015/08/19 22:49:46, Jeffrey Yasskin wrote:
> Return a const std::string&?

Done.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:33:
adapter_->GetDevice(device_address_)->IsGattConnected();
On 2015/08/19 22:49:46, Jeffrey Yasskin wrote:
> Do you need to check for adapter_->GetDevice() returning null? If not, comment
> why.

Done.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.cc:33:
adapter_->GetDevice(device_address_)->IsGattConnected();
On 2015/08/19 22:49:46, Jeffrey Yasskin wrote:
> This leads to an interesting situation where:
> 1) we have a Connection object
> 2) the device moves out of range leading to a disconnection
> 3) the device moves back into range
> 4) a *different webpage* reconnects.
> 
> I think at this point, our original Connection object now returns true from
> IsConnected(), which doesn't seem correct. It should record that it's
> disconnected until its owner reconnects it.

Done - refactored reference counting to have pointers to BluetoothGattConnection
objects and to ensure they are set to be disconnected.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_connection.h (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:25:
BluetoothGattConnection(BluetoothAdapter* adapter,
On 2015/08/19 22:49:46, Jeffrey Yasskin wrote:
> Take this as a scoped_refptr<> to make it clear that you're keeping a
reference.

Done.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:49: scoped_refptr<BluetoothAdapter>
adapter_;
On 2015/08/19 22:49:46, Jeffrey Yasskin wrote:
> I'm nervous about holding a reference to BluetoothAdapter from an object
that's
> notionally "inside" the adapter. How do we systematically prevent reference
> cycles?

The adapter doesn't own the connections. The connections are vended to the
consumers of the adapter. Holding a connection object keeps the connection
alive. We must keep the adapter alive to keep the connection alive. This design
predates this patch set and is how ChromeOS is implemented
 src/device/bluetooth/bluetooth_gatt_connection_chromeos.h

https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:55: bool
already_decremented_connection_reference_on_device_ = false;
On 2015/08/19 22:49:46, Jeffrey Yasskin wrote:
> Call this something like "owns_reference_for_connection_" or even just
> "connected_". The device could be connected without a particular Connection
> handle being connected.

Done.

[Jeffrey Yasskin, 2015-09-16 00:45:39]

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:308: DidFailToConnectGatt(ERROR_UNKNOWN);
On 2015/09/13 02:40:27, scheib wrote:
> On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> > Don't use "UNKNOWN" here (or ever, really). If you need to add a correct
> > enumerator, do that.
> 
> Ok, though this scenario is a situation where we don't know why we are
failing.
> I've added a VLOG to make that more clear, and moved to FAILED. If we don't
want
> UNKNOWN, we should just remove the enum.

We have places where we convert a string to an enum value. If an unexpected
string appears, I'd like to propagate it to the user, but our architecture
doesn't allow it, so we have to give the useless UNKNOWN error message. In other
cases, we should give the developer as much information as we can so that they
can debug.

"FAILED" is also bad for developers, since it doesn't tell them what caused the
error. But using FAILED lets us distinguish cases where we're missing a string
translation from other failures that we don't have any more information about.

> Though in this case I believe it is
> most semantically correct. In the following patch this is called when Android
> reports disconnection and a success error status.

Would that happen if the user calls connect() and then disconnect() before the
connect succeeds? Then their promises should reject with an AbortError (per
https://github.com/WebBluetoothCG/web-bluetooth/issues/152), not the
NetworkError that ERROR_FAILED causes.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:447: // DidDisconnectGatt immediately or
asynchronously as the connection state
On 2015/09/13 02:40:27, scheib wrote:
> On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> > "immediately or asynchronously" covers all the options, so you can probably
> skip
> > it.
> 
> Done. I was trying to communicate that these may be called safely at any time.

Ah, yeah, I like the explicit statement you've added to those functions that
they can be called at any time.

You didn't remove the "immediately or asynchronously" here. I don't insist, but
still think it's extraneous.

https://codereview.chromium.org/1292263002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:449: virtual void CreateGattConnectionImpl()
= 0;
On 2015/09/13 02:40:28, scheib wrote:
> On 2015/08/19 22:49:45, Jeffrey Yasskin wrote:
> > If you intend to only call this from BluetoothDevice, it should be private.
> > Subclasses can still override private members.
> 
> Doesn't overriding for implementation then defeat the private scoping? I'm not
> sure how this adds compile safety, but I do see that it will split related
> methods to be non contiguous.

Yeah, good point: subclasses will be able to call their own overrides. Making
the function private still makes it clear that the base class doesn't intend to
provide any service to the subclasses through this function, but, on the other
hand, the =0 makes the same statement, so I guess it doesn't matter much.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:319: for (auto connection :
gatt_connections_) {
'auto' often causes extra copies in range-based for loops. In this case, it
doesn't, so I'd use 'auto*' to be clear that it's going to be a pointer. Or just
BluetoothGattConnection* since that isn't too much longer and makes it easier to
find the target of the call in the loop.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:319: for (auto connection :
gatt_connections_) {
I think you need to gatt_connections_.clear() since the
InvalidateConnectionReference prevents the GattConnection from removing itself
in its destructor.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:457: // disconnection events. These may be
called at any time, even multiple times,
The implementation says DidDisconnectGatt() shouldn't be called between
CreateGattConnectionImpl() and the next call to either DidConnectGatt() and
DidFailToConnectGatt().

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:464: // BluetoothGattConnection instances.
Is it too obvious to say that the BluetoothGattConnection is responsible to call
RemoveGattConnection(this) before it's destroyed?

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_connection.h (right):

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:52: // Only to be called by
BluetoothDevice::~BluetoothDevice to avoid reentrant
DidDisconnectGatt() also calls this.

[scheib, 2015-09-16 19:52:04]

Patchset #5 (id:80001) has been deleted

[scheib, 2015-09-16 21:19:08]

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:319: for (auto connection :
gatt_connections_) {
On 2015/09/16 00:45:39, Jeffrey Yasskin wrote:
> I think you need to gatt_connections_.clear() since the
> InvalidateConnectionReference prevents the GattConnection from removing itself
> in its destructor.

Done.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:319: for (auto connection :
gatt_connections_) {
On 2015/09/16 00:45:39, Jeffrey Yasskin wrote:
> 'auto' often causes extra copies in range-based for loops. In this case, it
> doesn't, so I'd use 'auto*' to be clear that it's going to be a pointer. Or
just
> BluetoothGattConnection* since that isn't too much longer and makes it easier
to
> find the target of the call in the loop.

Done.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:457: // disconnection events. These may be
called at any time, even multiple times,
On 2015/09/16 00:45:39, Jeffrey Yasskin wrote:
> The implementation says DidDisconnectGatt() shouldn't be called between
> CreateGattConnectionImpl() and the next call to either DidConnectGatt() and
> DidFailToConnectGatt().

Done.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:464: // BluetoothGattConnection instances.
On 2015/09/16 00:45:39, Jeffrey Yasskin wrote:
> Is it too obvious to say that the BluetoothGattConnection is responsible to
call
> RemoveGattConnection(this) before it's destroyed?

Done.

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_gatt_connection.h (right):

https://codereview.chromium.org/1292263002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_connection.h:52: // Only to be called by
BluetoothDevice::~BluetoothDevice to avoid reentrant
On 2015/09/16 00:45:39, Jeffrey Yasskin wrote:
> DidDisconnectGatt() also calls this.

Done.

[scheib, 2015-09-16 21:20:51]

Patchset #5 (id:100001) has been deleted

[scheib, 2015-09-16 21:21:08]

Patchset #5 (id:120001) has been deleted

[Jeffrey Yasskin, 2015-09-17 00:13:02]

lgtm

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:7: #include <limits>
I think you don't need this anymore.

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:15: #include "base/numerics/safe_math.h"
I think you don't need this anymore.

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:467: // Maintains GattConnection reference
count, being called from friend class
The English major in me winces. Maybe "Tracks connected BluetoothGattConnection
instances, which call these methods on creation and deletion/disconnection."?

[scheib, 2015-09-17 18:18:10]

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.cc:7: #include <limits>
On 2015/09/17 00:13:02, Jeffrey Yasskin wrote:
> I think you don't need this anymore.

Done.

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:15: #include "base/numerics/safe_math.h"
On 2015/09/17 00:13:02, Jeffrey Yasskin wrote:
> I think you don't need this anymore.

Done.

https://codereview.chromium.org/1292263002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device.h:467: // Maintains GattConnection reference
count, being called from friend class
On 2015/09/17 00:13:02, Jeffrey Yasskin wrote:
> The English major in me winces. Maybe "Tracks connected
BluetoothGattConnection
> instances, which call these methods on creation and deletion/disconnection."?

Done.

[scheib, 2015-09-17 19:51:12]

Also, looks like I need to fix ProximityAuthBluetoothLowEnergyConnectionTest.*
-- working on that.

[scheib, 2015-09-18 20:57:24]

scheib@chromium.org changed reviewers:
+ armansito@chromium.org

[scheib, 2015-09-18 20:57:25]

Arman, OWNERS please.

[scheib, 2015-09-19 21:13:50]

scheib@chromium.org changed reviewers:
+ isherman@chromium.org

[scheib, 2015-09-19 21:13:51]

isherman:
 components/proximity_auth/ble/bluetooth_low_energy_connection_unittest.cc

[armansito, 2015-09-19 21:26:51]

lgtm

[Ilya Sherman, 2015-09-21 02:56:22]

LGTM

[scheib, 2015-09-21 04:00:26]

The CQ bit was checked by scheib@chromium.org

[scheib, 2015-09-21 04:00:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1292263002/#ps180001
(title: "fix ProximityAuthBluetoothLowEnergyConnectionTest")

[commit-bot: I haz the power, 2015-09-21 04:00:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1292263002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1292263002/180001

[commit-bot: I haz the power, 2015-09-21 05:09:42]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-09-21 05:10:26]

Patchset 7 (id:??) landed as
https://crrev.com/9556087a448034de335fc651008947bfa73c1cdd
Cr-Commit-Position: refs/heads/master@{#349900}