Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
876153002:
device/bluetooth:Implement Register() for BluetoothAudioSinkChromeOS. (Closed)
|
Created:
5 years, 11 months ago by Miao Modified:
5 years, 10 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondevice/bluetooth:Implement Register() for BluetoothAudioSinkChromeOS.
This CL implements Register function for BluetoothAudioSinkChromeOS
and comes up with the corresponding tests. For the tests,
FakeBluetoothMediaClient is changed to simulate
the behavior of the
real D-Bus APIs.
BUG=441581
TEST=device_unittests --gtest_filter=*AudioSink*
Committed: https://crrev.com/6cf69f3915ec2b0972f36161958d10642f3d27c9
Cr-Commit-Position: refs/heads/master@{#314244}
Patch Set 1 #
Total comments: 39
Patch Set 2 : Removed AdapterPoweredChanged(), |present_| and |powered_|. #
Total comments: 31
Patch Set 3 : Moved kBluetoothAudioSinkUUID to BluetoothMediaClient. #
Total comments: 30
Patch Set 4 : Removed unused include, moved default codec and fixed nits. #
Total comments: 8
Patch Set 5 : Fixed nits. #
Total comments: 2
Patch Set 6 : Marked BluetoothAudioSink with DEVICE_BLUETOOTH_EXPORT. #Messages
Total messages: 36 (11 generated)
mcchou@chromium.org changed reviewers: + armansito@chromium.org, benchan@chromium.org
First round comments. I haven't looked at the unit test yet. https://codereview.chromium.org/876153002/diff/1/chromeos/chromeos.gyp File chromeos/chromeos.gyp (left): https://codereview.chromium.org/876153002/diff/1/chromeos/chromeos.gyp#oldcod... chromeos/chromeos.gyp:130: 'dbus/face_bluetooth_media_client.h', lol https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:9: const char kNotImplementedError[] = "org.chromium.Error.NotImplemented"; nit: don't indent variables within namespace. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:10: const char kInvalidArgumentError[] = "org.chromium.Error.InvalidArgument"; nit: newline. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:43: media_path_ = object_path; I'm not exactly sure what you're using |media_path_| for here. Since a media interface exists on each adapter object, I'd say the proper behavior here would be to check with FakeBluetoothAdapterClient to see if |object_path| corresponds to a valid adapter and then fail based on that. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:44: if (properties.capabilities.empty()) { What about the |codec| and |uuid| fields? https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.h (left): https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.h:40: nit: Add a comment explaining what this member does. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:661: DCHECK(audio_sink); DCHECK(audio_sink.get())? https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:998: DCHECK_EQ(num_discovery_sessions_, 0); Is this refactor related to this CL? https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_chromeos.h (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.h:128: friend class BluetoothAudioSinkChromeOSTest; Is the friendship really necessary? https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:77: if (present == present_) return; nit: Put return in its own line. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:77: if (present == present_) return; This check isn't really necessary, since the adapter won't call this if the value hasn't changed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:80: present_ = present; I don't think that |present_| and |powered_| are necessary member variables since you can obtain these from the adapter by calling adapter_->IsPresent() and adapter_->IsPowered(). So I would remove them from the class. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:83: } else { This means that the state will become INVALID if |present_| is true and IsPowered is false, which is probably not what we want. I would simply mark the state as invalid if |present_| is false. Otherwise, mark the state as DISCONNECTED if no media transport is currently assigned. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:91: if (powered == powered_) return; Same as above. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:98: StateChanged(device::BluetoothAudioSink::STATE_INVALID); Same logic as above. Besides this code is essentially identical, so I would put the state determination into a single method. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:166: dbus::ObjectPath endpoint_path_ = GenerateEndpointPath(); This code shouldn't compile, since you're redeclaring |endpoint_path_| (which appears to be a member variable). https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:205: if (state == state_) return; nit: return; in its own line. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:235: default: { No need for empty scope here. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:27: class BluetoothAdapterChromeOS; I don't see you dealing with BluetoothAdapterChromeOS pointers in this header. Why is this forward declaration necessary? https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:93: // Called when the registration of Media Endpoint Done. s/Done/has succeeded/ https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015
https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:9: const char kNotImplementedError[] = "org.chromium.Error.NotImplemented"; On 2015/01/27 04:38:05, armansito wrote: > nit: don't indent variables within namespace. Done. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:10: const char kInvalidArgumentError[] = "org.chromium.Error.InvalidArgument"; On 2015/01/27 04:38:05, armansito wrote: > nit: newline. Done. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:43: media_path_ = object_path; On 2015/01/27 04:38:05, armansito wrote: > I'm not exactly sure what you're using |media_path_| for here. Since a media > interface exists on each adapter object, I'd say the proper behavior here would > be to check with FakeBluetoothAdapterClient to see if |object_path| corresponds > to a valid adapter and then fail based on that. Done. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:44: if (properties.capabilities.empty()) { On 2015/01/27 04:38:05, armansito wrote: > What about the |codec| and |uuid| fields? Done. https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.h (left): https://codereview.chromium.org/876153002/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.h:40: On 2015/01/27 04:38:05, armansito wrote: > nit: Add a comment explaining what this member does. Removed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:661: DCHECK(audio_sink); On 2015/01/27 04:38:05, armansito wrote: > DCHECK(audio_sink.get())? Done. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_adapter_chromeos.cc:998: DCHECK_EQ(num_discovery_sessions_, 0); On 2015/01/27 04:38:05, armansito wrote: > Is this refactor related to this CL? It is not related to this CL. Lint complained about these. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:77: if (present == present_) return; On 2015/01/27 04:38:06, armansito wrote: > nit: Put return in its own line. Removed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:80: present_ = present; On 2015/01/27 04:38:06, armansito wrote: > I don't think that |present_| and |powered_| are necessary member variables > since you can obtain these from the adapter by calling adapter_->IsPresent() and > adapter_->IsPowered(). So I would remove them from the class. Removed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:83: } else { On 2015/01/27 04:38:06, armansito wrote: > This means that the state will become INVALID if |present_| is true and > IsPowered is false, which is probably not what we want. I would simply mark the > state as invalid if |present_| is false. Otherwise, mark the state as > DISCONNECTED if no media transport is currently assigned. Will remove AdapterPoweredChanged and let MediaTransportRemoved change the state to STATE_DISCONNECTED. The MediaTransport will be destroyed if powered is down. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:91: if (powered == powered_) return; On 2015/01/27 04:38:05, armansito wrote: > Same as above. Removed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:98: StateChanged(device::BluetoothAudioSink::STATE_INVALID); On 2015/01/27 04:38:05, armansito wrote: > Same logic as above. Besides this code is essentially identical, so I would put > the state determination into a single method. Removed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:166: dbus::ObjectPath endpoint_path_ = GenerateEndpointPath(); On 2015/01/27 04:38:05, armansito wrote: > This code shouldn't compile, since you're redeclaring |endpoint_path_| (which > appears to be a member variable). The compilation will pass, since it will be a local variable hiding the class-wide one, but I shouldn't redeclare endpoint_point here anyway. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:205: if (state == state_) return; On 2015/01/27 04:38:06, armansito wrote: > nit: return; in its own line. Done. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:235: default: { On 2015/01/27 04:38:05, armansito wrote: > No need for empty scope here. Done. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:27: class BluetoothAdapterChromeOS; On 2015/01/27 04:38:06, armansito wrote: > I don't see you dealing with BluetoothAdapterChromeOS pointers in this header. > Why is this forward declaration necessary? Removed. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:93: // Called when the registration of Media Endpoint Done. On 2015/01/27 04:38:06, armansito wrote: > s/Done/has succeeded/ Done. https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/27 04:38:06, armansito wrote: > 2015 :P
https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:14: const char kBluetoothAudioSinkUUID[] = "0000110b-0000-1000-8000-00805f9b34fb"; do you expect to have a TODO to move these constants somewhere? https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:59: if (properties.uuid != std::string(kBluetoothAudioSinkUUID) || EndpointProperties.uuid is a std::string, so std::string on the RHS isn't necessary. https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:60: properties.codec != 0x00 || perhaps define 0x00 as something like kDefaultCodec or kCodecSBC? https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:310: BluetoothAudioSinkChromeOS* audio_sink = new BluetoothAudioSinkChromeOS(this); It's not very obvious/clear whether the scoped_refptr of |audio_sink| ever destroys |audio_sink| (due to ref count goes to zero) before Register returns, so it's probably better to write: scoped_refptr<device::BluetoothAudioSink> audio_sink(new ...); audio_sink->Register(... , audio_sink), error_callback); https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:161: endpoint_properties.uuid = std::string(kBluetoothAudioSinkUUID); std::string isn't necessary as EndpointProperties.uuid is a std::string https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:251: const BluetoothAudioSink::ErrorCallback& error_back, error_back? https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:257: endpoint_path_ = dbus::ObjectPath(""); is dbus::ObjectPath necessary? https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:43: // device::BluetoothAdapter::Observer override. this change is probably not necessary. see // BluetoothMediaClient::Observer overrides below https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:94: void OnRegisterFailed(const BluetoothAudioSink::ErrorCallback& error_back, error_back? https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:76: base::Bind(&BluetoothAudioSinkChromeOSTest::AdapterCallback, GetAdapterCallback https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:83: ASSERT_TRUE(adapter_.get() != nullptr); ASSERT_NE https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:102: ASSERT_TRUE(audio_sink_.get() != nullptr); ditto https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:121: int callback_count_; add a constructor to initialize these variables? https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:169: options.capabilities = std::vector<uint8_t>({}); {} isn't necessary. or just call options.capabilities.clear()
https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:14: const char kBluetoothAudioSinkUUID[] = "0000110b-0000-1000-8000-00805f9b34fb"; On 2015/01/28 15:54:27, Ben Chan wrote: > do you expect to have a TODO to move these constants somewhere? kBluetoothAudioSinkUUID will be placed in BluetoothMediaClient as a static const data memeber. The declaration of kBluetoothAudioSinkUUID will be removed from fake_bluetooth_media_client.cc and bluetooth_audio_sink_chromeos.cc. https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:59: if (properties.uuid != std::string(kBluetoothAudioSinkUUID) || On 2015/01/28 15:54:27, Ben Chan wrote: > EndpointProperties.uuid is a std::string, so std::string on the RHS isn't > necessary. Done. https://codereview.chromium.org/876153002/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:60: properties.codec != 0x00 || On 2015/01/28 15:54:27, Ben Chan wrote: > perhaps define 0x00 as something like kDefaultCodec or kCodecSBC? kDefaultCodec added. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:310: BluetoothAudioSinkChromeOS* audio_sink = new BluetoothAudioSinkChromeOS(this); On 2015/01/28 15:54:27, Ben Chan wrote: > It's not very obvious/clear whether the scoped_refptr of |audio_sink| ever > destroys |audio_sink| (due to ref count goes to zero) before Register returns, > so it's probably better to write: > > scoped_refptr<device::BluetoothAudioSink> audio_sink(new ...); > audio_sink->Register(... , audio_sink), error_callback); Done. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:17: const char kBluetoothAudioSinkUUID[] = "0000110b-0000-1000-8000-00805f9b34fb"; Removed. kBluetoothAudioSinkUUID is moved to BluetoothMediaClient. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:161: endpoint_properties.uuid = std::string(kBluetoothAudioSinkUUID); On 2015/01/28 15:54:27, Ben Chan wrote: > std::string isn't necessary as EndpointProperties.uuid is a std::string Done. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:251: const BluetoothAudioSink::ErrorCallback& error_back, On 2015/01/28 15:54:27, Ben Chan wrote: > error_back? s/error_back/error_callback https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:257: endpoint_path_ = dbus::ObjectPath(""); On 2015/01/28 15:54:27, Ben Chan wrote: > is dbus::ObjectPath necessary? The old path should be deprecated if the registration failed. Are you suggesting reusing the endpoint_path_? https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:43: // device::BluetoothAdapter::Observer override. On 2015/01/28 15:54:27, Ben Chan wrote: > this change is probably not necessary. see // BluetoothMediaClient::Observer > overrides below Done. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:94: void OnRegisterFailed(const BluetoothAudioSink::ErrorCallback& error_back, On 2015/01/28 15:54:27, Ben Chan wrote: > error_back? s/error_back/error_callback https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:76: base::Bind(&BluetoothAudioSinkChromeOSTest::AdapterCallback, On 2015/01/28 15:54:28, Ben Chan wrote: > GetAdapterCallback Done. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:83: ASSERT_TRUE(adapter_.get() != nullptr); On 2015/01/28 15:54:28, Ben Chan wrote: > ASSERT_NE Done. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:102: ASSERT_TRUE(audio_sink_.get() != nullptr); On 2015/01/28 15:54:28, Ben Chan wrote: > ditto Done. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:121: int callback_count_; On 2015/01/28 15:54:28, Ben Chan wrote: > add a constructor to initialize these variables? These variables can be initialized in SetUp() as well. I need SetUp() anyway, since DBusThreadManager::Initialize() may throw an exception. https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:169: options.capabilities = std::vector<uint8_t>({}); On 2015/01/28 15:54:28, Ben Chan wrote: > {} isn't necessary. or just call options.capabilities.clear() Done.
https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:257: endpoint_path_ = dbus::ObjectPath(""); On 2015/01/28 22:34:20, Miao wrote: > On 2015/01/28 15:54:27, Ben Chan wrote: > > is dbus::ObjectPath necessary? > > The old path should be deprecated if the registration failed. > Are you suggesting reusing the endpoint_path_? I was wondering if you can simply do: endpoint_path_ = "";
https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:257: endpoint_path_ = dbus::ObjectPath(""); On 2015/01/28 22:55:20, Ben Chan wrote: > On 2015/01/28 22:34:20, Miao wrote: > > On 2015/01/28 15:54:27, Ben Chan wrote: > > > is dbus::ObjectPath necessary? > > > > The old path should be deprecated if the registration failed. > > Are you suggesting reusing the endpoint_path_? > > I was wondering if you can simply do: > > endpoint_path_ = ""; It looks like the compiler complains about converting const char[] to const dbus::ObjectPath, so I guess the answer is no.
On 2015/01/28 23:41:59, Miao wrote: > https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... > File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): > > https://codereview.chromium.org/876153002/diff/20001/device/bluetooth/bluetoo... > device/bluetooth/bluetooth_audio_sink_chromeos.cc:257: endpoint_path_ = > dbus::ObjectPath(""); > On 2015/01/28 22:55:20, Ben Chan wrote: > > On 2015/01/28 22:34:20, Miao wrote: > > > On 2015/01/28 15:54:27, Ben Chan wrote: > > > > is dbus::ObjectPath necessary? > > > > > > The old path should be deprecated if the registration failed. > > > Are you suggesting reusing the endpoint_path_? > > > > I was wondering if you can simply do: > > > > endpoint_path_ = ""; > > It looks like the compiler complains about converting const char[] to const > dbus::ObjectPath, so I guess the answer is no. oh right, the dbus::ObjectPath(std::string) constructor is marked explicit.
https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... File chromeos/dbus/bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... chromeos/dbus/bluetooth_media_client.cc:44: const char BluetoothMediaClient::kBluetoothAudioSinkUUID[] = nit: All static variable assignments and method name should be preceded by "// static": // static const char BluetoothMediaClient::kBanana[] = "Banana"; https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... File chromeos/dbus/bluetooth_media_client.h (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... chromeos/dbus/bluetooth_media_client.h:95: // The UUID for Bluetooth audio sink service. This should be "The string representation for the 128-bit UUID for A2DP Sink" https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (left): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:46: error_callback.Run("org.bluez.NotImplemented", ""); Add a TODO here for making this do something meaningful. https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:14: const uint8_t kDefaultCodec = 0x00; I'd move this to the header as a static const class variable. https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.h (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.h:14: using dbus::ObjectPath; Don't use the "using" directive in header files. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (left): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:307: const device::BluetoothAudioSink::ErrorCallback& error_callback) { You should check here that adapter_->IsPresent is false and call error_callback. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.h (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.h:22: #include "device/bluetooth/bluetooth_audio_sink_chromeos.h" Is this include needed? I don't see that you're referring to BluetoothAudioSinkChromeOS anywhere. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.h:32: class BluetoothAudioSinkChromeOS; Is this forward declaration needed, especially since you're including bluetooth_audio_sink_chromeos.h above? https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:40: if (adapter_->IsPresent() && adapter_->IsPowered()) I would DCHECK(adapter_->IsPresent()), since it should never be false here. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:160: endpoint_properties.uuid = std::string( std::string() shouldn't be necessary afaik https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:258: media_endpoint_ = nullptr; Nice, this deletes the existing BluetoothMediaEndpointServiceProvider? https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:36: scoped_refptr<BluetoothAdapterChromeOS> adapter); Strange that this works. BluetoothAdapterChromeOS doesn't inherit from base::RefCounted<BluetoothAdapterChromeOS>. So, shouldn't this be a scoped_refptr<BluetoothAdapter> or is this OK somehow? https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:132: GetAdapter(); Call GetAdapter() from SetUp()? https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:159: options.capabilities = std::vector<uint8_t>({0x3f, 0xff, 0x12, 0x35}); You should be a bit more descriptive in the tests. So in this case, are you passing an invalid codec with valid capabilities? If so, say that in a comment. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:172: options.capabilities.clear(); Same as before, in a comment say that you're passing a valid codec with invalid capabilities.
https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... File chromeos/dbus/bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... chromeos/dbus/bluetooth_media_client.cc:44: const char BluetoothMediaClient::kBluetoothAudioSinkUUID[] = On 2015/01/29 04:22:55, armansito wrote: > nit: All static variable assignments and method name should be preceded by "// > static": > > // static > const char BluetoothMediaClient::kBanana[] = "Banana"; Done. https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... File chromeos/dbus/bluetooth_media_client.h (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/bluetooth_... chromeos/dbus/bluetooth_media_client.h:95: // The UUID for Bluetooth audio sink service. On 2015/01/29 04:22:55, armansito wrote: > This should be "The string representation for the 128-bit UUID for A2DP Sink" Done. https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (left): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:46: error_callback.Run("org.bluez.NotImplemented", ""); On 2015/01/29 04:22:55, armansito wrote: > Add a TODO here for making this do something meaningful. Done. https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:14: const uint8_t kDefaultCodec = 0x00; On 2015/01/29 04:22:55, armansito wrote: > I'd move this to the header as a static const class variable. Done. https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.h (right): https://codereview.chromium.org/876153002/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.h:14: using dbus::ObjectPath; On 2015/01/29 04:22:55, armansito wrote: > Don't use the "using" directive in header files. Removed. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.cc (left): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.cc:307: const device::BluetoothAudioSink::ErrorCallback& error_callback) { On 2015/01/29 04:22:55, armansito wrote: > You should check here that adapter_->IsPresent is false and call error_callback. Done. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_adapter_chromeos.h (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.h:22: #include "device/bluetooth/bluetooth_audio_sink_chromeos.h" On 2015/01/29 04:22:55, armansito wrote: > Is this include needed? I don't see that you're referring to > BluetoothAudioSinkChromeOS anywhere. Removed. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_adapter_chromeos.h:32: class BluetoothAudioSinkChromeOS; On 2015/01/29 04:22:55, armansito wrote: > Is this forward declaration needed, especially since you're including > bluetooth_audio_sink_chromeos.h above? Removed. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:40: if (adapter_->IsPresent() && adapter_->IsPowered()) On 2015/01/29 04:22:56, armansito wrote: > I would DCHECK(adapter_->IsPresent()), since it should never be false here. Right, since present is checked in BluetoothAdapterChromeOS::RegisterAudioSink(). https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:160: endpoint_properties.uuid = std::string( On 2015/01/29 04:22:56, armansito wrote: > std::string() shouldn't be necessary afaik Done. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:258: media_endpoint_ = nullptr; On 2015/01/29 04:22:55, armansito wrote: > Nice, this deletes the existing BluetoothMediaEndpointServiceProvider? // operator=. Allows assignment from a nullptr. Deletes the currently owned // object, if any. scoped_ptr& operator=(decltype(nullptr)) { reset(); return *this; } https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:36: scoped_refptr<BluetoothAdapterChromeOS> adapter); On 2015/01/29 04:22:56, armansito wrote: > Strange that this works. BluetoothAdapterChromeOS doesn't inherit from > base::RefCounted<BluetoothAdapterChromeOS>. So, shouldn't this be a > scoped_refptr<BluetoothAdapter> or is this OK somehow? It passed the compilation, and it looks like the unittest passed as well. But you're right. I should change it to the superclass. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:132: GetAdapter(); On 2015/01/29 04:22:56, armansito wrote: > Call GetAdapter() from SetUp()? Done. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:159: options.capabilities = std::vector<uint8_t>({0x3f, 0xff, 0x12, 0x35}); On 2015/01/29 04:22:56, armansito wrote: > You should be a bit more descriptive in the tests. So in this case, are you > passing an invalid codec with valid capabilities? If so, say that in a comment. Done. https://codereview.chromium.org/876153002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:172: options.capabilities.clear(); On 2015/01/29 04:22:56, armansito wrote: > Same as before, in a comment say that you're passing a valid codec with invalid > capabilities. Done.
lgtm with nits https://codereview.chromium.org/876153002/diff/60001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.h (right): https://codereview.chromium.org/876153002/diff/60001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.h:19: static const uint8_t kDefaultCodec = 0x00; nit: Don't make static variable assignments in the header file. https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:102: // The destructor should invoke Unregister() to ensure the audio sink will be nit: Say "The destructor invokes Unregister()". Saying it "should" makes it sound like your guessing :P https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:170: scoped_refptr<BluetoothAdapterChromeOS> adapter_chromeos( nit: no need to create a scoped_refptr here, just a raw pointer for the cast is sufficient. https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:159: // Sets options with a invalid codec and valid capabilities. nit: s/a invalid/an invalid/
https://codereview.chromium.org/876153002/diff/60001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.h (right): https://codereview.chromium.org/876153002/diff/60001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.h:19: static const uint8_t kDefaultCodec = 0x00; On 2015/01/30 01:27:46, armansito wrote: > nit: Don't make static variable assignments in the header file. Done. https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:102: // The destructor should invoke Unregister() to ensure the audio sink will be On 2015/01/30 01:27:46, armansito wrote: > nit: Say "The destructor invokes Unregister()". Saying it "should" makes it > sound like your guessing :P Done. https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:170: scoped_refptr<BluetoothAdapterChromeOS> adapter_chromeos( On 2015/01/30 01:27:46, armansito wrote: > nit: no need to create a scoped_refptr here, just a raw pointer for the cast is > sufficient. Done. https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/876153002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:159: // Sets options with a invalid codec and valid capabilities. On 2015/01/30 01:27:46, armansito wrote: > nit: s/a invalid/an invalid/ Done.
mcchou@chromium.org changed reviewers: + keybuk@chromium.org
+keybuk for owner review.
On 2015/01/30 21:51:29, Miao wrote: > +keybuk for owner review. You have an OWNER's rubberstamp already ;)
On 2015/01/30 22:00:50, armansito wrote: > On 2015/01/30 21:51:29, Miao wrote: > > +keybuk for owner review. > > You have an OWNER's rubberstamp already ;) Oh, I thought I need the review from one of the owners in /device/OWNERS.
On 2015/01/30 22:00:50, armansito wrote: > On 2015/01/30 21:51:29, Miao wrote: > > +keybuk for owner review. > > You have an OWNER's rubberstamp already ;) Oh, I thought I need the review from one of the owners in /device/OWNERS.
The CQ bit was checked by mcchou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/876153002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/876153002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/876153002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:26: class BluetoothAudioSink : public base::RefCounted<BluetoothAudioSink> { you probably need to mark this class with DEVICE_BLUETOOTH_EXPORT if it's being used elsewhere via libdevice_bluetooth.so
https://codereview.chromium.org/876153002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/876153002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:26: class BluetoothAudioSink : public base::RefCounted<BluetoothAudioSink> { On 2015/02/02 19:43:35, Ben Chan wrote: > you probably need to mark this class with DEVICE_BLUETOOTH_EXPORT if it's being > used elsewhere via libdevice_bluetooth.so Done.
The CQ bit was checked by mcchou@chromium.org
The CQ bit was unchecked by mcchou@chromium.org
The CQ bit was checked by mcchou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/876153002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by mcchou@chromium.org
The CQ bit was unchecked by mcchou@chromium.org
The CQ bit was checked by mcchou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/876153002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6cf69f3915ec2b0972f36161958d10642f3d27c9 Cr-Commit-Position: refs/heads/master@{#314244} |
