Change ChooserController ownership model

Change ChooserController ownership model

Currently chooser bubble and chooser dialog have different ChooserController
ownership models. For chooser bubble, the ChooserController is owned by
ChooserBubbleDelegate, which is a subclass of BubbleDelegate and BubbleManager
owns it. For chooser dialog, ChromeExtensionBluetoothChooser owns it, which
is a subclass of BluetoothChooser.

Since the ChooserController is only used by view classes, a better way is to
let ChooserContentView (for non-Mac) and ChooserContentViewCocoa (for Mac)
own it.

BUG=621216
Committed: https://crrev.com/474e1e11c78fc65fe773af28dcec21bb3014b269
Cr-Commit-Position: refs/heads/master@{#402647}

[juncai, 2016-06-21 22:56:45]

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

[juncai, 2016-06-21 22:56:46]

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

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

msw@chromium.org: Please review changes in 
//chrome/browser/ui/bluetooth/
//chrome/browser/ui/views/
//chrome/browser/ui/website_settings/

[Reilly Grant (use Gerrit), 2016-06-21 23:47:45]

https://codereview.chromium.org/2086663003/diff/20001/components/chooser_cont...
File components/chooser_controller/chooser_controller.cc (right):

https://codereview.chromium.org/2086663003/diff/20001/components/chooser_cont...
components/chooser_controller/chooser_controller.cc:21: }
Make owning_frame_ non-const to avoid this cast. This can then be written more
clearly as:

return owning_frame_ ? owning_frame_->GetLastCommittedOrigin() : url::Origin();

[Reilly Grant (use Gerrit), 2016-06-22 00:01:33]

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc (right):

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:16:
bluetooth_chooser_controller_ = bluetooth_chooser_controller.get();
Please add a comment here explaining why storing this raw pointer is safe.

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right):

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:41:
std::unique_ptr<ChooserController> chooser_controller_;
The comment is confusing because it seems to be directly contradicted by the
fact that this is a unique_ptr. I think instead you mean to say that this field
only temporarily owns the ChooserController. It is moved into the
ChooserContentViewCocoa when BuildBubbleUi is called.

[juncai, 2016-06-22 01:17:45]

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc (right):

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:16:
bluetooth_chooser_controller_ = bluetooth_chooser_controller.get();
On 2016/06/22 00:01:32, Reilly Grant wrote:
> Please add a comment here explaining why storing this raw pointer is safe.

Done.

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right):

https://codereview.chromium.org/2086663003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:41:
std::unique_ptr<ChooserController> chooser_controller_;
On 2016/06/22 00:01:32, Reilly Grant wrote:
> The comment is confusing because it seems to be directly contradicted by the
> fact that this is a unique_ptr. I think instead you mean to say that this
field
> only temporarily owns the ChooserController. It is moved into the
> ChooserContentViewCocoa when BuildBubbleUi is called.

Done.

https://codereview.chromium.org/2086663003/diff/20001/components/chooser_cont...
File components/chooser_controller/chooser_controller.cc (right):

https://codereview.chromium.org/2086663003/diff/20001/components/chooser_cont...
components/chooser_controller/chooser_controller.cc:21: }
On 2016/06/21 23:47:45, Reilly Grant wrote:
> Make owning_frame_ non-const to avoid this cast. This can then be written more
> clearly as:
> 
> return owning_frame_ ? owning_frame_->GetLastCommittedOrigin() :
url::Origin();

