Change ChooserController ownership model

chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.h"
 
 #include "chrome/browser/extensions/chrome_extension_chooser_dialog.h"
 #include "chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h"
 #include "content/public/browser/web_contents.h"
 
 ChromeExtensionBluetoothChooser::ChromeExtensionBluetoothChooser(
     content::RenderFrameHost* frame,
     const content::BluetoothChooser::EventHandler& event_handler) {
-  bluetooth_chooser_controller_.reset(
+  std::unique_ptr<BluetoothChooserController> bluetooth_chooser_controller(
       new BluetoothChooserController(frame, event_handler));
+  // Since ChromeExtensionBluetoothChooser object is destroyed before the

[msw, 2016/06/22 18:23:17]

I can't really reason about the lifetime of the dialog's views in relation to
this ChromeExtensionBluetoothChooser instance (owned by
content/browser/bluetooth/bluetooth_device_chooser_controller.cc). How did you
come to this assessment and can you really assert that the dialog never closes
(tears down its view hierarchy or similar cocoa structures) earlier than this
object in all cases?

[juncai, 2016/06/22 22:41:20]

When user makes a selection or cancels, it will call
ChooserContentView::Accept(), or Cancel() before bubble/dialog closes, and that
will call ChooserController::Select(), or Cancel(). For bluetooth, it will call
event_handler_.Run():
https://cs.chromium.org/chromium/src/chrome/browser/ui/bluetooth/bluetooth_ch...
and that will call:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_devi...
and it frees the |chooser_| at:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_devi...

So I guess this object is destroyed before chooser view is destroyed.

[msw, 2016/06/22 23:13:37]

Acknowledged.

+  // view object which owns |bluetooth_chooser_controller_| when the chooser
+  // bubble/dialog closes, it is safe to store and use the raw pointer here.
+  bluetooth_chooser_controller_ = bluetooth_chooser_controller.get();
   chooser_dialog_.reset(new ChromeExtensionChooserDialog(
       content::WebContents::FromRenderFrameHost(frame)));
-  chooser_dialog_->ShowDialog(bluetooth_chooser_controller_.get());
+  chooser_dialog_->ShowDialog(std::move(bluetooth_chooser_controller));
 }
 
 ChromeExtensionBluetoothChooser::~ChromeExtensionBluetoothChooser() {}
 
 void ChromeExtensionBluetoothChooser::SetAdapterPresence(
     AdapterPresence presence) {}
 
 void ChromeExtensionBluetoothChooser::ShowDiscoveryState(DiscoveryState state) {
 }
 
 void ChromeExtensionBluetoothChooser::AddDevice(
     const std::string& device_id,
     const base::string16& device_name) {
   bluetooth_chooser_controller_->AddDevice(device_id, device_name);

[msw, 2016/06/22 18:23:17]

I wonder why the addition and removal of devices comes through here at all;
couldn't BluetoothChooserController be notified in another manner (maybe via
some observer pattern)?

[juncai, 2016/06/22 22:41:20]

I guess that would need some refactoring of the BluetoothChooser class:
https://cs.chromium.org/chromium/src/content/public/browser/bluetooth_chooser...

[msw, 2016/06/22 23:13:37]

Acknowledged.

 }
 
 void ChromeExtensionBluetoothChooser::RemoveDevice(
     const std::string& device_id) {
   bluetooth_chooser_controller_->RemoveDevice(device_id);
 }