Add chrome.hid.getUserSelectedDevices API.

Add chrome.hid.getUserSelectedDevices API.

This change wires up the HID API to the DevicePermissionsManager so
that devices the user has granted permission are enumerated and can be
connected to. It then adds a chrome.hid.getUserSelectedDevices function
which displays a dialog box (using the new AskForHidDevices method
added to DevicePermissionsPrompt) asking the user to select one or more
HID devices. The application can filter which devices are available for
the user to select.

BUG=457899
Committed: https://crrev.com/7c2dc58d437b8dfa269227981b0221c196888d41
Cr-Commit-Position: refs/heads/master@{#335316}

[Reilly Grant (use Gerrit), 2015-05-05 20:30:32]

reillyg@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Reilly Grant (use Gerrit), 2015-05-05 20:30:51]

Devlin, please take a look.

[Reilly Grant (use Gerrit), 2015-05-05 20:31:01]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-05-05 22:19:51]

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:112: ExtensionFunction::ResponseAction
HidGetUserSelectedDevicesFunction::Run() {
I thought I saw something in the IDL about this needing user gesture, but I
don't see that implemented anywhere?

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:115:
EXTENSION_FUNCTION_VALIDATE(parameters.get());
nit: scoped_ptrs are Testable, so you don't need the .get().  I don't really
care between using it and not, but you should probably be consistent :)  (It
looks like you don't use it on, e.g., line 123)

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:118: if (parameters->options.multiple) {
inline all this?
bool multiple = parameters->options.multiple && *parameters->options.multiple;

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:131: GetAssociatedWebContents());
Prefer GetSenderWebContents() in most cases.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:145: if (!device_manager) {
I'm seeing a lot of these null checks in this patch.  Under what circumstances
can device_manager be unavailable?  Should we gate the availability on the API
on the availability of the manager?  Should we at least make this check before
we try and show a picker to the user?

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:151:
OneArgument(device_manager->GetApiDevicesFromList(devices).release()));
There's a OneArgument variation that takes a scoped_ptr - use that instead of
release()ing here.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_apitest.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_apitest.cc:267: // TODO(reillyg): Enable this
part of the test when crbug.com/483328 is fixed.
according to the bug, it is fixed?

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_device_manager.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:133: const auto& device_entry =
resource_ids_.find(device->device_id());
a const& to an iterator is kind of weird...

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:134: DCHECK(device_entry !=
resource_ids_.end());
DCHECK_NE

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:174: return true;
When is |updatre_last_used| true?  (Maybe I just missed it somewhere, but I
didn't see it in this CL).  Also, this looks kind of weird - it looks like this
method does something totally different when |update_last_used| is true than
when it's not.  Does this really all belong in one method?  If so, why?  (It
totally could - I'm not super familiar with this code yet.)

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_device_manager.h (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.h:114: bool initialized_ = false;
please be consistent - if certain variables are going to be default-initialized,
they all should be.

https://codereview.chromium.org/1115213004/diff/20001/extensions/common/api/h...
File extensions/common/api/hid.idl (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/common/api/h...
extensions/common/api/hid.idl:103: static void
getUserSelectedDevices(DevicePromptOptions options,
Why is |options| required?

[Reilly Grant (use Gerrit), 2015-05-06 00:52:53]

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:112: ExtensionFunction::ResponseAction
HidGetUserSelectedDevicesFunction::Run() {
On 2015/05/05 22:19:50, Devlin wrote:
> I thought I saw something in the IDL about this needing user gesture, but I
> don't see that implemented anywhere?

Good catch.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:115:
EXTENSION_FUNCTION_VALIDATE(parameters.get());
On 2015/05/05 22:19:50, Devlin wrote:
> nit: scoped_ptrs are Testable, so you don't need the .get().  I don't really
> care between using it and not, but you should probably be consistent :)  (It
> looks like you don't use it on, e.g., line 123)

Fixed.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:118: if (parameters->options.multiple) {
On 2015/05/05 22:19:50, Devlin wrote:
> inline all this?
> bool multiple = parameters->options.multiple && *parameters->options.multiple;

Done.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:131: GetAssociatedWebContents());
On 2015/05/05 22:19:50, Devlin wrote:
> Prefer GetSenderWebContents() in most cases.

Done.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:145: if (!device_manager) {
On 2015/05/05 22:19:50, Devlin wrote:
> I'm seeing a lot of these null checks in this patch.  Under what circumstances
> can device_manager be unavailable?  Should we gate the availability on the API
> on the availability of the manager?  Should we at least make this check before
> we try and show a picker to the user?

I've moved the check into Run. HidDeviceManager should always be available but
I've been bitten by incognito bugs where something goes wrong and it isn't and
I'd rather a clean failure than a crash.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:151:
OneArgument(device_manager->GetApiDevicesFromList(devices).release()));
On 2015/05/05 22:19:50, Devlin wrote:
> There's a OneArgument variation that takes a scoped_ptr - use that instead of
> release()ing here.

Done.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_apitest.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_apitest.cc:267: // TODO(reillyg): Enable this
part of the test when crbug.com/483328 is fixed.
On 2015/05/05 22:19:50, Devlin wrote:
> according to the bug, it is fixed?

I sent out this review as I was landing the patch. ^_^

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_device_manager.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:133: const auto& device_entry =
resource_ids_.find(device->device_id());
On 2015/05/05 22:19:50, Devlin wrote:
> a const& to an iterator is kind of weird...

Fixed.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:134: DCHECK(device_entry !=
resource_ids_.end());
On 2015/05/05 22:19:50, Devlin wrote:
> DCHECK_NE

DCHECK_EQ and DCHECK_NE don't work for iterators because they don't implement
operator<<.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:174: return true;
On 2015/05/05 22:19:50, Devlin wrote:
> When is |updatre_last_used| true?  (Maybe I just missed it somewhere, but I
> didn't see it in this CL).  Also, this looks kind of weird - it looks like
this
> method does something totally different when |update_last_used| is true than
> when it's not.  Does this really all belong in one method?  If so, why?  (It
> totally could - I'm not super familiar with this code yet.)

All it does when update_last_used is set update the timestamp on the permission
entry. It's set to true by the hid.connect API and false by hid.getDevices and
other places where we're only enumerating devices the app has permission to
access.

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_device_manager.h (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.h:114: bool initialized_ = false;
On 2015/05/05 22:19:50, Devlin wrote:
> please be consistent - if certain variables are going to be
default-initialized,
> they all should be.

Done.

https://codereview.chromium.org/1115213004/diff/20001/extensions/common/api/h...
File extensions/common/api/hid.idl (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/common/api/h...
extensions/common/api/hid.idl:103: static void
getUserSelectedDevices(DevicePromptOptions options,
On 2015/05/05 22:19:51, Devlin wrote:
> Why is |options| required?

No reason.

[Devlin, 2015-05-06 23:04:28]

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:145: if (!device_manager) {
On 2015/05/06 00:52:53, reillyg wrote:
> On 2015/05/05 22:19:50, Devlin wrote:
> > I'm seeing a lot of these null checks in this patch.  Under what
circumstances
> > can device_manager be unavailable?  Should we gate the availability on the
API
> > on the availability of the manager?  Should we at least make this check
before
> > we try and show a picker to the user?
> 
> I've moved the check into Run. HidDeviceManager should always be available but
> I've been bitten by incognito bugs where something goes wrong and it isn't and
> I'd rather a clean failure than a crash.

I can sympathize, but if something should always be available, we should treat
it as though it is (feel free to put in a DCHECK/CHECK so we can get really nice
traces), for the same reasons that we discourage behavior like
DCHECK(!bad)
if (bad)
  return;

So if the manager is meant to be null, e.g., in incognito, then the check should
be
if (!manager)
  return Error()  // Manager can be null while incognito

But otherwise, we should assume it exists, so we can flush out any times it
doesn't erroneously.

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:123: return RespondNow(OneArgument(new
base::ListValue()));
This looks like it should be an error, not an empty result.

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
File extensions/browser/api/hid/hid_device_manager.cc (right):

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
extensions/browser/api/hid/hid_device_manager.cc:163: if (permissions_manager) {
Same question here - when do we expect this to be null?

[Reilly Grant (use Gerrit), 2015-05-06 23:48:52]

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/20001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:145: if (!device_manager) {
On 2015/05/06 23:04:28, Devlin wrote:
> On 2015/05/06 00:52:53, reillyg wrote:
> > On 2015/05/05 22:19:50, Devlin wrote:
> > > I'm seeing a lot of these null checks in this patch.  Under what
> circumstances
> > > can device_manager be unavailable?  Should we gate the availability on the
> API
> > > on the availability of the manager?  Should we at least make this check
> before
> > > we try and show a picker to the user?
> > 
> > I've moved the check into Run. HidDeviceManager should always be available
but
> > I've been bitten by incognito bugs where something goes wrong and it isn't
and
> > I'd rather a clean failure than a crash.
> 
> I can sympathize, but if something should always be available, we should treat
> it as though it is (feel free to put in a DCHECK/CHECK so we can get really
nice
> traces), for the same reasons that we discourage behavior like
> DCHECK(!bad)
> if (bad)
>   return;
> 
> So if the manager is meant to be null, e.g., in incognito, then the check
should
> be
> if (!manager)
>   return Error()  // Manager can be null while incognito
> 
> But otherwise, we should assume it exists, so we can flush out any times it
> doesn't erroneously.

I've removed the kErrorServiceUnavailable message and turned all instances into
CHECKs.

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:123: return RespondNow(OneArgument(new
base::ListValue()));
On 2015/05/06 23:04:28, Devlin wrote:
> This looks like it should be an error, not an empty result.

The documented behavior is that this returns an empty result when there is not a
user gesture.

[Reilly Grant (use Gerrit), 2015-05-07 20:34:30]

reillyg@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Reilly Grant (use Gerrit), 2015-05-07 20:34:30]

Adding Alexei for a histograms review.

[Devlin, 2015-05-07 20:43:55]

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/40001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:123: return RespondNow(OneArgument(new
base::ListValue()));
On 2015/05/06 23:48:52, reillyg wrote:
> On 2015/05/06 23:04:28, Devlin wrote:
> > This looks like it should be an error, not an empty result.
> 
> The documented behavior is that this returns an empty result when there is not
a
> user gesture.

Oh, odd.  In other places (e.g., the management api), we return an extension
error so that the developer knows what happened - which I think makes more
sense.  But, that's just me.  I'll leave it to the API owners.

https://codereview.chromium.org/1115213004/diff/60001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/60001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:133: CHECK(prompt_);
This has me a little concerned.  If the extension calls this from a web contents
that doesn't support modal dialogs, then this will fail.  I'm not sure how often
that can happen when it's also in a user gesture, but I wouldn't be surprised if
there are cases.  I'd probably feel better if we checked that the web contents
supported modal dialogs before creating and using a prompt (that might be better
placed in the prompt itself?  Or in CreateDevicePermissionsPrompt?)

WDYT?

[Reilly Grant (use Gerrit), 2015-05-07 20:53:39]

https://codereview.chromium.org/1115213004/diff/60001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/60001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:133: CHECK(prompt_);
On 2015/05/07 20:43:55, Devlin wrote:
> This has me a little concerned.  If the extension calls this from a web
contents
> that doesn't support modal dialogs, then this will fail.  I'm not sure how
often
> that can happen when it's also in a user gesture, but I wouldn't be surprised
if
> there are cases.  I'd probably feel better if we checked that the web contents
> supported modal dialogs before creating and using a prompt (that might be
better
> placed in the prompt itself?  Or in CreateDevicePermissionsPrompt?)
> 
> WDYT?

There's no check in this path for whether the web contents supports modal
dialogs. (CreateDevicePermissionsPrompt in Chrome just calls "new" so it will
never fail.) The problem will likely occur later when we call ShowDialog and it
gets to, for example, ShowWebModalDialogViews and discovers the issue.

[Devlin, 2015-05-07 21:02:43]

https://codereview.chromium.org/1115213004/diff/60001/extensions/browser/api/...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/60001/extensions/browser/api/...
extensions/browser/api/hid/hid_api.cc:133: CHECK(prompt_);
On 2015/05/07 20:53:39, reillyg wrote:
> On 2015/05/07 20:43:55, Devlin wrote:
> > This has me a little concerned.  If the extension calls this from a web
> contents
> > that doesn't support modal dialogs, then this will fail.  I'm not sure how
> often
> > that can happen when it's also in a user gesture, but I wouldn't be
surprised
> if
> > there are cases.  I'd probably feel better if we checked that the web
contents
> > supported modal dialogs before creating and using a prompt (that might be
> better
> > placed in the prompt itself?  Or in CreateDevicePermissionsPrompt?)
> > 
> > WDYT?
> 
> There's no check in this path for whether the web contents supports modal
> dialogs. (CreateDevicePermissionsPrompt in Chrome just calls "new" so it will
> never fail.) The problem will likely occur later when we call ShowDialog and
it
> gets to, for example, ShowWebModalDialogViews and discovers the issue.

Yeah, that's my point - I think we'll just crash.  But we should never crash
because an extension api was called from a weird place. :)  So, in other words,
I think we should *add* a check somewhere along this path (either here, or in
the CreateDevicePermissionsPrompt would be my recommendations).

[Alexei Svitkine (slow), 2015-05-08 18:35:44]

histograms lgtm

[Reilly Grant (use Gerrit), 2015-05-09 20:05:53]

I've added checks for unavailable web contexts and web contexts without popup
managers. This should catch the cases where this API is called from a strange
context.

[Reilly Grant (use Gerrit), 2015-05-13 01:33:09]

reillyg@chromium.org changed reviewers:
+ benwells@chromium.org

[Reilly Grant (use Gerrit), 2015-05-13 01:33:10]

Adding Ben for the
chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc change.

[benwells, 2015-05-13 02:57:57]

On 2015/05/13 01:33:10, reillyg wrote:
> Adding Ben for the
> chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc change.

^^^ file lgtm

[Devlin, 2015-05-14 17:38:11]

Sorry for the delay - was OOO.

https://codereview.chromium.org/1115213004/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm
(right):

https://codereview.chromium.org/1115213004/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:65:
prompt()->Dismissed();
- Can we consolidate this code with the views equivalent in the base class
somehow?
- Is treating the prompt as Dismissed() the right this to do in this case?  Or
should we instead indicate that it was never shown?  (Even though for the API,
the result might be the same, if we ever wanna show this prompt again, it might
be good to be able to differentiate).
- Calling dismissed synchronously like this is problematic (see comments in
api.cc).

https://codereview.chromium.org/1115213004/diff/100001/extensions/browser/api...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/100001/extensions/browser/api...
extensions/browser/api/hid/hid_api.cc:135: prompt_->AskForHidDevices(
This is problematic with your handling of having no PopupManager.
- This calls into AskForHidDevices()
- AskForHidDevices() calls into ShowDialog()
- ShowDialog() calls Prompt::Dismissed()
- Dimissed() calls OnDevicesChosen()
- OnDevicesChosen() calls Respond()
(All of the above happens synchronously...)
- The call to AskForHidDevices() finishes, and we call RespondLater() (after
already calling Respond()). -> Error.

[Reilly Grant (use Gerrit), 2015-05-14 18:04:28]

https://codereview.chromium.org/1115213004/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm
(right):

https://codereview.chromium.org/1115213004/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:65:
prompt()->Dismissed();
On 2015/05/14 17:38:11, Devlin wrote:
> - Can we consolidate this code with the views equivalent in the base class
> somehow?
> - Is treating the prompt as Dismissed() the right this to do in this case?  Or
> should we instead indicate that it was never shown?  (Even though for the API,
> the result might be the same, if we ever wanna show this prompt again, it
might
> be good to be able to differentiate).
> - Calling dismissed synchronously like this is problematic (see comments in
> api.cc).

The modal dialog managers seem to work slightly differently so I don't think
they can be consolidated. I'll push the failure handling up into the parent
class.

https://codereview.chromium.org/1115213004/diff/100001/extensions/browser/api...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/100001/extensions/browser/api...
extensions/browser/api/hid/hid_api.cc:135: prompt_->AskForHidDevices(
On 2015/05/14 17:38:11, Devlin wrote:
> This is problematic with your handling of having no PopupManager.
> - This calls into AskForHidDevices()
> - AskForHidDevices() calls into ShowDialog()
> - ShowDialog() calls Prompt::Dismissed()
> - Dimissed() calls OnDevicesChosen()
> - OnDevicesChosen() calls Respond()
> (All of the above happens synchronously...)
> - The call to AskForHidDevices() finishes, and we call RespondLater() (after
> already calling Respond()). -> Error.

You're right that the documentation for RespondLater says that Respond should be
called afterwards, but actually RespondLater is a no-op. This should be
documented because requiring RespondLater to be returned first leads to
unnecessary complexity.

The browser tests for this API exercise the synchronous path to make sure it is
safe.

[Devlin, 2015-05-14 18:32:02]

https://codereview.chromium.org/1115213004/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm
(right):

https://codereview.chromium.org/1115213004/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/device_permissions_dialog_controller.mm:65:
prompt()->Dismissed();
And you're confident that Dismissed()'ing the dialog is the right thing to do,
even if it never showed?  (You didn't respond to this in your comment.)

https://codereview.chromium.org/1115213004/diff/100001/extensions/browser/api...
File extensions/browser/api/hid/hid_api.cc (right):

https://codereview.chromium.org/1115213004/diff/100001/extensions/browser/api...
extensions/browser/api/hid/hid_api.cc:135: prompt_->AskForHidDevices(
On 2015/05/14 18:04:28, reillyg wrote:
> On 2015/05/14 17:38:11, Devlin wrote:
> > This is problematic with your handling of having no PopupManager.
> > - This calls into AskForHidDevices()
> > - AskForHidDevices() calls into ShowDialog()
> > - ShowDialog() calls Prompt::Dismissed()
> > - Dimissed() calls OnDevicesChosen()
> > - OnDevicesChosen() calls Respond()
> > (All of the above happens synchronously...)
> > - The call to AskForHidDevices() finishes, and we call RespondLater() (after
> > already calling Respond()). -> Error.
> 
> You're right that the documentation for RespondLater says that Respond should
be
> called afterwards, but actually RespondLater is a no-op. This should be
> documented because requiring RespondLater to be returned first leads to
> unnecessary complexity.
> 
> The browser tests for this API exercise the synchronous path to make sure it
is
> safe.

But there's a larger problem, which is that the API is very clear and well
documented, and it should be used in a certain way.  It's not just for safety;
it's also for readability and reasoning.  Calling things re-entrantly is
generally dangerous, especially when it's undocumented and unpredictable. 
Couple that with the fact that this makes the RespondLater() below a lie, and
this code becomes very hard to understand.  I'd rather have
slightly-more-complex, deterministic code than simple code with an execution
trail that we won't be able to grok in three months. :)

My advice for this would probably be to have PostTask() for the prompt
dismissal, so that any future calls of the prompt also avoid re-entrant calling.
 It's about two more lines of code, and waives my concerns.

[Reilly Grant (use Gerrit), 2015-06-10 23:01:12]

After discussing the ExtensionFunction interface with kalman@ I've been
convinced that calling callback.Run() directly is a dangerous pattern because of
the possibilities for accidental reentrancy and so I've updated this patch to
post the call to prompt->Dismissed() to the current thread's task runner.

[Reilly Grant (use Gerrit), 2015-06-19 16:31:36]

Devlin, ping?

[Devlin, 2015-06-19 17:29:41]

On 2015/06/19 16:31:36, reillyg wrote:
> Devlin, ping?

Sorry for the delay - please feel free to reping after a day or two.  LGTM, and
thanks for fixing the re-entrancy issues!

[Reilly Grant (use Gerrit), 2015-06-19 18:06:22]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-06-19 18:06:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, benwells@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1115213004/#ps140001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-06-19 18:07:24]

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

[commit-bot: I haz the power, 2015-06-19 19:23:05]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-06-19 19:24:11]

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