Done.

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

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:16: // Since
ChromeExtensionBluetoothChooser object is destroyed before the
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?

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:36:
bluetooth_chooser_controller_->AddDevice(device_id, device_name);
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)?

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/browser_dialogs_views.cc (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/browser_dialogs_views.cc:14: #include
"components/chooser_controller/chooser_controller.h"
nit: does chrome_extension_chooser_dialog.h's forward decl suffice here?

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:41:
views::Widget* modal_dialog = views::DialogDelegate::CreateDialogWidget(
Why do we need to actually create and show a dialog widget now?

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:23: explicit
ChooserBubbleDelegate(
nit: remove explicit

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:37: //
ChooserContentView(non-Mac)/ChooserContentViewCocoa(Mac).
optional nit: update all the comments to instead say ChooserContentView[Cocoa]

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:40: //
BuildBubbleUi() is called and the bubble is shown.
aside: Once again, I really wish the bubble manager code either didn't exist or
served some purpose... as-is it's just increasing complexity with no value.

[Robert Sesek, 2016-06-22 21:46:30]

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right):

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:35: ChooserController:
nit: lowercase first c

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:72: -
(NSInteger)numOptions;
naming: numberOfOptions

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:75: -
(NSString*)getOption:(NSInteger)index;
naming: optionAtIndex:

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:111: chooserController_ =
std::move(chooserController);
Place this in the if block below.

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right):

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:39: //
|chooser_controller_| is not owned by this class, it is owned by
This seems significantly more confusing than the current ownership model. Views
generally don't own things in Cocoa; their controllers do.

[juncai, 2016-06-22 22:41:21]

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:16: // Since
ChromeExtensionBluetoothChooser object is destroyed before the
On 2016/06/22 18:23:17, msw wrote:
> 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?

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.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:36:
bluetooth_chooser_controller_->AddDevice(device_id, device_name);
On 2016/06/22 18:23:17, msw wrote:
> 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)?

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

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/browser_dialogs_views.cc (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/browser_dialogs_views.cc:14: #include
"components/chooser_controller/chooser_controller.h"
On 2016/06/22 18:23:17, msw wrote:
> nit: does chrome_extension_chooser_dialog.h's forward decl suffice here?

Tried commenting this #include, but got compile errors.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:41:
views::Widget* modal_dialog = views::DialogDelegate::CreateDialogWidget(
On 2016/06/22 18:23:17, msw wrote:
> Why do we need to actually create and show a dialog widget now?

Creating and showing the dialog make sure that the ChooserContentView is freed
when the test is done since all the child views are released.

I tried running this test without showing the dialog, it will complain
|mock_chooser_controller_| object is leaked. It is used at EXPECT_CALL(), and
owned by ChooserContentView. I guess the reason is if not showing the dialog,
the ChooserContentView is not added to the view hierarchy so it is not freed.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:23: explicit
ChooserBubbleDelegate(
On 2016/06/22 18:23:17, msw wrote:
> nit: remove explicit

Done.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:37: //
ChooserContentView(non-Mac)/ChooserContentViewCocoa(Mac).
On 2016/06/22 18:23:17, msw wrote:
> optional nit: update all the comments to instead say ChooserContentView[Cocoa]

Done.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:40: //
BuildBubbleUi() is called and the bubble is shown.
On 2016/06/22 18:23:17, msw wrote:
> aside: Once again, I really wish the bubble manager code either didn't exist
or
> served some purpose... as-is it's just increasing complexity with no value.

Acknowledged.

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right):

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:35: ChooserController:
On 2016/06/22 21:46:30, Robert Sesek wrote:
> nit: lowercase first c

Done.

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:72: -
(NSInteger)numOptions;
On 2016/06/22 21:46:30, Robert Sesek wrote:
> naming: numberOfOptions

Done.

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:75: -
(NSString*)getOption:(NSInteger)index;
On 2016/06/22 21:46:29, Robert Sesek wrote:
> naming: optionAtIndex:

Done.

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:111: chooserController_ =
std::move(chooserController);
On 2016/06/22 21:46:30, Robert Sesek wrote:
> Place this in the if block below.

Done.

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h (right):

https://codereview.chromium.org/2086663003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.h:39: //
|chooser_controller_| is not owned by this class, it is owned by
On 2016/06/22 21:46:30, Robert Sesek wrote:
> This seems significantly more confusing than the current ownership model.
Views
> generally don't own things in Cocoa; their controllers do.

The current ownership model causes many places to have a weak pointer of
ChooserController* chooser_controller_, and it makes safely using that weak
pointer hard. By changing it to let ChooserContentView[Cocoa] own the
ChooserController, it removes almost all of the ChooserController weak pointers
except at ChromeExtensionBluetoothChooser and BluetoothChooserDesktop classes.
And in that case, I think it is safe to store and use it (explained at another
comment).

Also ChooserContentView[Cocoa] is used by both the chooser bubble and dialog. If
let their controllers own |chooser_controller_|, the two controllers will have
some similar redundant code that uses the |chooser_controller_|.

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

I also think this is a bit confusing, but lgtm

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc (right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:16: // Since
ChromeExtensionBluetoothChooser object is destroyed before the
On 2016/06/22 22:41:20, juncai wrote:
> On 2016/06/22 18:23:17, msw wrote:
> > 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?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/chrome_extension_bluetooth_chooser.cc:36:
bluetooth_chooser_controller_->AddDevice(device_id, device_name);
On 2016/06/22 22:41:20, juncai wrote:
> On 2016/06/22 18:23:17, msw wrote:
> > 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)?
> 
> I guess that would need some refactoring of the BluetoothChooser class:
>
https://cs.chromium.org/chromium/src/content/public/browser/bluetooth_chooser...

Acknowledged.

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2086663003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:41:
views::Widget* modal_dialog = views::DialogDelegate::CreateDialogWidget(
On 2016/06/22 22:41:20, juncai wrote:
> On 2016/06/22 18:23:17, msw wrote:
> > Why do we need to actually create and show a dialog widget now?
> 
> Creating and showing the dialog make sure that the ChooserContentView is freed
> when the test is done since all the child views are released.
> 
> I tried running this test without showing the dialog, it will complain
> |mock_chooser_controller_| object is leaked. It is used at EXPECT_CALL(), and
> owned by ChooserContentView. I guess the reason is if not showing the dialog,
> the ChooserContentView is not added to the view hierarchy so it is not freed.

Acknowledged.

[Robert Sesek, 2016-06-27 22:13:50]

On 2016/06/22 23:13:37, msw wrote:
> I also think this is a bit confusing, but lgtm

If both reviewers share this concern, I feel like this should be addressed.

My main concern is how one object holds the unique_ptr temporarily before
handing it off to something else. Would it be possible to remove that temporary
ownership?

[juncai, 2016-06-28 01:08:43]

On 2016/06/27 22:13:50, Robert Sesek wrote:
> On 2016/06/22 23:13:37, msw wrote:
> > I also think this is a bit confusing, but lgtm
> 
> If both reviewers share this concern, I feel like this should be addressed.
> 
> My main concern is how one object holds the unique_ptr temporarily before
> handing it off to something else. Would it be possible to remove that
temporary
> ownership?

Removed the temporary unique_ptr at ChooserBubbleUiCocoa and
ChooserBubbleUiController.

Please take a look. Thanks!

[Robert Sesek, 2016-06-28 22:26:01]

LGTM thanks that's more clear

[Reilly Grant (use Gerrit), 2016-06-28 22:29:52]

lgtm

[juncai, 2016-06-28 23:27:44]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-06-28 23:27:45]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2086663003/#ps120001
(title: "removed temporary unique ptr at ChooserBubbleUiCocoa")

[commit-bot: I haz the power, 2016-06-28 23:29:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 01:57:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-29 01:58:35]

Description was changed from

==========
Change ChooserController ownership model

Currently chooser bubble and chooser dialog have different ChooserController
ownership models. For chooser bubble, the ChooserController is owned by
ChooserBubbleDelegate, which is a subclass of BubbleDelegate and BubbleManager
owns it. For chooser dialog, ChromeExtensionBluetoothChooser owns it, which
is a subclass of BluetoothChooser.

Since the ChooserController is only used by view classes, a better way is to
let ChooserContentView (for non-Mac) and ChooserContentViewCocoa (for Mac)
own it.

BUG=621216
==========

to

==========
Change ChooserController ownership model

Currently chooser bubble and chooser dialog have different ChooserController
ownership models. For chooser bubble, the ChooserController is owned by
ChooserBubbleDelegate, which is a subclass of BubbleDelegate and BubbleManager
owns it. For chooser dialog, ChromeExtensionBluetoothChooser owns it, which
is a subclass of BluetoothChooser.

Since the ChooserController is only used by view classes, a better way is to
let ChooserContentView (for non-Mac) and ChooserContentViewCocoa (for Mac)
own it.

BUG=621216

Committed: https://crrev.com/474e1e11c78fc65fe773af28dcec21bb3014b269
Cr-Commit-Position: refs/heads/master@{#402647}
==========

[commit-bot: I haz the power, 2016-06-29 01:58:36]

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