[Harmony] Harmonize Bluetooth dialogs.

[Harmony] Harmonize Bluetooth dialogs.

BUG=680381
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2684793004
Cr-Commit-Position: refs/heads/master@{#449702}
Committed: https://chromium.googlesource.com/chromium/src/+/36bf620f1e9240fea9df9ab0e07bd650bb392bcc

[xdai1, 2017-02-09 00:36:50]

Description was changed from

==========
[Harmony] Harmony Bluetooth dialogs.

BUG=680381
==========

to

==========
[Harmony] Harmony Bluetooth dialogs.

BUG=680381
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[xdai1, 2017-02-09 00:41:09]

Patchset #1 (id:1) has been deleted

[xdai1, 2017-02-09 00:48:29]

Patchset #1 (id:20001) has been deleted

[xdai1, 2017-02-09 00:49:53]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 00:50:26]

Dry run: 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, 2017-02-09 01:03:06]

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

[commit-bot: I haz the power, 2017-02-09 01:03:07]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[xdai1, 2017-02-09 01:12:59]

Patchset #1 (id:40001) has been deleted

[xdai1, 2017-02-09 01:14:18]

xdai@chromium.org changed reviewers:
+ stevenjb@chromium.org

[xdai1, 2017-02-09 01:14:18]

stevenjb@, could you talk a look at this CL please? Thanks!
I'll upload the screenshots later in the bug.

[xdai1, 2017-02-09 01:32:42]

On 2017/02/09 01:14:18, xdai1 wrote:
> stevenjb@, could you talk a look at this CL please? Thanks!
> I'll upload the screenshots later in the bug.

Screenshots have been uploaded.

[stevenjb, 2017-02-09 01:41:43]

stevenjb@chromium.org changed reviewers:
+ jamescook@chromium.org

[stevenjb, 2017-02-09 01:41:43]

This change makes me a bit nervous because we don't have any tests for this code
and the Buletooth pairing UI is fickle.

That said, I don't see any obvious issues. Please make sure to test this with
all of the fake bluetooth devices on desktop and make sure the behavior is the
same (e.g. typing a pin and pressing enter connects :) ).

+jamescook@ - Can you look at the views code, it's been a while for me and I'm
only peripherally familiar with mus.

[James Cook, 2017-02-09 15:18:02]

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc (right):

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc:60: views::Widget*
widget = new views::Widget;
This block of code looks mostly copied from
SystemTrayClient::CreateUnownedDialogWidget().

Can the two share code? If it's hard to do that, at least document that it's
similar.

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.cc (right):

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.cc:26:
BluetoothPairingDialog* dialog = new BluetoothPairingDialog(device);
nit: document ownership (since you're using raw new and not unique_ptr)

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h (right):

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h:32: // Shows the
customarized Bluetooth pairing dialog.
nit: "customized"

Also, document parameters, especially container_id.

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h:33: static void
ShowDialog(int container_id,
It's kind of weird that there's a "ShowDialog" method on a View class. I think
the Show method should be on the Dialog class.

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h:39:
DISALLOW_COPY_AND_ASSIGN(BluetoothPairingView);
#include macros.h

[xdai1, 2017-02-09 22:58:29]

stevenjb@, jamescook@, I've addressed your comments. Please take another look,
thanks! 

Note this CL doesn't do any functional change (Bluetooth should work exactly the
way it works before), only layout/style change.

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc (right):

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc:60: views::Widget*
widget = new views::Widget;
On 2017/02/09 15:18:02, James Cook wrote:
> This block of code looks mostly copied from
> SystemTrayClient::CreateUnownedDialogWidget().
> 
> Can the two share code? If it's hard to do that, at least document that it's
> similar.

I don't think they're same. Actually this block of code does exactly the same
thing as chrome::ShowWebDialogInContainer() except it's creating a frameless
window.

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h (right):

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h:32: // Shows the
customarized Bluetooth pairing dialog.
On 2017/02/09 15:18:02, James Cook wrote:
> nit: "customized"
> 
> Also, document parameters, especially container_id.

Done.

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h:33: static void
ShowDialog(int container_id,
On 2017/02/09 15:18:02, James Cook wrote:
> It's kind of weird that there's a "ShowDialog" method on a View class. I think
> the Show method should be on the Dialog class.

In this case we don't need this view class at all. Removed it...

https://codereview.chromium.org/2684793004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/bluetooth/bluetooth_pairing_view.h:39:
DISALLOW_COPY_AND_ASSIGN(BluetoothPairingView);
On 2017/02/09 15:18:02, James Cook wrote:
> #include macros.h

Done.

[xdai1, 2017-02-09 23:01:31]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 23:02:20]

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

[stevenjb, 2017-02-09 23:06:07]

chrome/browser/resources/ lgtm

[James Cook, 2017-02-09 23:14:39]

LGTM

[commit-bot: I haz the power, 2017-02-10 00:54:53]

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

[commit-bot: I haz the power, 2017-02-10 00:54:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xdai1, 2017-02-10 17:39:50]

Description was changed from

==========
[Harmony] Harmony Bluetooth dialogs.

BUG=680381
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Harmony] Harmonize Bluetooth dialogs.

BUG=680381
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[xdai1, 2017-02-10 19:31:13]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2017-02-10 19:32:59]

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, 2017-02-10 19:49:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486755073110410,
"parent_rev": "11779b13c335bea1ebe2d09629be2988578dd4b0", "commit_rev":
"36bf620f1e9240fea9df9ab0e07bd650bb392bcc"}

[commit-bot: I haz the power, 2017-02-10 19:49:53]

Description was changed from

==========
[Harmony] Harmonize Bluetooth dialogs.

BUG=680381
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Harmony] Harmonize Bluetooth dialogs.

BUG=680381
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2684793004
Cr-Commit-Position: refs/heads/master@{#449702}
Committed:
https://chromium.googlesource.com/chromium/src/+/36bf620f1e9240fea9df9ab0e07b...
==========

[commit-bot: I haz the power, 2017-02-10 19:49:58]

Committed patchset #2 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/36bf620f1e9240fea9df9ab0e07b...