Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1132943002:
bluetooth: Move mock creation out of BluetoothDispatcherHost to LayoutTestBluetoothAdapterProvider. (Closed)
|
Created:
5 years, 7 months ago by ortuno Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth-testing-layout-tests Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Move mock creation out of BluetoothDispatcherHost to
LayoutTestBluetoothAdapterProvider.
This patch doesn't add any new tests.
Split for easier reviewing. This is the last of two patches to remove
testing from BluetoothDispatcherHost and BluetoothDispatcher:
[1] http://crrev.com/1125133005
[2] This patch.
BUG=436284
Committed: https://crrev.com/fd24faac014bc95571dbfb51bfd043c390dd6cd4
Cr-Commit-Position: refs/heads/master@{#330748}
Patch Set 1 : #
Total comments: 45
Patch Set 2 : Removed unused constants #Patch Set 3 : Address @scheib comments. #
Total comments: 6
Patch Set 4 : Adding whitespace for easier readability. #Patch Set 5 : Address scheib comments #
Total comments: 17
Patch Set 6 : Remove member variables. #
Total comments: 8
Patch Set 7 : Changed new'ed variables to scoped_ptr. #
Total comments: 2
Patch Set 8 : Change release() to pass() #Patch Set 9 : Move testing out of browser context. #
Total comments: 2
Patch Set 10 : Update documentation. #
Total comments: 8
Patch Set 11 : Fixes based on comments #Patch Set 12 : Add non const method #
Total comments: 4
Patch Set 13 : Address armansito's comments #Patch Set 14 : Merged with previous patch #Patch Set 15 : Fix API tests failing #Messages
Total messages: 50 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
scheib@chromium.org changed reviewers: + scheib@chromium.org
https://codereview.chromium.org/1132943002/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:20: const uint32 kUnspecifiedDeviceClass = Delete constant here, it is now unused. https://codereview.chromium.org/1132943002/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:112: browser_context_->GetBluetoothAdapterForTesting( Whitespace typo, run git cl format. https://codereview.chromium.org/1132943002/diff/100001/content/content_shell.... File content/content_shell.gypi (right): https://codereview.chromium.org/1132943002/diff/100001/content/content_shell.... content/content_shell.gypi:5: { Update src/content/shell/BUILD.gn as well. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:18: LayoutTestBluetoothAdapterProvider::~LayoutTestBluetoothAdapterProvider() { Insert blank line above. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:28: class LayoutTestBluetoothAdapterProvider { Class comment, along the lines of: Implements fake adapters with named mock data sets for use in tests as a result of layout tests calling testRunner.setBluetoothMockDataSet. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:32: void GetBluetoothAdapter( Same comment as used in BrowserContext. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:41: /* Use // comment style for consistency. Though /* */ is style compliant it's not used in chromium code. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Comment_Style https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:44: - |StartDiscoverySession| invokes |SuccessfulDiscoverySession| Add periods to all sentence fragments comments in this file. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:53: - |GetDevices| returns an list with an |Empty Device| "returns a list". https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:81: - |GetUUIDs| returns a list with two UUIDs: |uuid1| and |uuid2| I like these method docs. At some point we'll move test data into the spec (or an companion spec) and from here want to reference that and probably copy text in a similar way to how we write that up. That text will need to contain all information about how the fake data set will behave, so I'd like to have all literal values exist in either comments here or in literal values declared in this file. In cases where one-off literal values are used, we should document the value "1F00" meaning "Unspecified Device Class". If values are being used multiple times, let's move them to constants declared in this file e.g. LayoutTestBluetoothAdapterProvider::kUnspecifiedDeviceClass. I think it's OK to provide the value definition in this header when possible as well to have improved readability of what the test data sets are without the indirection of going to the cc file. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:83: void SetUpMockEmptyDevice(MockBluetoothAdapter* adapter); Everything is a mock in this class, and 'Setup' is a word, so let's just use SetupEmptyDevice. Insert empty line after method and data members. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:84: scoped_ptr<BluetoothUUID> uuid1; FYI Naming style of members has an underscore suffix, but: No need to have the UUIDs be member variables I think, just push_back(BluetoothUUID("1234")).
https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:9: Add using statements to simplify code below, such as: using testing::Invoke; using testing::Return; using testing::NiceMock; using testing::_; https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:36: NOTREACHED(); "if one part of an if-else statement uses curly braces, the other part must too:" http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:41: testing::NiceMock<MockBluetoothAdapter>* mock_adapter_ = Let's keep a scoped_refptr in this class to the adapter, but as written here with a local variable the suffix '_' is incorrect. Later in Setup* calls instead of passing the adapter we can just reference the member variable. Rename mock_adapter_ to adapter_ to keep things as simple as possible, this whole class is in the context of providing mock / fake objects. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:45: testing::Invoke(this, &LayoutTestBluetoothAdapterProvider:: The LayoutTestBluetoothAdapterProvider:: is redundant I believe, as this code is already in that name scope. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:49: callback.Run(scoped_refptr<BluetoothAdapter>(mock_adapter_)); These should then be able to collapse to just callback.Run(adapter_); https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:82: empty_device_.reset(empty_device); Just set the device member at the top of this method when it is created. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:40: private: We can leave everything in protected, move the data member down to reduce sections. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:41: /* Lead these comment blocks with the fake_adapter_name identifier string, something like; // Provides "EmptyAdapter" fake BluetoothAdapter to |callback| with the // following characteristics: // ... https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:53: - |GetDevices| returns an list with an |Empty Device| In the fake adapter names you've gone with a CamelCase name identifier, do the same with EmptyDevice for consistency. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:63: void SuccessfulDiscoverySessionCallback( How about just "SuccessfulDiscoverySession". https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:67: // Calls |callback| Just merge this method up to be covered by previous method comment. Rename SuccessfulDiscoverySessionStop.
Patchset #3 (id:140001) has been deleted
@scheib: Ready for review again. https://codereview.chromium.org/1132943002/diff/100001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:112: browser_context_->GetBluetoothAdapterForTesting( On 2015/05/11 at 20:19:26, scheib wrote: > Whitespace typo, run git cl format. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:9: On 2015/05/12 at 01:16:28, scheib wrote: > Add using statements to simplify code below, such as: > using testing::Invoke; > using testing::Return; > using testing::NiceMock; > using testing::_; Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:18: LayoutTestBluetoothAdapterProvider::~LayoutTestBluetoothAdapterProvider() { On 2015/05/11 at 20:19:26, scheib wrote: > Insert blank line above. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:36: NOTREACHED(); On 2015/05/12 at 01:16:28, scheib wrote: > "if one part of an if-else statement uses curly braces, the other part must too:" http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:41: testing::NiceMock<MockBluetoothAdapter>* mock_adapter_ = On 2015/05/12 at 01:16:28, scheib wrote: > Let's keep a scoped_refptr in this class to the adapter, but as written here with a local variable the suffix '_' is incorrect. Later in Setup* calls instead of passing the adapter we can just reference the member variable. > > Rename mock_adapter_ to adapter_ to keep things as simple as possible, this whole class is in the context of providing mock / fake objects. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:45: testing::Invoke(this, &LayoutTestBluetoothAdapterProvider:: On 2015/05/12 at 01:16:28, scheib wrote: > The LayoutTestBluetoothAdapterProvider:: is redundant I believe, as this code is already in that name scope. It is not :( https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:49: callback.Run(scoped_refptr<BluetoothAdapter>(mock_adapter_)); On 2015/05/12 at 01:16:27, scheib wrote: > These should then be able to collapse to just callback.Run(adapter_); Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:82: empty_device_.reset(empty_device); On 2015/05/12 at 01:16:28, scheib wrote: > Just set the device member at the top of this method when it is created. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:28: class LayoutTestBluetoothAdapterProvider { On 2015/05/11 at 20:19:26, scheib wrote: > Class comment, along the lines of: > Implements fake adapters with named mock data sets for use in tests as a result of layout tests calling testRunner.setBluetoothMockDataSet. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:32: void GetBluetoothAdapter( On 2015/05/11 at 20:19:26, scheib wrote: > Same comment as used in BrowserContext. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:40: private: On 2015/05/12 at 01:16:28, scheib wrote: > We can leave everything in protected, move the data member down to reduce sections. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:41: /* On 2015/05/12 at 01:16:28, scheib wrote: > Lead these comment blocks with the fake_adapter_name identifier string, something like; > > // Provides "EmptyAdapter" fake BluetoothAdapter to |callback| with the > // following characteristics: > // ... Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:44: - |StartDiscoverySession| invokes |SuccessfulDiscoverySession| On 2015/05/11 at 20:19:26, scheib wrote: > Add periods to all sentence fragments comments in this file. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:53: - |GetDevices| returns an list with an |Empty Device| On 2015/05/12 at 01:16:28, scheib wrote: > In the fake adapter names you've gone with a CamelCase name identifier, do the same with EmptyDevice for consistency. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:63: void SuccessfulDiscoverySessionCallback( On 2015/05/12 at 01:16:28, scheib wrote: > How about just "SuccessfulDiscoverySession". Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:67: // Calls |callback| On 2015/05/12 at 01:16:28, scheib wrote: > Just merge this method up to be covered by previous method comment. Rename SuccessfulDiscoverySessionStop. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:81: - |GetUUIDs| returns a list with two UUIDs: |uuid1| and |uuid2| On 2015/05/11 at 20:19:26, scheib wrote: > I like these method docs. At some point we'll move test data into the spec (or an companion spec) and from here want to reference that and probably copy text in a similar way to how we write that up. That text will need to contain all information about how the fake data set will behave, so I'd like to have all literal values exist in either comments here or in literal values declared in this file. > > In cases where one-off literal values are used, we should document the value "1F00" meaning "Unspecified Device Class". > > If values are being used multiple times, let's move them to constants declared in this file e.g. LayoutTestBluetoothAdapterProvider::kUnspecifiedDeviceClass. I think it's OK to provide the value definition in this header when possible as well to have improved readability of what the test data sets are without the indirection of going to the cc file. Moved everything inline since we aren't using anything more than once. As I write more tests I will turn them into constants. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:83: void SetUpMockEmptyDevice(MockBluetoothAdapter* adapter); On 2015/05/11 at 20:19:26, scheib wrote: > Everything is a mock in this class, and 'Setup' is a word, so let's just use SetupEmptyDevice. > Insert empty line after method and data members. Done. https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:84: scoped_ptr<BluetoothUUID> uuid1; On 2015/05/11 at 20:19:26, scheib wrote: > FYI Naming style of members has an underscore suffix, but: No need to have the UUIDs be member variables I think, just push_back(BluetoothUUID("1234")). Done.
LGTM, with small fixes: https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:48: callback.Run(NULL); We start off with a real adapter, and then can enable fakes, so it feels right to me to allow "" to restore the real adapter. https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:80: adapter_.get(), 0x1F00, "Empty Mock Device name", Comment the 0x1F00 literal with parameter name. https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:66: Has extra blank line here.
Thanks! https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:48: callback.Run(NULL); On 2015/05/12 at 20:15:06, scheib wrote: > We start off with a real adapter, and then can enable fakes, so it feels right to me to allow "" to restore the real adapter. Done. https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:80: adapter_.get(), 0x1F00, "Empty Mock Device name", On 2015/05/12 at 20:15:06, scheib wrote: > Comment the 0x1F00 literal with parameter name. Done. https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/160001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:66: On 2015/05/12 at 20:15:06, scheib wrote: > Has extra blank line here. Done.
jyasskin@chromium.org changed reviewers: + jyasskin@chromium.org
In the change description, you should say "Move testing from BluetoothDispatcherHost to Xyz", not just "Move testing out of BluetoothDispatcherHost" https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... File content/content_shell.gypi (right): https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... content/content_shell.gypi:54: '../device/bluetooth/bluetooth.gyp:device_bluetooth_mocks', Call these "fakes" or "stubs" unless they're actually mocks: http://xunitpatterns.com/Mocks,%20Fakes,%20Stubs%20and%20Dummies.html, http://martinfowler.com/articles/mocksArentStubs.html#TheDifferenceBetweenMoc... https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... content/content_shell.gypi:67: '../testing/gmock.gyp:gmock', I'm worried about including gtest & gmock in a non-testing library like content_shell. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:58: adapter_ = new NiceMock<MockBluetoothAdapter>(); Why do you stash the adapter in the Provider? It seems like the callback should keep it alive long enough, and if that's not true, then stashing it would break as soon as two tests try to get the adapter at the same time. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:81: SetupEmptyDevice(); Definitely pass the adapter into this function as an argument, and return the new device as a return value, instead of passing them through data members of *this. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:29: // Provides "EmptyAdapter" fake BluetoothAdapter to |callback| with following s/Provides "EmptyAdapter"/Provides the "emptyAdapter"/ https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:36: // Provides "SingleEmptyDevice" fake BluetoothAdapter to |callback with the s/|callback/|callback|/ https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:38: // - |StartDiscoverySession| invokes |SuccessfulDiscoverySessionCallback|. There's no "SuccessfulDiscoverySessionCallback" here. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:45: // - |Stop| will invoke |SuccessfulStopDiscoverySession|. Is SuccessfulStopDiscoverySession() different from SuccessfulDiscoverySessionStop()?
https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:41: testing::NiceMock<MockBluetoothAdapter>* mock_adapter_ = On 2015/05/12 01:16:28, scheib wrote: > Let's keep a scoped_refptr in this class to the adapter, but as written here > with a local variable the suffix '_' is incorrect. Later in Setup* calls instead > of passing the adapter we can just reference the member variable. > > Rename mock_adapter_ to adapter_ to keep things as simple as possible, this > whole class is in the context of providing mock / fake objects. We shouldn't stash single-function-call objects into member variables unless there's a reason to do it. Is there a reason here?
https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:41: testing::NiceMock<MockBluetoothAdapter>* mock_adapter_ = On 2015/05/12 22:37:49, Jeffrey Yasskin wrote: > On 2015/05/12 01:16:28, scheib wrote: > > Let's keep a scoped_refptr in this class to the adapter, but as written here > > with a local variable the suffix '_' is incorrect. Later in Setup* calls > instead > > of passing the adapter we can just reference the member variable. > > > > Rename mock_adapter_ to adapter_ to keep things as simple as possible, this > > whole class is in the context of providing mock / fake objects. > > We shouldn't stash single-function-call objects into member variables unless > there's a reason to do it. Is there a reason here? The reason I discussed with ortuno in person was to reduce code verbosity with parameter passing. Everything in this class is and will be designed to coordinate to create a set of fakes for the single data set under test. The already dense test code seems easier to read to me to have the objects stored as members in a single line and referenced directly as needed. This is a weak preference though, so if you feel strongly then it's fine to marshal objects around.
Patchset #6 (id:210001) has been deleted
@jyassking: Ready for review again. https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... File content/content_shell.gypi (right): https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... content/content_shell.gypi:54: '../device/bluetooth/bluetooth.gyp:device_bluetooth_mocks', On 2015/05/12 at 22:17:53, Jeffrey Yasskin wrote: > Call these "fakes" or "stubs" unless they're actually mocks: http://xunitpatterns.com/Mocks,%20Fakes,%20Stubs%20and%20Dummies.html, http://martinfowler.com/articles/mocksArentStubs.html#TheDifferenceBetweenMoc... We don't have any control over those. They were written by someone else. https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... content/content_shell.gypi:67: '../testing/gmock.gyp:gmock', On 2015/05/12 at 22:17:53, Jeffrey Yasskin wrote: > I'm worried about including gtest & gmock in a non-testing library like content_shell. We talked to @jam and he was OK with this. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:58: adapter_ = new NiceMock<MockBluetoothAdapter>(); On 2015/05/12 at 22:17:54, Jeffrey Yasskin wrote: > Why do you stash the adapter in the Provider? It seems like the callback should keep it alive long enough, and if that's not true, then stashing it would break as soon as two tests try to get the adapter at the same time. Done. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:81: SetupEmptyDevice(); On 2015/05/12 at 22:17:54, Jeffrey Yasskin wrote: > Definitely pass the adapter into this function as an argument, and return the new device as a return value, instead of passing them through data members of *this. Done. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:29: // Provides "EmptyAdapter" fake BluetoothAdapter to |callback| with following On 2015/05/12 at 22:17:54, Jeffrey Yasskin wrote: > s/Provides "EmptyAdapter"/Provides the "emptyAdapter"/ Done. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:36: // Provides "SingleEmptyDevice" fake BluetoothAdapter to |callback with the On 2015/05/12 at 22:17:54, Jeffrey Yasskin wrote: > s/|callback/|callback|/ Done. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:38: // - |StartDiscoverySession| invokes |SuccessfulDiscoverySessionCallback|. On 2015/05/12 at 22:17:54, Jeffrey Yasskin wrote: > There's no "SuccessfulDiscoverySessionCallback" here. Outdated docs. Done. https://codereview.chromium.org/1132943002/diff/200001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:45: // - |Stop| will invoke |SuccessfulStopDiscoverySession|. On 2015/05/12 at 22:17:54, Jeffrey Yasskin wrote: > Is SuccessfulStopDiscoverySession() different from SuccessfulDiscoverySessionStop()? Done.
https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/100001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:41: testing::NiceMock<MockBluetoothAdapter>* mock_adapter_ = On 2015/05/13 04:11:05, scheib wrote: > On 2015/05/12 22:37:49, Jeffrey Yasskin wrote: > > On 2015/05/12 01:16:28, scheib wrote: > > > Let's keep a scoped_refptr in this class to the adapter, but as written here > > > with a local variable the suffix '_' is incorrect. Later in Setup* calls > > instead > > > of passing the adapter we can just reference the member variable. > > > > > > Rename mock_adapter_ to adapter_ to keep things as simple as possible, this > > > whole class is in the context of providing mock / fake objects. > > > > We shouldn't stash single-function-call objects into member variables unless > > there's a reason to do it. Is there a reason here? > > The reason I discussed with ortuno in person was to reduce code verbosity with > parameter passing. Everything in this class is and will be designed to > coordinate to create a set of fakes for the single data set under test. The > already dense test code seems easier to read to me to have the objects stored as > members in a single line and referenced directly as needed. This is a weak > preference though, so if you feel strongly then it's fine to marshal objects > around. Yeah, even if it increases the size of a line or two, it's easier to read data flow that's all local than flow that goes through class variables. When I see the use of a member variable, it makes me want to know what else it's used for, which requires more work. https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... File content/content_shell.gypi (right): https://codereview.chromium.org/1132943002/diff/200001/content/content_shell.... content/content_shell.gypi:67: '../testing/gmock.gyp:gmock', On 2015/05/13 17:14:49, ortuno wrote: > On 2015/05/12 at 22:17:53, Jeffrey Yasskin wrote: > > I'm worried about including gtest & gmock in a non-testing library like > content_shell. > > We talked to @jam and he was OK with this. Sounds good. That also deals with your worry that people might not accept the use of gmock. https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:58: NiceMock<MockBluetoothAdapter>* adapter = You should almost always catch a new'ed pointer into a smart pointer, a scoped_refptr<NiceMock<MockBluetoothAdapter>> in this case. https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:82: std::vector<MockBluetoothDevice*> devices; Similarly here, the vector<> should hold scoped_ptr<MockBluetoothDevice>s, or you should use a ScopedVector and .Pass() it to SetMockDevices. https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:84: adapter->SetMockDevices(devices); I'd probably have this accept one device at a time, so adapter->AddMockDevice(GetEmptyDevice()). I don't think you'd ever need to mutate an adapter to remove its devices. In general, either have functions take their parameters by const&, or std::move() or .Pass() arguments into them. In this case, if you really want to pass all the devices at once, use ScopedVector with .Pass(). https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:91: NiceMock<MockBluetoothDevice>* Return a scoped_ptr<NiceMock<MockBluetoothDevice>> instead of an unowned pointer. You may need to use .PassAs<MockBluetoothDevice>() in some places to do conversions.
Patchset #6 (id:230001) has been deleted
Patchset #6 (id:250001) has been deleted
Patchset #7 (id:290001) has been deleted
@jyasskin: Ready for review again! https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:58: NiceMock<MockBluetoothAdapter>* adapter = On 2015/05/13 at 19:40:50, Jeffrey Yasskin wrote: > You should almost always catch a new'ed pointer into a smart pointer, a scoped_refptr<NiceMock<MockBluetoothAdapter>> in this case. Done. https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:82: std::vector<MockBluetoothDevice*> devices; On 2015/05/13 at 19:40:51, Jeffrey Yasskin wrote: > Similarly here, the vector<> should hold scoped_ptr<MockBluetoothDevice>s, or you should use a ScopedVector and .Pass() it to SetMockDevices. Changed MockBluetoothAdapter to accept a single device. https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:84: adapter->SetMockDevices(devices); On 2015/05/13 at 19:40:51, Jeffrey Yasskin wrote: > I'd probably have this accept one device at a time, so adapter->AddMockDevice(GetEmptyDevice()). I don't think you'd ever need to mutate an adapter to remove its devices. > > In general, either have functions take their parameters by const&, or std::move() or .Pass() arguments into them. In this case, if you really want to pass all the devices at once, use ScopedVector with .Pass(). Done. https://codereview.chromium.org/1132943002/diff/270001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:91: NiceMock<MockBluetoothDevice>* On 2015/05/13 at 19:40:51, Jeffrey Yasskin wrote: > Return a scoped_ptr<NiceMock<MockBluetoothDevice>> instead of an unowned pointer. You may need to use .PassAs<MockBluetoothDevice>() in some places to do conversions. Done.
@jyasskin: Ready for review again!
Thanks, LGTM! https://codereview.chromium.org/1132943002/diff/310001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_adapter.cc (right): https://codereview.chromium.org/1132943002/diff/310001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_adapter.cc:57: mock_devices_.push_back(mock_device.release()); You could use .Pass() here, but it doesn't matter much.
Thanks! https://codereview.chromium.org/1132943002/diff/310001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_adapter.cc (right): https://codereview.chromium.org/1132943002/diff/310001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_adapter.cc:57: mock_devices_.push_back(mock_device.release()); On 2015/05/13 at 21:44:36, Jeffrey Yasskin wrote: > You could use .Pass() here, but it doesn't matter much. Done.
ortuno@chromium.org changed reviewers: + jam@chromium.org
@jam: PTAL
@jyasskin: We changed the approach in previous patches so I had to do some refactoring. PTAL
Still lgtm. https://codereview.chromium.org/1132943002/diff/350001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/350001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:21: static scoped_refptr<device::BluetoothAdapter> GetBluetoothAdapter( Yay for fewer callbacks!
Still lgtm.
@scheib: PTAL https://codereview.chromium.org/1132943002/diff/350001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1132943002/diff/350001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:21: static scoped_refptr<device::BluetoothAdapter> GetBluetoothAdapter( On 2015/05/15 at 23:38:01, Jeffrey Yasskin wrote: > Yay for fewer callbacks! Ah forgot to update the documentation. Done.
LGTM with small fixes: https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:102: void BluetoothDispatcherHost::SetBluetoothAdapterForTesting( Move to match declaration order. https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:37: void SetBluetoothAdapterForTesting( Make this match the signature https://codereview.chromium.org/1125133005/ https://codereview.chromium.org/1132943002/diff/370001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/370001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:30: scoped_refptr<BluetoothAdapter> It's common, and I think useful, to add "// static" comment line above static methods. https://codereview.chromium.org/1132943002/diff/370001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/1132943002/diff/370001/content/test/layouttes... content/test/layouttest_support.cc:317: void SetBluetoothAdapter(int render_process_id, Only change signature once here and https://codereview.chromium.org/1125133005
lgtm
https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.h (right): https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.h:37: void SetBluetoothAdapterForTesting( On 2015/05/16 02:39:14, scheib wrote: > Make this match the signature https://codereview.chromium.org/1125133005/ Sorry, my miss-reading here. Fine to have this signature.
https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1132943002/diff/370001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:102: void BluetoothDispatcherHost::SetBluetoothAdapterForTesting( On 2015/05/16 at 02:39:14, scheib wrote: > Move to match declaration order. Done. https://codereview.chromium.org/1132943002/diff/370001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1132943002/diff/370001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:30: scoped_refptr<BluetoothAdapter> On 2015/05/16 at 02:39:14, scheib wrote: > It's common, and I think useful, to add "// static" comment line above static methods. Done. https://codereview.chromium.org/1132943002/diff/370001/content/test/layouttes... File content/test/layouttest_support.cc (right): https://codereview.chromium.org/1132943002/diff/370001/content/test/layouttes... content/test/layouttest_support.cc:317: void SetBluetoothAdapter(int render_process_id, On 2015/05/16 at 02:39:14, scheib wrote: > Only change signature once here and https://codereview.chromium.org/1125133005 Needs to change twice.
ortuno@chromium.org changed reviewers: + armansito@chromium.org
@armansito: PTAL at new DEPS and mock_bluetooth_adapter.h/cc. This change adds a new method to add mock devices to a MockBluetoothAdapter. That way the adapter can manage the life of BluetoothDevices directly.
lgtm with nits https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_adapter.cc (right): https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_adapter.cc:57: it != mock_devices_.end(); ++it) { nit: for (auto& it : mock_devices_) https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_adapter.h (right): https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_adapter.h:106: // This methods takes ownership of the BluetoothDevices. I would mention here that this is only for convenience as far testing is concerned and that it is possible to write test cases without this logic.
Thanks! https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_adapter.cc (right): https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_adapter.cc:57: it != mock_devices_.end(); ++it) { On 2015/05/19 at 20:18:33, armansito wrote: > nit: for (auto& it : mock_devices_) Done. https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_adapter.h (right): https://codereview.chromium.org/1132943002/diff/410001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_adapter.h:106: // This methods takes ownership of the BluetoothDevices. On 2015/05/19 at 20:18:33, armansito wrote: > I would mention here that this is only for convenience as far testing is concerned and that it is possible to write test cases without this logic. Done.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, jam@chromium.org, scheib@chromium.org, armansito@chromium.org Link to the patchset: https://codereview.chromium.org/1132943002/#ps430001 (title: "Address armansito's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132943002/430001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, armansito@chromium.org, jam@chromium.org, scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1132943002/#ps470001 (title: "Fix API tests failing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132943002/470001
Message was sent while issue was closed.
Committed patchset #15 (id:470001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/fd24faac014bc95571dbfb51bfd043c390dd6cd4 Cr-Commit-Position: refs/heads/master@{#330748} |
