Remove Close button for chooser bubble and call CloseBubble when Cancel

Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed.

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204
Committed: https://crrev.com/22707c4f0558a333b63f6b99c7f06d5bd27a4f4e
Cr-Commit-Position: refs/heads/master@{#366699}

[juncai, 2015-12-14 22:54:31]

juncai@chromium.org changed reviewers:
+ felt@chromium.org, rockot@chromium.org

[juncai, 2015-12-14 22:54:32]

felt@chromium.org: Please review changes in 
//chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc

rockot@chromium.org: Please review changes in 
//chrome/browser/usb/

[juncai, 2015-12-14 23:04:03]

Description was changed from

==========
Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204
==========

to

==========
Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed.

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204
==========

[Ken Rockot(use gerrit already), 2015-12-15 06:24:05]

lgtm

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

https://codereview.chromium.org/1530493002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:280: owner_ =
nullptr;
nit: No need to set this to nullptr

https://codereview.chromium.org/1530493002/diff/20001/chrome/browser/usb/web_...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1530493002/diff/20001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_permission_bubble.cc:35: new
UsbChooserBubbleDelegate(browser, device_filters.Pass(),
nit: since you're changing this code, please migrate to using std::move(foo)
instead of foo.Pass()

[juncai, 2015-12-15 17:23:39]

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

https://codereview.chromium.org/1530493002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:280: owner_ =
nullptr;
On 2015/12/15 06:24:05, Ken Rockot wrote:
> nit: No need to set this to nullptr

Done.

https://codereview.chromium.org/1530493002/diff/20001/chrome/browser/usb/web_...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1530493002/diff/20001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_permission_bubble.cc:35: new
UsbChooserBubbleDelegate(browser, device_filters.Pass(),
On 2015/12/15 06:24:05, Ken Rockot wrote:
> nit: since you're changing this code, please migrate to using std::move(foo)
> instead of foo.Pass()

Done.

[felt, 2015-12-17 02:11:23]

Can you run this change by rolfe (or whichever UI designer owns this bubble)? I
am wondering whether it makes more sense to keep the "x" and remove the "cancel"
to be consistent with all other bubbles, which all have "x"es.

[felt, 2015-12-17 02:17:00]

just a nit

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

https://codereview.chromium.org/1530493002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:308:
button_pressed_ = true;
why did this line move? was there a bug (some ordering dependency), or are you
just moving it around for aesthetics?

https://codereview.chromium.org/1530493002/diff/60001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1530493002/diff/60001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.h:63: BubbleReference
bubble_reference_;
nit: this is not a very descriptive name... could you give it something more
descriptive? maybe even bubble_controller_ which would describe what it is a bit
better

[juncai, 2015-12-17 17:36:52]

On 2015/12/17 02:11:23, felt wrote:
> Can you run this change by rolfe (or whichever UI designer owns this bubble)?
I
> am wondering whether it makes more sense to keep the "x" and remove the
"cancel"
> to be consistent with all other bubbles, which all have "x"es.

Reilly and I had a meeting with Rebecca Rolfe before and we discussed the
chooser UI mock:
go/webusb-mocks
It doesn't have the close button. The WebBluetooth chooser UI mock also doesn't
have the close button.

[juncai, 2015-12-17 17:57:20]

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

https://codereview.chromium.org/1530493002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:308:
button_pressed_ = true;
On 2015/12/17 02:16:59, felt wrote:
> why did this line move? was there a bug (some ordering dependency), or are you
> just moving it around for aesthetics?

Yes, it is a bug since chooser_bubble_delegate_->Select or
chooser_bubble_delegate_->Cancel function will call
ChooserBubbleUiViewDelegate::Close and in that function it checks if the bubble
is closed because "Connect"/"Cancel" button is pressed or because tab switching,
tab closing, etc. If it is caused by tab switching, tab closing, etc., then
chooser_bubble_delegate_->Close() needs to be called. So button_pressed_ needs
to be assigned true value before calling chooser_bubble_delegate_->Select or
chooser_bubble_delegate_->Cancel.

https://codereview.chromium.org/1530493002/diff/60001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1530493002/diff/60001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.h:63: BubbleReference
bubble_reference_;
On 2015/12/17 02:16:59, felt wrote:
> nit: this is not a very descriptive name... could you give it something more
> descriptive? maybe even bubble_controller_ which would describe what it is a
bit
> better

Done.

[juncai, 2015-12-21 20:01:59]

ping felt@, :).

[felt, 2015-12-22 23:28:39]

lgtm

[juncai, 2015-12-23 00:15:43]

The CQ bit was checked by juncai@chromium.org

[juncai, 2015-12-23 00:15:43]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1530493002/#ps100001
(title: "use std::move instead of .Pass()")

[commit-bot: I haz the power, 2015-12-23 00:16:03]

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

[commit-bot: I haz the power, 2015-12-23 00:20:18]

Description was changed from

==========
Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed.

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204
==========

to

==========
Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed.

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204
==========

[commit-bot: I haz the power, 2015-12-23 00:20:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-23 00:21:23]

Description was changed from

==========
Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed.

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204
==========

to

==========
Remove Close button for chooser bubble and call CloseBubble when Cancel
button is pressed.

The Close button (the upper-right X) is redundant since the Cancel
button exists for the chooser bubble. The Close button is already
removed at chooser_bubble_ui_cocoa.mm, so also do it here for non-Mac
implementation. Also added code to call CloseBubble when Cancel button
is pressed.

BUG=492204

Committed: https://crrev.com/22707c4f0558a333b63f6b99c7f06d5bd27a4f4e
Cr-Commit-Position: refs/heads/master@{#366699}
==========

[commit-bot: I haz the power, 2015-12-23 00:21:25]

Patchset 6 (id:??) landed as
https://crrev.com/22707c4f0558a333b63f6b99c7f06d5bd27a4f4e
Cr-Commit-Position: refs/heads/master@{#366699}