Expose SetDiscoveryFilter from BluetoothEventRouter

Expose SetDiscoveryFilter from BluetoothEventRouter

This patch makes it possible to control BluetoothDiscoveryFilter assigned to discovery sessions through BluetoothEventRouter router.

BUG=407773
R=armansito@chromium.org

Committed: https://crrev.com/b51933daebb2453735df35a21519d7c4439c25fc
Cr-Commit-Position: refs/heads/master@{#325557}

[armansito, 2015-04-16 19:13:55]

https://codereview.chromium.org/1083163002/diff/1/device/bluetooth/test/mock_...
File device/bluetooth/test/mock_bluetooth_adapter.h (right):

https://codereview.chromium.org/1083163002/diff/1/device/bluetooth/test/mock_...
device/bluetooth/test/mock_bluetooth_adapter.h:93: void
StartDiscoverySessionWithFilter(
Why not just MOCK_METHOD here? Why do you need the raw one?

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right):

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:93: 
Add comments explaining what's going on here.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:102:
pre_set_filter_map_.erase(pre_set_iter);
Do an early return and drop 'else'.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:137: VLOG(1) <<
__func__;
Prefer DVLOG

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:146: VLOG(1) << "No
active discovery session exists for extension.";
This may be interpreted incorrectly as an error case in the logs, so I would
explain more:

"No active discovery session exists for extension, so caching filter for later
use." or something along those.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:146: VLOG(1) << "No
active discovery session exists for extension.";
DVLOG

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:369: it !=
pre_set_filter_map_.end(); it++) {
for (auto& it : pre_set_filter_map_)

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:400: const
base::Closure& callback) {
DVLOG success?

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc
(right):

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:97: // This
test check that calling SetDiscoveryFitler before StartDiscoverySession
nit: s/SetDiscoveryFitler/SetDiscoveryFilter/

Note: Fitler sounds like Hitler.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:123:
EXPECT_CALL(*mock_adapter_, RemoveObserver(testing::_)).Times(1);
You'll also want test cases to bluetooth_private_apitest.cc that exercise the
actual JS code using your mocks.

[jpawlowski1, 2015-04-16 19:36:28]

https://codereview.chromium.org/1083163002/diff/1/device/bluetooth/test/mock_...
File device/bluetooth/test/mock_bluetooth_adapter.h (right):

https://codereview.chromium.org/1083163002/diff/1/device/bluetooth/test/mock_...
device/bluetooth/test/mock_bluetooth_adapter.h:93: void
StartDiscoverySessionWithFilter(
On 2015/04/16 19:13:54, armansito wrote:
> Why not just MOCK_METHOD here? Why do you need the raw one?

Because it takes scoped_ptr as argument, and that's the only workaround I'm
aware of, here's discussion about that:
https://groups.google.com/a/google.com/forum/#!topic/gmock-users/JtZd5hBw_qg

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right):

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:93: 
On 2015/04/16 19:13:54, armansito wrote:
> Add comments explaining what's going on here.

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:102:
pre_set_filter_map_.erase(pre_set_iter);
On 2015/04/16 19:13:55, armansito wrote:
> Do an early return and drop 'else'.

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:137: VLOG(1) <<
__func__;
On 2015/04/16 19:13:54, armansito wrote:
> Prefer DVLOG

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:146: VLOG(1) << "No
active discovery session exists for extension.";
On 2015/04/16 19:13:55, armansito wrote:
> This may be interpreted incorrectly as an error case in the logs, so I would
> explain more:
> 
> "No active discovery session exists for extension, so caching filter for later
> use." or something along those.

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:146: VLOG(1) << "No
active discovery session exists for extension.";
On 2015/04/16 19:13:55, armansito wrote:
> DVLOG

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:369: it !=
pre_set_filter_map_.end(); it++) {
On 2015/04/16 19:13:54, armansito wrote:
> for (auto& it : pre_set_filter_map_)

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:400: const
base::Closure& callback) {
On 2015/04/16 19:13:55, armansito wrote:
> DVLOG success?

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc
(right):

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:97: // This
test check that calling SetDiscoveryFitler before StartDiscoverySession
On 2015/04/16 19:13:55, armansito wrote:
> nit: s/SetDiscoveryFitler/SetDiscoveryFilter/
> 
> Note: Fitler sounds like Hitler.

Done.

https://codereview.chromium.org/1083163002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth/bluetooth_event_router_unittest.cc:123:
EXPECT_CALL(*mock_adapter_, RemoveObserver(testing::_)).Times(1);
On 2015/04/16 19:13:55, armansito wrote:
> You'll also want test cases to bluetooth_private_apitest.cc that exercise the
> actual JS code using your mocks.

this code is not yet connected to bluetooth private api. Next CL will calling
EventRouter from bluetooth_private_api together with tests.

[armansito, 2015-04-16 21:24:03]

lgtm with nits

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right):

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:94: // Check wether
user pre set discovery filter by calling SetDiscoveryFilter
nit: s/wether/whether/

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:94: // Check wether
user pre set discovery filter by calling SetDiscoveryFilter
A little more concise: "If the user has set a discovery filter then start a
filtered discovery session, otherwise start a regular session".

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:402: DVLOG(1) <<
"Succesfully set DiscoveryFilter.";
nit: s/Succesfully/Successfully/

[jpawlowski1, 2015-04-16 21:30:36]

The CQ bit was checked by jpawlowski@chromium.org

[jpawlowski1, 2015-04-16 21:30:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1083163002/#ps40001
(title: "Nits fixes")

[commit-bot: I haz the power, 2015-04-16 21:31:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1083163002/40001

[jpawlowski1, 2015-04-16 22:00:02]

The CQ bit was unchecked by jpawlowski@chromium.org

[jpawlowski1, 2015-04-16 22:03:59]

The CQ bit was checked by jpawlowski@chromium.org

[jpawlowski1, 2015-04-16 22:03:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1083163002/#ps60001
(title: "Windows build fix")

[commit-bot: I haz the power, 2015-04-16 22:09:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1083163002/60001

[jpawlowski1, 2015-04-16 22:32:47]

The CQ bit was unchecked by jpawlowski@chromium.org

[jpawlowski1, 2015-04-16 22:36:07]

The CQ bit was checked by jpawlowski@chromium.org

[jpawlowski1, 2015-04-16 22:36:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1083163002/#ps80001
(title: "Another windows build fix")

[commit-bot: I haz the power, 2015-04-16 22:39:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1083163002/80001

[commit-bot: I haz the power, 2015-04-16 23:58:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-16 23:59:45]

Patchset 5 (id:??) landed as
https://crrev.com/b51933daebb2453735df35a21519d7c4439c25fc
Cr-Commit-Position: refs/heads/master@{#325557}

[jpawlowski1, 2015-04-17 04:48:14]

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_event_router.cc (right):

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:94: // Check wether
user pre set discovery filter by calling SetDiscoveryFilter
On 2015/04/16 21:24:02, armansito wrote:
> nit: s/wether/whether/

Done.

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:94: // Check wether
user pre set discovery filter by calling SetDiscoveryFilter
On 2015/04/16 21:24:02, armansito wrote:
> A little more concise: "If the user has set a discovery filter then start a
> filtered discovery session, otherwise start a regular session".
> 

Done.

https://codereview.chromium.org/1083163002/diff/20001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_event_router.cc:402: DVLOG(1) <<
"Succesfully set DiscoveryFilter.";
On 2015/04/16 21:24:02, armansito wrote:
> nit: s/Succesfully/Successfully/

Done.