Refactor the BluetoothDispatcherHost to store requestDevice sessions

Refactor the BluetoothDispatcherHost to store requestDevice sessions
in a member map, instead of in bound callback parameters.

This gives us a place to store the dialog where we can update it incrementally
as new devices are discovered.

BUG=500989
Committed: https://crrev.com/ba252308adb747c82eb89d88687bc72fc313c5f2
Cr-Commit-Position: refs/heads/master@{#339362}

[Jeffrey Yasskin, 2015-07-15 06:52:50]

jyasskin@chromium.org changed reviewers:
+ asvitkine@chromium.org, nick@chromium.org, ortuno@chromium.org,
scheib@chromium.org

[Jeffrey Yasskin, 2015-07-15 06:52:50]

PTAL (I won't get back to this until Thursday, so no rush.)

Nick for bad_messages.h and Alexei for histograms.xml. Doesn't it get annoying
that there's no 1 person who can approve changes to both?

[Jeffrey Yasskin, 2015-07-15 07:44:29]

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

[commit-bot: I haz the power, 2015-07-15 07:44:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235243005/1

[commit-bot: I haz the power, 2015-07-15 07:46:18]

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

[commit-bot: I haz the power, 2015-07-15 07:46:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  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_...)

(exceeded global retry quota)

[Alexei Svitkine (slow), 2015-07-15 13:22:58]

https://codereview.chromium.org/1235243005/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1235243005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:50920: +  <int value="93"
label="BDH_DUPLICATE_REQUEST_DEVICE_ID"/>
Given that this list has grown recently, it would be better to use a sparse
histogram to log the values.

Would you mind changing:

 UMA_HISTOGRAM_ENUMERATION("Stability.BadMessageTerminated.Content", reason,
                            BAD_MESSAGE_MAX);

to:

UMA_HISTOGRAM_SPARSE_SLOWLY("Stability.BadMessageTerminated.Content", reason);

[ncarter (slow), 2015-07-15 17:04:05]

On 2015/07/15 13:22:58, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1235243005/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1235243005/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:50920: +  <int value="93"
> label="BDH_DUPLICATE_REQUEST_DEVICE_ID"/>
> Given that this list has grown recently, it would be better to use a sparse
> histogram to log the values.
> 
> Would you mind changing:
> 
>  UMA_HISTOGRAM_ENUMERATION("Stability.BadMessageTerminated.Content", reason,
>                             BAD_MESSAGE_MAX);
> 
> to:
> 
> UMA_HISTOGRAM_SPARSE_SLOWLY("Stability.BadMessageTerminated.Content", reason);

bad_message.h lgtm, though if you have to rebase due to a conflict there, feel
free to ask me to double-check you work.

asvitkine: I'll make that SPARSE_SLOWLY change (lfg and I were talking about
that last week, actually -- and it is out of scope for jyasskin's work). In
practice these histograms shouldn't actually be instantiated often, it's good to
not bloat memory when we do.

[scheib, 2015-07-15 17:12:31]

Code LGTM, but a design Q:

It will be possible to have multiple tabs open at once with multiple chooser
dialogs. As we're discovering are you planning to iterate the map and update all
dialogs with new devices?

[Alexei Svitkine (slow), 2015-07-15 17:23:21]

LGTM assuming the sparse change will come in a follow-up CL. Thanks!

[Jeffrey Yasskin, 2015-07-15 17:24:53]

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

[commit-bot: I haz the power, 2015-07-15 17:27:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1235243005/1

[commit-bot: I haz the power, 2015-07-15 20:15:42]

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

[commit-bot: I haz the power, 2015-07-15 20:15:43]

Dry run: This issue passed the CQ dry run.

[ortuno, 2015-07-15 22:39:12]

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:234:
std::make_pair(thread_id, request_id),
Would an iframe and it's parent use the same web_bluetooth_impl? I'm worried
that since thread_id is always 0 you might have some collisions with request_id.

Maybe add tests to make sure you never get coalitions?

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:519:
request_device_sessions_.find(std::make_pair(thread_id, request_id));
We use thread_id + request_id everywhere. What do you think about passing them
together through IPC?

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.h:54: struct
RequestDeviceSession;
Why 'RequestDeviceSession'? It took me a bit to realize that you were just
saving the filters in there.

[ortuno, 2015-07-15 23:10:18]

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.h:54: struct
RequestDeviceSession;
On 2015/07/15 at 22:39:12, ortuno wrote:
> Why 'RequestDeviceSession'? It took me a bit to realize that you were just
saving the filters in there.

Ah now that I see the follow up patch I get why it's called
RequestDeviceSession. Can you add a comment?

[Jeffrey Yasskin, 2015-07-17 01:07:23]

On 2015/07/15 17:12:31, scheib wrote:
> Code LGTM, but a design Q:
> 
> It will be possible to have multiple tabs open at once with multiple chooser
> dialogs. As we're discovering are you planning to iterate the map and update
all
> dialogs with new devices?

Yep.

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:234:
std::make_pair(thread_id, request_id),
On 2015/07/15 22:39:12, ortuno wrote:
> Would an iframe and it's parent use the same web_bluetooth_impl? I'm worried
> that since thread_id is always 0 you might have some collisions with
request_id.


They'll use the same BluetoothDispatcher (since it's looked up by thread), and
it's BluetoothDispatcher that allocates the request_id.

> Maybe add tests to make sure you never get collisions?

Sure: https://codereview.chromium.org/1227863007.

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:519:
request_device_sessions_.find(std::make_pair(thread_id, request_id));
On 2015/07/15 22:39:12, ortuno wrote:
> We use thread_id + request_id everywhere. What do you think about passing them
> together through IPC?

It'd make sense to do that, but I'd rather do it after this patch. We'll also
have to double-check how to keep
BluetoothMessageFilter::GetWorkerThreadIdForMessage working.

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.h:54: struct
RequestDeviceSession;
On 2015/07/15 23:10:18, ortuno wrote:
> On 2015/07/15 at 22:39:12, ortuno wrote:
> > Why 'RequestDeviceSession'? It took me a bit to realize that you were just
> saving the filters in there.
> 
> Ah now that I see the follow up patch I get why it's called
> RequestDeviceSession. Can you add a comment?

I have a comment by the definition of request_device_sessions_. It'd make sense
to add one by the definition of RequestDeviceSession in the .cc file too. Done.

[Jeffrey Yasskin, 2015-07-17 01:08:44]

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

[commit-bot: I haz the power, 2015-07-17 01:10:36]

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

[commit-bot: I haz the power, 2015-07-17 03:00:27]

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

[commit-bot: I haz the power, 2015-07-17 03:00:28]

Dry run: This issue passed the CQ dry run.

[ortuno, 2015-07-17 16:13:46]

lgtm

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1235243005/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.h:54: struct
RequestDeviceSession;
On 2015/07/17 at 01:07:23, Jeffrey Yasskin wrote:
> On 2015/07/15 23:10:18, ortuno wrote:
> > On 2015/07/15 at 22:39:12, ortuno wrote:
> > > Why 'RequestDeviceSession'? It took me a bit to realize that you were just
> > saving the filters in there.
> > 
> > Ah now that I see the follow up patch I get why it's called
> > RequestDeviceSession. Can you add a comment?
> 
> I have a comment by the definition of request_device_sessions_. It'd make
sense to add one by the definition of RequestDeviceSession in the .cc file too.
Done.

Thanks!

[Jeffrey Yasskin, 2015-07-18 00:10:08]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-07-18 00:10:08]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
asvitkine@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1235243005/#ps20001
(title: "Add a comment")

[commit-bot: I haz the power, 2015-07-18 00:10:35]

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

[commit-bot: I haz the power, 2015-07-18 00:14:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-18 00:15:52]

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