bluetooth: Remove indicator only when there are no more devices connected

bluetooth: Remove indicator only when there are no more devices connected

Before, if multiple devices were connected and we disconnected from one
we would remove the indicator even though a device could still be connected.

BUG=599281
Committed: https://crrev.com/df4d79807643681b9bff15aa6dd7558230aed583
Cr-Commit-Position: refs/heads/master@{#385942}

[ortuno, 2016-03-30 23:21:12]

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

[ortuno, 2016-03-30 23:21:12]

jyasskin: PTAL

[ortuno, 2016-03-30 23:21:16]

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

[commit-bot: I haz the power, 2016-03-30 23:21:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848663002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848663002/20001

[commit-bot: I haz the power, 2016-03-31 01:02:59]

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

[commit-bot: I haz the power, 2016-03-31 01:03:00]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-03-31 16:18:43]

https://codereview.chromium.org/1848663002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:732: if
(!device_id_to_connection_map_.empty()) {
Currently, this is going to keep the indicator showing if any frame from the
process is connected, right? This is better than what we had, but we really
ought to maintain the indicator if there are any connections from the current
WebContents, and drop it if there are connections from other WebContents that
happen to share a process.

https://codereview.chromium.org/1848663002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:734: return;
For this sort of thing, I'd avoid returning early, in case we add another task
to disconnection that should happen even if there are some other connections in
the tab.

[ortuno, 2016-03-31 17:53:25]

https://codereview.chromium.org/1848663002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:732: if
(!device_id_to_connection_map_.empty()) {
On 2016/03/31 at 16:18:43, Jeffrey Yasskin wrote:
> Currently, this is going to keep the indicator showing if any frame from the
process is connected, right? This is better than what we had, but we really
ought to maintain the indicator if there are any connections from the current
WebContents, and drop it if there are connections from other WebContents that
happen to share a process.

OK. What do you think of the new approach?

[Jeffrey Yasskin, 2016-03-31 18:00:26]

https://codereview.chromium.org/1848663002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:738:
web_contents->DecrementBluetoothConnectedDeviceCount();
Are we guaranteed that if a process crashes, it'll disconnect all of its
connections? What if the device goes out of range and the connection is lost?
Can we wind up leaking a connection count here?

https://codereview.chromium.org/1848663002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1848663002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1130: if
(bluetooth_connected_device_count_++ == 0) {
I hate post-increment, because I have to think too much about what value it's
going to return. Make this two statements?

[ortuno, 2016-04-04 21:20:56]

https://codereview.chromium.org/1848663002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:738:
web_contents->DecrementBluetoothConnectedDeviceCount();
On 2016/03/31 at 18:00:25, Jeffrey Yasskin wrote:
> Are we guaranteed that if a process crashes, it'll disconnect all of its
connections?

From some experimentation it seems that if the RenderProcress crashes, the
RenderProcessHost does not get notified. So we would remain connected until the
RenderProcessHost is destroyed. I hope that by moving to frame an implementing a
WebContentsObserver we can get notified of this.

> What if the device goes out of range and the connection is lost? Can we wind
up leaking a connection count here?

This is actually not hard to solve. We would just have to keep track of the
frames that have active connections and when a device disconnects notify the
WebContents for that frame.

The part that I can't solve is hiding the indicator when the frame navigates
because we have no way of knowing of navigation events. We could try to
implement WebContentsObserver but I think that time would better spent moving to
frame.

https://codereview.chromium.org/1848663002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1848663002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1130: if
(bluetooth_connected_device_count_++ == 0) {
On 2016/03/31 at 18:00:25, Jeffrey Yasskin wrote:
> I hate post-increment, because I have to think too much about what value it's
going to return. Make this two statements?

Done.

[ortuno, 2016-04-04 21:21:00]

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

[commit-bot: I haz the power, 2016-04-04 21:22:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848663002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848663002/100001

[commit-bot: I haz the power, 2016-04-04 22:47:56]

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

[commit-bot: I haz the power, 2016-04-04 22:47:57]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-04-06 23:05:13]

lgtm

https://codereview.chromium.org/1848663002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:720: // Indicate there is
one less connected device.
Add a TODO here saying that this can miss disconnections, and that we should
improve it once we're done Mojoifying connections. (Or a more accurate statement
of the problem and plan.)

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1129: // Notification for UI
updates if the state changes.
s/Notification/Notify/? Here and below.

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1312: size_t
bluetooth_connected_device_count_;
I was going to suggest an int here, but
http://www.chromium.org/developers/coding-style#TOC-Types says to use unsigned
for "object counts", which this is. So sure, size_t.

https://codereview.chromium.org/1848663002/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1848663002/diff/100001/content/public/browser...
content/public/browser/web_contents.h:357: // Increment calls must be balanced
with an equivalent number of
Maybe "Call Increment when a frame connects to a Bluetooth device and Decrement
when a connection is broken. IsConnectedToBluetoothDevice() then indicates
whether any frame in the WebContents is connected to a Bluetooth Device."

[ortuno, 2016-04-07 15:09:21]

Thanks!

https://codereview.chromium.org/1848663002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:720: // Indicate there is
one less connected device.
On 2016/04/06 at 23:05:13, Jeffrey Yasskin wrote:
> Add a TODO here saying that this can miss disconnections, and that we should
improve it once we're done Mojoifying connections. (Or a more accurate statement
of the problem and plan.)

Done.

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1129: // Notification for UI
updates if the state changes.
On 2016/04/06 at 23:05:13, Jeffrey Yasskin wrote:
> s/Notification/Notify/? Here and below.

Done.

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1848663002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:1312: size_t
bluetooth_connected_device_count_;
On 2016/04/06 at 23:05:13, Jeffrey Yasskin wrote:
> I was going to suggest an int here, but
http://www.chromium.org/developers/coding-style#TOC-Types says to use unsigned
for "object counts", which this is. So sure, size_t.

Ack.

https://codereview.chromium.org/1848663002/diff/100001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1848663002/diff/100001/content/public/browser...
content/public/browser/web_contents.h:357: // Increment calls must be balanced
with an equivalent number of
On 2016/04/06 at 23:05:13, Jeffrey Yasskin wrote:
> Maybe "Call Increment when a frame connects to a Bluetooth device and
Decrement when a connection is broken. IsConnectedToBluetoothDevice() then
indicates whether any frame in the WebContents is connected to a Bluetooth
Device."

Done.

[ortuno, 2016-04-07 15:10:04]

ortuno@chromium.org changed reviewers:
+ miu@chromium.org, nick@chromium.org

[ortuno, 2016-04-07 15:10:05]

miu@: PTAL at tab_utils

nick@: PTAL at web_contents

[ncarter (slow), 2016-04-07 19:48:54]

https://codereview.chromium.org/1848663002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:349:
device_id_to_connection_map_.clear();
Do we need to decrement here?

https://codereview.chromium.org/1848663002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1418:
device_id_to_connection_map_[device_id] = std::move(connection);
If the device was already in the map, then the connection count on the
WebContents is incorrect -- we wouldn't have decremented it for the overwritten
entry. Do we need to defend against that?

It might be worthwhile considering making the values of the
device_id_to_connection_map_ be RAII-style wrapper objects, that hold the
|connection| but also increment/decrement the WebContents count in their ctors &
dtors. My experience is that it is much easier to keep the increments/decrements
balanced that way, especially as the code gets refactored.

https://codereview.chromium.org/1848663002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1848663002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:281: void
DecrementBluetoothConnectedDeviceCount() override;
Looks like the increment/decrement methods are only called from inside content;
let's remove them from the public interface, and make them non-virtual methods
of WebContentsImpl.

(bluetooth_dispatcher_host, being inside of content, can freely static_cast its
web_contents to a WebContentsImpl).

[miu, 2016-04-07 20:05:45]

chrome/browser/ui/tabs/tab_utils.cc lgtm

[ortuno, 2016-04-07 22:34:52]

Thanks for the thorough review!

https://codereview.chromium.org/1848663002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:349:
device_id_to_connection_map_.clear();
On 2016/04/07 at 19:48:54, ncarter wrote:
> Do we need to decrement here?

This code is only run during tests. Anyway, the new object cleans up WebContents
before destruction.

https://codereview.chromium.org/1848663002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1418:
device_id_to_connection_map_[device_id] = std::move(connection);
On 2016/04/07 at 19:48:54, ncarter wrote:
> If the device was already in the map, then the connection count on the
WebContents is incorrect -- we wouldn't have decremented it for the overwritten
entry. Do we need to defend against that?
> 
> It might be worthwhile considering making the values of the
device_id_to_connection_map_ be RAII-style wrapper objects, that hold the
|connection| but also increment/decrement the WebContents count in their ctors &
dtors. My experience is that it is much easier to keep the increments/decrements
balanced that way, especially as the code gets refactored.

Added a new ConnectedDevicesMap. WDYT?

https://codereview.chromium.org/1848663002/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1848663002/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:281: void
DecrementBluetoothConnectedDeviceCount() override;
On 2016/04/07 at 19:48:54, ncarter wrote:
> Looks like the increment/decrement methods are only called from inside
content; let's remove them from the public interface, and make them non-virtual
methods of WebContentsImpl.
> 
> (bluetooth_dispatcher_host, being inside of content, can freely static_cast
its web_contents to a WebContentsImpl).

Done.

[ncarter (slow), 2016-04-07 22:44:53]

lgtm

https://codereview.chromium.org/1848663002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:459: :
render_process_id_(render_process_id) {}
Add a blank line here, between the two functions (or just git cl format)

https://codereview.chromium.org/1848663002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:519: }
Thanks for doing this.

[ortuno, 2016-04-07 22:49:01]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-04-07 22:49:03]

https://codereview.chromium.org/1848663002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1848663002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:459: :
render_process_id_(render_process_id) {}
On 2016/04/07 at 22:44:53, ncarter wrote:
> Add a blank line here, between the two functions (or just git cl format)

Hmm git cl format didn't catch this one... Done.

[ortuno, 2016-04-07 22:49:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, miu@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1848663002/#ps180001
(title: "Fix format"")

[commit-bot: I haz the power, 2016-04-07 22:49:39]

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

[commit-bot: I haz the power, 2016-04-08 02:33:49]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-08 02:35:21]

Description was changed from

==========
bluetooth: Remove indicator only when there are no more devices connected

Before, if multiple devices were connected and we disconnected from one
we would remove the indicator even though a device could still be connected.

BUG=599281
==========

to

==========
bluetooth: Remove indicator only when there are no more devices connected

Before, if multiple devices were connected and we disconnected from one
we would remove the indicator even though a device could still be connected.

BUG=599281

Committed: https://crrev.com/df4d79807643681b9bff15aa6dd7558230aed583
Cr-Commit-Position: refs/heads/master@{#385942}
==========

[commit-bot: I haz the power, 2016-04-08 02:35:23]

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