Implement bluetooth chooser for Chrome Apps on Mac

Implement bluetooth chooser for Chrome Apps on Mac

This is the part 3 of enabling the chooser UIs for Bluetooth from
Apps to enable developers to make a transition.

This CL implements the chooser UI for Bluetooth from Apps on Mac.
There will be different CLs for non-Mac since the chooser UI is
different.

BUG=613638, 577170
Committed: https://crrev.com/17fc8532d48139d6bb4386cc6aa6a4beb4bab5e7
Cr-Commit-Position: refs/heads/master@{#397320}

[juncai, 2016-05-23 19:16:54]

juncai@chromium.org changed reviewers:
+ reillyg@chromium.org, rockot@chromium.org, rsesek@chromium.org,
thestig@chromium.org

[juncai, 2016-05-23 19:16:54]

thestig@chromium.org: Please review changes in 
//chrome/chrome_browser_extensions.gypi
//chrome/chrome_browser_ui.gypi

rsesek@chromium.org: Please review changes in 
//chrome/browser/ui/cocoa/extensions

reillyg@chromium.org: Please review changes in 
//chrome/browser/extensions
//extensions/browser

rockot@chromium.org: Please review changes in 
//extensions/browser/DEPS

[Reilly Grant (use Gerrit), 2016-05-23 19:36:49]

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/bluetooth_chrome_extension_chooser_dialog.h
(right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/bluetooth_chrome_extension_chooser_dialog.h:20: GURL
GetHelpCenterUrl() const override;
Can you move this virtual function to ChooserController so that it can be
implemented by BluetoothChooserController? That would remove the need for both a
BluetoothChromeExtensionChooserDialog and UsbChromeExtensionChooserDialog.

[Robert Sesek, 2016-05-23 20:13:53]

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.h (right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.h:38: void Dismissed();
This isn't a getter, so move it up and document.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.mm (right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.mm:48:
onOptionAdded:static_cast<NSInteger>(index)];
NSUInteger is an unsigned version that should be implicitly convertible from
size_t.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.h
(right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.h:32:
initWithChooserDialogCocoa:(ChooserDialogCocoa*)chooserDialogCocoa
nit: break after the : instead

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.h:33:
initWithChooserController:(ChooserController*)chooserController;
nit: indent 4

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm
(right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
How about a unit test?

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:80:
static_cast<NSInteger>(1));
You shouldn't need to static_cast 1...

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:137:
chooserController_ = nil;
nil is only for ObjC objects, use nullptr for C++ ones

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:137:
chooserController_ = nil;
If you mash the button hooked up to onConnect, can you cause a null deref?

[juncai, 2016-05-23 20:28:19]

On 2016/05/23 19:36:49, Reilly Grant wrote:
>
https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/extensio...
> File chrome/browser/extensions/bluetooth_chrome_extension_chooser_dialog.h
> (right):
> 
>
https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/bluetooth_chrome_extension_chooser_dialog.h:20: GURL
> GetHelpCenterUrl() const override;
> Can you move this virtual function to ChooserController so that it can be
> implemented by BluetoothChooserController? That would remove the need for both
a
> BluetoothChromeExtensionChooserDialog and UsbChromeExtensionChooserDialog.

It is already in the ChooserController:
https://codereview.chromium.org/1984923002/diff/130001/components/ui/chooser_...
And ExtensionBluetoothChooser is a subclass of both ChooserController and
content::BluetoothChooser, it needs to implement GetHelpCenterUrl(). Since
ExtensionBluetoothChooser owns a pointer to ExtensionChooserDialog which has
subclass BluetoothChromeExtensionChooserDialog, it implements the
GetHelpCenterUrl functionality for bluetooth.

[Ken Rockot(use gerrit already), 2016-05-24 05:29:49]

On 2016/05/23 at 19:16:54, juncai wrote:
> thestig@chromium.org: Please review changes in 
> //chrome/chrome_browser_extensions.gypi
> //chrome/chrome_browser_ui.gypi
> 
> rsesek@chromium.org: Please review changes in 
> //chrome/browser/ui/cocoa/extensions
> 
> reillyg@chromium.org: Please review changes in 
> //chrome/browser/extensions
> //extensions/browser
> 
> rockot@chromium.org: Please review changes in 
> //extensions/browser/DEPS

said file lgtm

[juncai, 2016-05-27 21:30:37]

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/bluetooth_chrome_extension_chooser_dialog.h
(right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/bluetooth_chrome_extension_chooser_dialog.h:20: GURL
GetHelpCenterUrl() const override;
On 2016/05/23 19:36:49, Reilly Grant wrote:
> Can you move this virtual function to ChooserController so that it can be
> implemented by BluetoothChooserController? That would remove the need for both
a
> BluetoothChromeExtensionChooserDialog and UsbChromeExtensionChooserDialog.

Done.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.h (right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.h:38: void Dismissed();
On 2016/05/23 20:13:53, Robert Sesek wrote:
> This isn't a getter, so move it up and document.

Done.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.mm (right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa.mm:48:
onOptionAdded:static_cast<NSInteger>(index)];
On 2016/05/23 20:13:53, Robert Sesek wrote:
> NSUInteger is an unsigned version that should be implicitly convertible from
> size_t.

Done.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.h
(right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.h:32:
initWithChooserDialogCocoa:(ChooserDialogCocoa*)chooserDialogCocoa
On 2016/05/23 20:13:53, Robert Sesek wrote:
> nit: break after the : instead

Done.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.h:33:
initWithChooserController:(ChooserController*)chooserController;
On 2016/05/23 20:13:53, Robert Sesek wrote:
> nit: indent 4

Done.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm
(right):

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/05/23 20:13:53, Robert Sesek wrote:
> How about a unit test?

Done.

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:80:
static_cast<NSInteger>(1));
On 2016/05/23 20:13:53, Robert Sesek wrote:
> You shouldn't need to static_cast 1...

It seems that the static_cast is necessary, otherwise will get compile error:
"error: no matching function for call to 'max'"

https://codereview.chromium.org/2005443002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller.mm:137:
chooserController_ = nil;
On 2016/05/23 20:13:53, Robert Sesek wrote:
> nil is only for ObjC objects, use nullptr for C++ ones

It seems that after switching the order of:
chooserController_->Select(row);
chooserDialogCocoa_->Dismissed();
to be:
chooserDialogCocoa_->Dismissed();
chooserController_->Select(row);
it is not necessary to explicitly set them to nullptr.

I think what's happening is after the "Connect" button is pressed, somehow line
83: - (id)tableView:(NSTableView*)tableView is still called. And it accesses the
chooserController_ pointer, but after chooserController_->Select(row) is called,
the pointer is not valid any more. So accessing it causes segmentation fault. By
switching the order of function calls, it is guaranteed that the
chooserController_ pointer is valid when chooserDialogCocoa_->Dismissed() is
called.

[Robert Sesek, 2016-05-31 17:27:51]

LGTM % nit

https://codereview.chromium.org/2005443002/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2005443002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:68:
std::vector<base::string16> optionNames_;
nit: names in C++ classes should use under_scores, same with lines 113-120.

[juncai, 2016-05-31 20:45:03]

https://codereview.chromium.org/2005443002/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2005443002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:68:
std::vector<base::string16> optionNames_;
On 2016/05/31 17:27:51, Robert Sesek wrote:
> nit: names in C++ classes should use under_scores, same with lines 113-120.

Done.

[juncai, 2016-06-01 15:58:43]

ping reillyg@, thestig@, :).

[Reilly Grant (use Gerrit), 2016-06-01 17:59:24]

https://codereview.chromium.org/2005443002/diff/140001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.h (right):

https://codereview.chromium.org/2005443002/diff/140001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.h:22: class
ChromeExtensionBluetoothChooser : public content::BluetoothChooser {
This seems to be a duplicate of BluetoothChooserDesktop.

[Lei Zhang, 2016-06-01 18:18:04]

gypi files lgtm

[juncai, 2016-06-01 21:15:47]

https://codereview.chromium.org/2005443002/diff/140001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.h (right):

https://codereview.chromium.org/2005443002/diff/140001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.h:22: class
ChromeExtensionBluetoothChooser : public content::BluetoothChooser {
On 2016/06/01 17:59:23, Reilly Grant wrote:
> This seems to be a duplicate of BluetoothChooserDesktop.

Talked offline. The ChromeExtensionBluetoothChooser and BluetoothChooserDesktop
have different ownership model of the BluetoothChooserController object. So it
seems better to have two separate classes for Chrome and extension.

[juncai, 2016-06-01 21:19:31]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-06-01 21:19:32]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2005443002/#ps140001
(title: "rebase and address comments")

[Reilly Grant (use Gerrit), 2016-06-01 21:19:37]

lgtm

[commit-bot: I haz the power, 2016-06-01 21:20:26]

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

[commit-bot: I haz the power, 2016-06-02 02:08:56]

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

[commit-bot: I haz the power, 2016-06-02 02:08:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[juncai, 2016-06-02 02:27:34]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2016-06-02 02:27:52]

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

[commit-bot: I haz the power, 2016-06-02 03:09:52]

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

[commit-bot: I haz the power, 2016-06-02 03:09:54]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[juncai, 2016-06-02 03:23:53]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2016-06-02 03:24:20]

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

[commit-bot: I haz the power, 2016-06-02 04:25:28]

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

[commit-bot: I haz the power, 2016-06-02 04:25:30]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2016-06-02 05:54:27]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-06-02 05:54:47]

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

[commit-bot: I haz the power, 2016-06-02 06:46:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-02 06:47:11]

Description was changed from

==========
Implement bluetooth chooser for Chrome Apps on Mac

This is the part 3 of enabling the chooser UIs for Bluetooth from
Apps to enable developers to make a transition.

This CL implements the chooser UI for Bluetooth from Apps on Mac.
There will be different CLs for non-Mac since the chooser UI is
different.

BUG=613638, 577170
==========

to

==========
Implement bluetooth chooser for Chrome Apps on Mac

This is the part 3 of enabling the chooser UIs for Bluetooth from
Apps to enable developers to make a transition.

This CL implements the chooser UI for Bluetooth from Apps on Mac.
There will be different CLs for non-Mac since the chooser UI is
different.

BUG=613638, 577170

Committed: https://crrev.com/17fc8532d48139d6bb4386cc6aa6a4beb4bab5e7
Cr-Commit-Position: refs/heads/master@{#397320}
==========

[commit-bot: I haz the power, 2016-06-02 06:47:13]

Patchset 8 (id:??) landed as
https://crrev.com/17fc8532d48139d6bb4386cc6aa6a4beb4bab5e7
Cr-Commit-Position: refs/heads/master@{#397320}