Implement bluetooth chooser for Chrome Apps on non-Mac desktops

Implement bluetooth chooser for Chrome Apps on non-Mac desktops

This is the part 5 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 non-Mac
desktops.

BUG=616602, 577170
Committed: https://crrev.com/65ba5f961c774fe301c4955759cbddb0da1ad1dc
Cr-Commit-Position: refs/heads/master@{#399094}

[juncai, 2016-06-03 01:32:37]

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

[juncai, 2016-06-03 01:32:37]

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

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

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

[Reilly Grant (use Gerrit), 2016-06-03 19:27:07]

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (right):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:9: #include
"chrome/browser/ui/views/chooser_content_view.h"
This file appears to be missing from your patch.

[Reilly Grant (use Gerrit), 2016-06-03 19:34:23]

Never mind. lgtm

[msw, 2016-06-03 19:46:23]

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

[msw, 2016-06-03 19:46:24]

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_chooser_dialog.h (left):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_chooser_dialog.h:25: // TODO(juncai):
remove this preprocessor directive once the non-Mac
I'm confused, why remove this if the CL is for "non-Mac desktops", and the
chooser_dialog_view.* files are only added to
'chrome_browser_ui_views_extensions_non_mac_sources'? Does this implement the
dialog for Mac or not?

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:54: delete this;
Is this actually needed? DialogDelegateView::DeleteDelegate() should suffice.

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (right):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:9: #include
"chrome/browser/ui/views/chooser_content_view.h"
On 2016/06/03 19:27:07, Reilly Grant wrote:
> This file appears to be missing from your patch.

This CL depends on https://codereview.chromium.org/2029863002

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:17: // Displays a
chooser dialog as a modal sheet constrained
nit: "sheet" is a Mac term, right? Consider: "Displays a chooser view as a modal
dialog constrained"...

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:25: //
views::BubbleDialogDelegateView:
+estade for sanity checking the use of BubbleDialogDelegateView with
ShowWebModalDialogViews / DialogDelegate::CreateDialogWidgetWithBounds... Should
this just work as-is or should it override
CreateNonClientFrameView/ShouldUseCustomFrame/etc?

[Evan Stade, 2016-06-03 21:35:35]

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (right):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:25: //
views::BubbleDialogDelegateView:
On 2016/06/03 19:46:24, msw wrote:
> +estade for sanity checking the use of BubbleDialogDelegateView with
> ShowWebModalDialogViews / DialogDelegate::CreateDialogWidgetWithBounds...
Should
> this just work as-is or should it override
> CreateNonClientFrameView/ShouldUseCustomFrame/etc?

yea I think this is going to give some weird behavior around padding and stuff.
It's somewhat unintended and worrisome that this even works at all. I think you
need to use composition instead, i.e. own a ChooserContentView which is just a
View, and make that a contained view of ChooserDialogView. It can also provide
GetDialogButtonLabel, IsDialogButtonEnabled, and CreateFootnoteView, and your
ChooserDialogView will have to call these as pass throughs. (Same story for
ChooserBubbleUiViewDelegate.)

[Nico, 2016-06-04 00:59:02]

test?

chrome_browser_ui.gypi lgtm

[juncai, 2016-06-07 23:19:19]

On 2016/06/04 00:59:02, Nico wrote:
> test?
> 
> chrome_browser_ui.gypi lgtm

Added test code.

[juncai, 2016-06-07 23:19:39]

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_chooser_dialog.h (left):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_chooser_dialog.h:25: // TODO(juncai):
remove this preprocessor directive once the non-Mac
On 2016/06/03 19:46:24, msw wrote:
> I'm confused, why remove this if the CL is for "non-Mac desktops", and the
> chooser_dialog_view.* files are only added to
> 'chrome_browser_ui_views_extensions_non_mac_sources'? Does this implement the
> dialog for Mac or not?

Actually, this preprocessor directive was removed at the following CL:
https://codereview.chromium.org/2036033003/

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:54: delete this;
On 2016/06/03 19:46:24, msw wrote:
> Is this actually needed? DialogDelegateView::DeleteDelegate() should suffice.

removed.

Done.

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (right):

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:17: // Displays a
chooser dialog as a modal sheet constrained
On 2016/06/03 19:46:24, msw wrote:
> nit: "sheet" is a Mac term, right? Consider: "Displays a chooser view as a
modal
> dialog constrained"...

Done.

https://codereview.chromium.org/2033283002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:25: //
views::BubbleDialogDelegateView:
On 2016/06/03 21:35:35, Evan Stade wrote:
> On 2016/06/03 19:46:24, msw wrote:
> > +estade for sanity checking the use of BubbleDialogDelegateView with
> > ShowWebModalDialogViews / DialogDelegate::CreateDialogWidgetWithBounds...
> Should
> > this just work as-is or should it override
> > CreateNonClientFrameView/ShouldUseCustomFrame/etc?
> 
> yea I think this is going to give some weird behavior around padding and
stuff.
> It's somewhat unintended and worrisome that this even works at all. I think
you
> need to use composition instead, i.e. own a ChooserContentView which is just a
> View, and make that a contained view of ChooserDialogView. It can also provide
> GetDialogButtonLabel, IsDialogButtonEnabled, and CreateFootnoteView, and your
> ChooserDialogView will have to call these as pass throughs. (Same story for
> ChooserBubbleUiViewDelegate.)

Done.

[msw, 2016-06-08 01:16:32]

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:27: const int
kChooserDialogWidth = 300;
nit: these are shared between dialog & bubble, add them to ChooserView?

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:36: //
------------------------------------
I'm starting to wonder if ChooserView should implement/encapsulate more
functionality as a DialogClientView... Then the bubble and dialog delegates
would have less redundant code and responsibilities. WDYT?

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:56: table_view_ =
chooser_content_view_->table_view();
ditto nit (from other CL): remove this member.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:57: table_model_ =
table_view_->model();
ditto nit (from other CL): remove this member.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:68:
table_view_->SetModel(nullptr);
Do these in the ChooserView dtor?

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:147: void
ChromeExtensionChooserDialog::ShowDialogImpl(
Do we need to split ShowDialog and ShowDialogImpl?
Could this just be the implementation of ShowDialog?

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (right):

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:63: friend class
ChooserDialogViewTest;
nit: remove this, add public table_view_for_test(), access TableView::model().

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

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:20: class
MockChooserController : public ChooserController {
Can you share this between the dialog and bubble?

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:80:
ASSERT_TRUE(mock_chooser_controller_);
nit: this seems unnecessary.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:83:
ASSERT_TRUE(chooser_dialog_view_);
nit: this seems unnecessary.

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

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:27: const int
kChooserDialogWidth = 300;
On 2016/06/08 01:16:32, msw wrote:
> nit: these are shared between dialog & bubble, add them to ChooserView?

Done.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:36: //
------------------------------------
On 2016/06/08 01:16:32, msw wrote:
> I'm starting to wonder if ChooserView should implement/encapsulate more
> functionality as a DialogClientView... Then the bubble and dialog delegates
> would have less redundant code and responsibilities. WDYT?

Changed ChooserContentView to be a subclass of views::View and override
GetContentsView() to return an instance of ChooserContentView.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:56: table_view_ =
chooser_content_view_->table_view();
On 2016/06/08 01:16:32, msw wrote:
> ditto nit (from other CL): remove this member.

Done.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:57: table_model_ =
table_view_->model();
On 2016/06/08 01:16:32, msw wrote:
> ditto nit (from other CL): remove this member.

Done.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:68:
table_view_->SetModel(nullptr);
On 2016/06/08 01:16:32, msw wrote:
> Do these in the ChooserView dtor?

Done.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:147: void
ChromeExtensionChooserDialog::ShowDialogImpl(
On 2016/06/08 01:16:32, msw wrote:
> Do we need to split ShowDialog and ShowDialogImpl?
> Could this just be the implementation of ShowDialog?

This is a similar pattern as:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/chrome_dev...
which is a workaround for MacViews.

And in a follow-up CL I'll do similar thing as:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/devic...

At:
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/choos...

I'll add:
if (chrome::ToolkitViewsWebUIDialogsEnabled())
    return ChromeExtensionChooserDialog::ShowDialogImpl();

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.h (right):

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.h:63: friend class
ChooserDialogViewTest;
On 2016/06/08 01:16:32, msw wrote:
> nit: remove this, add public table_view_for_test(), access TableView::model().

Done.

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

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:20: class
MockChooserController : public ChooserController {
On 2016/06/08 01:16:32, msw wrote:
> Can you share this between the dialog and bubble?

Done.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:80:
ASSERT_TRUE(mock_chooser_controller_);
On 2016/06/08 01:16:32, msw wrote:
> nit: this seems unnecessary.

Done.

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:83:
ASSERT_TRUE(chooser_dialog_view_);
On 2016/06/08 01:16:32, msw wrote:
> nit: this seems unnecessary.

Done.

[msw, 2016-06-09 18:40:05]

lgtm

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right):

https://codereview.chromium.org/2033283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/chooser_dialog_view.cc:147: void
ChromeExtensionChooserDialog::ShowDialogImpl(
On 2016/06/09 01:59:53, juncai wrote:
> On 2016/06/08 01:16:32, msw wrote:
> > Do we need to split ShowDialog and ShowDialogImpl?
> > Could this just be the implementation of ShowDialog?
> 
> This is a similar pattern as:
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/chrome_dev...
> which is a workaround for MacViews.
> 
> And in a follow-up CL I'll do similar thing as:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/devic...
> 
> At:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/extensions/choos...
> 
> I'll add:
> if (chrome::ToolkitViewsWebUIDialogsEnabled())
>     return ChromeExtensionChooserDialog::ShowDialogImpl();

Acknowledged; thanks for the explanation.

[juncai, 2016-06-10 04:12:53]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-06-10 04:12:54]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
reillyg@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2033283002/#ps140001
(title: "rebase again")

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

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

[commit-bot: I haz the power, 2016-06-10 04:13:30]

This CL has an open dependency (Issue 2029863002 Patch 300001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[juncai, 2016-06-10 04:27:33]

The CQ bit was checked by juncai@chromium.org

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

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

[commit-bot: I haz the power, 2016-06-10 04:35:08]

Committed patchset #8 (id:140001)

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

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 04:37:30]

Description was changed from

==========
Implement bluetooth chooser for Chrome Apps on non-Mac desktops

This is the part 5 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 non-Mac
desktops.

BUG=616602, 577170
==========

to

==========
Implement bluetooth chooser for Chrome Apps on non-Mac desktops

This is the part 5 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 non-Mac
desktops.

BUG=616602, 577170

Committed: https://crrev.com/65ba5f961c774fe301c4955759cbddb0da1ad1dc
Cr-Commit-Position: refs/heads/master@{#399094}
==========

[commit-bot: I haz the power, 2016-06-10 04:37:36]

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