Refactor ChooserBubbleUiViewDelegate

Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170, 618618
Committed: https://crrev.com/69d8c2e016d84e8276aee3b7467b5129106af752
Cr-Commit-Position: refs/heads/master@{#399085}

[juncai, 2016-06-01 23:20:14]

Description was changed from

==========
Refactored ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth on desktop
and Chrome Apps, it is better to pull it out of ChooserBubbleUiViewDelegate
and have a class ChooserContentView that can be reused.

BUG=616598, 577170
==========

to

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth 
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170
==========

[juncai, 2016-06-03 01:26:53]

juncai@chromium.org changed reviewers:
+ msw@chromium.org, thakis@chromium.org

[juncai, 2016-06-03 01:26:53]

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

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

[msw, 2016-06-03 20:04:08]

msw@chromium.org changed reviewers:
+ groby@chromium.org

[msw, 2016-06-03 20:04:09]

mostly just nits.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:25: // Chooser width
nit: remove comment or add a trailing period; ditto below.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:178: return button ==
ui::DIALOG_BUTTON_OK
nit: button != ui::DIALOG_BUTTON_OK || !table_view_->selection_model().empty();

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:186: size_t offset;
nit: = 0

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:206: chooser_controller_ =
nullptr;
Is this even necessary? Nothing checks for a null |chooser_controller_|.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chooser_content_view.h (right):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.h:16: // A chooser content view
class that user can select an option.
nit: "A bubble or dialog view for choosing among several options in a table."
Maybe also say "Used for bluetooth device selection for extensions." or similar.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.h:40: // Called by the destructor
of the class that passed |chooser_controller_|
nit: "Called just before |chooser_controller_| is destroyed."

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (left):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:378:
chooser_bubble_ui_view_delegate_->chooser_table_model()->Update();
Why is this update no longer required? Has it moved?

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:83:
bubble_reference_->CloseBubble(BUBBLE_CLOSE_ACCEPTED);
+groby: it's too bad the unused bubble manager infra complicates the shared
dialog/bubble pattern here; see also https://codereview.chromium.org/2033283002

[Nico, 2016-06-03 20:05:51]

no class but no new test?

[Nico, 2016-06-03 20:05:59]

*new class bot no new test?

[groby-ooo-7-16, 2016-06-03 20:10:13]

On 2016/06/03 20:04:09, msw wrote:
> mostly just nits.
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/chooser_content_view.cc (right):
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chooser_content_view.cc:25: // Chooser width
> nit: remove comment or add a trailing period; ditto below.
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chooser_content_view.cc:178: return button ==
> ui::DIALOG_BUTTON_OK
> nit: button != ui::DIALOG_BUTTON_OK ||
!table_view_->selection_model().empty();
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chooser_content_view.cc:186: size_t offset;
> nit: = 0
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chooser_content_view.cc:206: chooser_controller_ =
> nullptr;
> Is this even necessary? Nothing checks for a null |chooser_controller_|.
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/chooser_content_view.h (right):
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chooser_content_view.h:16: // A chooser content view
> class that user can select an option.
> nit: "A bubble or dialog view for choosing among several options in a table."
> Maybe also say "Used for bluetooth device selection for extensions." or
similar.
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chooser_content_view.h:40: // Called by the destructor
> of the class that passed |chooser_controller_|
> nit: "Called just before |chooser_controller_| is destroyed."
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc
(left):
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:378:
> chooser_bubble_ui_view_delegate_->chooser_table_model()->Update();
> Why is this update no longer required? Has it moved?
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc
(right):
> 
>
https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:83:
> bubble_reference_->CloseBubble(BUBBLE_CLOSE_ACCEPTED);
> +groby: it's too bad the unused bubble manager infra complicates the shared
> dialog/bubble pattern here; see also
https://codereview.chromium.org/2033283002

msw: Yep. But I'm reluctant to invest too much into bubble manager until/unless
mac views is sorted out. Especially since E_PLATE_FULL. (If it's a _huge_
obstacle, let's rip out the manager and just re-instrument for metrics. )

[juncai, 2016-06-07 18:10:50]

On 2016/06/03 20:05:59, Nico wrote:
> *new class bot no new test?

Added test code.

[juncai, 2016-06-07 18:11:06]

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:25: // Chooser width
On 2016/06/03 20:04:08, msw wrote:
> nit: remove comment or add a trailing period; ditto below.

Done.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:178: return button ==
ui::DIALOG_BUTTON_OK
On 2016/06/03 20:04:08, msw wrote:
> nit: button != ui::DIALOG_BUTTON_OK ||
!table_view_->selection_model().empty();

Done.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:186: size_t offset;
On 2016/06/03 20:04:08, msw wrote:
> nit: = 0

Done.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:206: chooser_controller_ =
nullptr;
On 2016/06/03 20:04:08, msw wrote:
> Is this even necessary? Nothing checks for a null |chooser_controller_|.

This function is moved back to its original place at
|ChooserBubbleUiViewDelegate|.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chooser_content_view.h (right):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.h:16: // A chooser content view
class that user can select an option.
On 2016/06/03 20:04:08, msw wrote:
> nit: "A bubble or dialog view for choosing among several options in a table."
> Maybe also say "Used for bluetooth device selection for extensions." or
similar.

Done.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.h:40: // Called by the destructor
of the class that passed |chooser_controller_|
On 2016/06/03 20:04:09, msw wrote:
> nit: "Called just before |chooser_controller_| is destroyed."

Done.

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (left):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:378:
chooser_bubble_ui_view_delegate_->chooser_table_model()->Update();
On 2016/06/03 20:04:09, msw wrote:
> Why is this update no longer required? Has it moved?

At ChooserContentView constructor, it enables/disables |table_view_| using:
table_view_->SetEnabled(chooser_controller_->NumOptions() > 0);

So here after showing the chooser bubble, the |table_view_| is already
initialized correctly. So I think this line is redundant.

[Nico, 2016-06-07 21:52:14]

gypi changes lgtm

[msw, 2016-06-08 00:58:45]

Really nice comprehensive tests! Some comments...

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (left):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:378:
chooser_bubble_ui_view_delegate_->chooser_table_model()->Update();
On 2016/06/07 18:11:06, juncai wrote:
> On 2016/06/03 20:04:09, msw wrote:
> > Why is this update no longer required? Has it moved?
> 
> At ChooserContentView constructor, it enables/disables |table_view_| using:
> table_view_->SetEnabled(chooser_controller_->NumOptions() > 0);
> 
> So here after showing the chooser bubble, the |table_view_| is already
> initialized correctly. So I think this line is redundant.

Acknowledged, I hope this is okay, since ChooserTableModel::Update also calls
observer_->OnModelChanged() if there are 0 options. I wonder if perhaps that
call is not even necessary... (maybe defer optimization?)

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:19: ~ChooserTableModel()
override {}
optional nit: "= default;"?

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:101: ChooserController*
chooser_controller) {
Init |: table_view_(nullptr)| here or with |= nullptr;| in the header.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.h (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:34: ~ChooserContentView();
nit: "= default;"?

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:38: // The returned view is owned
by its parent view.
nit: remove this comment or say "Ownership of the view is passed to the caller."

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:45: // |table_view_| is not owned
by this class, it is owned by its parent view.
nit: remove this comment; that's standard behavior for views::View objects.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:48:
DISALLOW_COPY_AND_ASSIGN(ChooserContentView);
nit: add a blank line above

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view_unittest.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:88:
ASSERT_TRUE(mock_chooser_controller_);
nit: this seems unnecessary.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:90:
ASSERT_TRUE(mock_table_view_observer_);
nit: this seems unnecessary.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:93:
ASSERT_TRUE(chooser_content_view_);
nit: this seems unnecessary.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:99:
ASSERT_TRUE(mock_styled_label_listener_);
nit: this seems unnecessary.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:123:
EXPECT_EQ(table_view_->RowCount(), 1);
nit: put expectation before the actual value here and elsewhere (see decl):
  #define EXPECT_EQ(expected, actual) \

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:163:
EXPECT_CALL(*mock_table_view_observer_, OnSelectionChanged()).Times(3);
q: why does the selection change on removing options, but not adding?

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:223: // There is no
option shown now. But since "No devices found."
This isn't right... option "a" is still present.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:240: // Disselect
option 0.
nit: "Unselect" here and elsewhere.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:96:
views::TableView* table_view_;
optional nit: remove this member, just access
chooser_content_view_->table_view() as needed?

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:98:
ui::TableModel* table_model_;
nit: remove this member, just access table_view_->model() as needed?

[juncai, 2016-06-09 01:59:20]

juncai@chromium.org changed reviewers:
+ blundell@chromium.org

[juncai, 2016-06-09 01:59:22]

blundell@chromium.org: Please review changes in 
//components

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (left):

https://codereview.chromium.org/2029863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:378:
chooser_bubble_ui_view_delegate_->chooser_table_model()->Update();
On 2016/06/08 00:58:45, msw wrote:
> On 2016/06/07 18:11:06, juncai wrote:
> > On 2016/06/03 20:04:09, msw wrote:
> > > Why is this update no longer required? Has it moved?
> > 
> > At ChooserContentView constructor, it enables/disables |table_view_| using:
> > table_view_->SetEnabled(chooser_controller_->NumOptions() > 0);
> > 
> > So here after showing the chooser bubble, the |table_view_| is already
> > initialized correctly. So I think this line is redundant.
> 
> Acknowledged, I hope this is okay, since ChooserTableModel::Update also calls
> observer_->OnModelChanged() if there are 0 options. I wonder if perhaps that
> call is not even necessary... (maybe defer optimization?)

Added it back to be safe, :).

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:19: ~ChooserTableModel()
override {}
On 2016/06/08 00:58:45, msw wrote:
> optional nit: "= default;"?

Got compile error "Overriding method must be marked with 'override' or 'final'."
when using default.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:101: ChooserController*
chooser_controller) {
On 2016/06/08 00:58:45, msw wrote:
> Init |: table_view_(nullptr)| here or with |= nullptr;| in the header.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.h (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:34: ~ChooserContentView();
On 2016/06/08 00:58:45, msw wrote:
> nit: "= default;"?

Changed ChooserContentView to be a subclass of views::View. And the destructor
needs to be marked as override, so it seems default can not be used here.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:38: // The returned view is owned
by its parent view.
On 2016/06/08 00:58:45, msw wrote:
> nit: remove this comment or say "Ownership of the view is passed to the
caller."

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:45: // |table_view_| is not owned
by this class, it is owned by its parent view.
On 2016/06/08 00:58:45, msw wrote:
> nit: remove this comment; that's standard behavior for views::View objects.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.h:48:
DISALLOW_COPY_AND_ASSIGN(ChooserContentView);
On 2016/06/08 00:58:45, msw wrote:
> nit: add a blank line above

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view_unittest.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:88:
ASSERT_TRUE(mock_chooser_controller_);
On 2016/06/08 00:58:45, msw wrote:
> nit: this seems unnecessary.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:90:
ASSERT_TRUE(mock_table_view_observer_);
On 2016/06/08 00:58:45, msw wrote:
> nit: this seems unnecessary.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:93:
ASSERT_TRUE(chooser_content_view_);
On 2016/06/08 00:58:45, msw wrote:
> nit: this seems unnecessary.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:99:
ASSERT_TRUE(mock_styled_label_listener_);
On 2016/06/08 00:58:45, msw wrote:
> nit: this seems unnecessary.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:123:
EXPECT_EQ(table_view_->RowCount(), 1);
On 2016/06/08 00:58:45, msw wrote:
> nit: put expectation before the actual value here and elsewhere (see decl):
>   #define EXPECT_EQ(expected, actual) \

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:163:
EXPECT_CALL(*mock_table_view_observer_, OnSelectionChanged()).Times(3);
On 2016/06/08 00:58:45, msw wrote:
> q: why does the selection change on removing options, but not adding?

https://cs.chromium.org/chromium/src/ui/views/controls/table/table_view.cc?dr...

I guess that when items are added, they are added to the end and the already
selected item's index won't change. But when items are removed, the previously
selected item may be removed, or its index may change.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:223: // There is no
option shown now. But since "No devices found."
On 2016/06/08 00:58:45, msw wrote:
> This isn't right... option "a" is still present.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:240: // Disselect
option 0.
On 2016/06/08 00:58:45, msw wrote:
> nit: "Unselect" here and elsewhere.

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:96:
views::TableView* table_view_;
On 2016/06/08 00:58:45, msw wrote:
> optional nit: remove this member, just access
> chooser_content_view_->table_view() as needed?

Done.

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:98:
ui::TableModel* table_model_;
On 2016/06/08 00:58:45, msw wrote:
> nit: remove this member, just access table_view_->model() as needed?

Done.

[blundell, 2016-06-09 07:36:35]

lgtm

You can add per-file OWNERS to //components/OWNERS.

[juncai, 2016-06-09 18:14:38]

On 2016/06/09 07:36:35, blundell wrote:
> lgtm
> 
> You can add per-file OWNERS to //components/OWNERS.

Done.

[msw, 2016-06-09 18:31:16]

lgtm with nits; thanks for bearing with me!

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view_unittest.cc (right):

https://codereview.chromium.org/2029863002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view_unittest.cc:163:
EXPECT_CALL(*mock_table_view_observer_, OnSelectionChanged()).Times(3);
On 2016/06/09 01:59:21, juncai wrote:
> On 2016/06/08 00:58:45, msw wrote:
> > q: why does the selection change on removing options, but not adding?
> 
>
https://cs.chromium.org/chromium/src/ui/views/controls/table/table_view.cc?dr...
> 
> I guess that when items are added, they are added to the end and the already
> selected item's index won't change. But when items are removed, the previously
> selected item may be removed, or its index may change.

Okay, it might be nice if that were only called if the selection actually
changes... but that's not important for this CL. Feel free to add a TODO there,
file a bug, or follow up.

https://codereview.chromium.org/2029863002/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2029863002/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:167: if (chooser_controller_)
nit: chooser_controller_ should always be non-null. remove these null checks
(here and below), but now I feel silly for suggesting these pass-through
functions at all (Accept/Cancel/Close/StyledLabelLinkClicked), since they don't
really consolidate any code... Feel free to remove the null checks, remove the
pass-through functions altogether, or ignore this comment!

https://codereview.chromium.org/2029863002/diff/220001/components/chooser_con...
File components/chooser_controller/mock_chooser_controller.h (right):

https://codereview.chromium.org/2029863002/diff/220001/components/chooser_con...
components/chooser_controller/mock_chooser_controller.h:22: MOCK_METHOD1(Select,
void(size_t index));
nit: I prefer overrides with no body to MOCK_METHOD.

[msw, 2016-06-09 18:39:18]

still lgtm

https://codereview.chromium.org/2029863002/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2029863002/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:167: if (chooser_controller_)
On 2016/06/09 18:31:15, msw wrote:
> nit: chooser_controller_ should always be non-null. remove these null checks
> (here and below), but now I feel silly for suggesting these pass-through
> functions at all (Accept/Cancel/Close/StyledLabelLinkClicked), since they
don't
> really consolidate any code... Feel free to remove the null checks, remove the
> pass-through functions altogether, or ignore this comment!

nvm about removing the null checks (i see it's needed in a later patch set)
still, feel free to inline these pass-through functions or leave as-is.

https://codereview.chromium.org/2029863002/diff/220001/components/chooser_con...
File components/chooser_controller/mock_chooser_controller.h (right):

https://codereview.chromium.org/2029863002/diff/220001/components/chooser_con...
components/chooser_controller/mock_chooser_controller.h:22: MOCK_METHOD1(Select,
void(size_t index));
On 2016/06/09 18:31:15, msw wrote:
> nit: I prefer overrides with no body to MOCK_METHOD.

Nvm, I see the mocks are used/counted in the second CL.

[juncai, 2016-06-10 00:39:03]

Description was changed from

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth 
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170
==========

to

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth 
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170, 618618
==========

[juncai, 2016-06-10 01:09:01]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-06-10 01:09:01]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
blundell@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2029863002/#ps300001
(title: "make ChooserContentView::ChooserControllerDestroying a private
function")

[commit-bot: I haz the power, 2016-06-10 01:09:52]

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

[commit-bot: I haz the power, 2016-06-10 03:06:22]

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

[commit-bot: I haz the power, 2016-06-10 03:06:23]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[juncai, 2016-06-10 03:52:50]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2016-06-10 03:53:18]

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

[commit-bot: I haz the power, 2016-06-10 04:26:53]

Description was changed from

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth 
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170, 618618
==========

to

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth 
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170, 618618
==========

[commit-bot: I haz the power, 2016-06-10 04:26:57]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-06-10 04:28:59]

Description was changed from

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth 
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170, 618618
==========

to

==========
Refactor ChooserBubbleUiViewDelegate

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

Since the chooser content view is the same for WebBluetooth
on desktop and Chrome Apps, it is better to pull it out of
ChooserBubbleUiViewDelegate and have a class
ChooserContentView that can be reused.

BUG=616598, 577170, 618618

Committed: https://crrev.com/69d8c2e016d84e8276aee3b7467b5129106af752
Cr-Commit-Position: refs/heads/master@{#399085}
==========

[commit-bot: I haz the power, 2016-06-10 04:29:00]

Patchset 16 (id:??) landed as
https://crrev.com/69d8c2e016d84e8276aee3b7467b5129106af752
Cr-Commit-Position: refs/heads/master@{#399085}