Update device name in chooser when device changes name (desktops)

Update device name in chooser when device changes name (desktops)

This CL modified code to update device name in chooser when device
changes its name. Also added test code to cover the changed code.

BUG=634366
Committed: https://crrev.com/bb5ba23019e1c6cd772c51c34a35104a3e39dd99
Cr-Commit-Position: refs/heads/master@{#412290}

[juncai, 2016-08-12 18:19:40]

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

[commit-bot: I haz the power, 2016-08-12 18:20:04]

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

[juncai, 2016-08-12 18:29:51]

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

[juncai, 2016-08-12 18:29:52]

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

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

jyasskin@chromium.org: Please review changes in 
//chrome/browser/choose_controller/
//chrome/browser/ui/bluetooth/

[commit-bot: I haz the power, 2016-08-12 19:16:07]

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

[commit-bot: I haz the power, 2016-08-12 19:16:08]

Dry run: This issue passed the CQ dry run.

[msw, 2016-08-12 22:19:34]

Some mostly minor nits and qs.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:49: // Called when
|index|th option needs to be updated.
nit: "Called when the option at |index| has been updated." (if correct)

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/mock_chooser_controller.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/mock_chooser_controller.cc:99: for (auto it =
option_names_.begin(); it != option_names_.end(); ++it) {
optional nit: use std::find here too.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:177: //
TODO(ortuno): Update device's information.
q: can you remove this TODO now that we're updating the device name?
(ditto for the TODO in the header)

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:199:
view()->OnOptionUpdated(index);
Do we want to report that the option was updated if the name wasn't changed? Why
would we ever supply false for |should_update_name|?

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:176:
EXPECT_EQ(-1, table_view_.selectedRow);
q: Can you check that the second item is now "d" instead of "b"?

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:213:
EXPECT_EQ(-1, table_view_.selectedRow);
ditto q: can you check the values in the list?

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:342:
ASSERT_TRUE(connect_button_.enabled);
ditto q: can you check the name of the selected option?

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:102:
table_view_->OnItemsChanged(static_cast<int>(index), 1);
nit q: can you use or [D]CHECK something from base/numerics/safe_conversions.h
here and elsewhere static casting size_t -> int?

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
File chrome/browser/ui/views/chooser_content_view_unittest.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view_unittest.cc:338: EXPECT_EQ(1,
table_view_->SelectedRowCount());
ditto q: can you check that the selected item's name was updated?

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

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view_unittest.cc:191:
EXPECT_TRUE(cancel_button_->enabled());
optional nit: remove "d" after this and check that ok_button is disabled.

[juncai, 2016-08-15 18:08:58]

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

[commit-bot: I haz the power, 2016-08-15 18:09:21]

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

[juncai, 2016-08-15 18:11:19]

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:49: // Called when
|index|th option needs to be updated.
On 2016/08/12 22:19:33, msw wrote:
> nit: "Called when the option at |index| has been updated." (if correct)

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/mock_chooser_controller.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/mock_chooser_controller.cc:99: for (auto it =
option_names_.begin(); it != option_names_.end(); ++it) {
On 2016/08/12 22:19:33, msw wrote:
> optional nit: use std::find here too.

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:177: //
TODO(ortuno): Update device's information.
On 2016/08/12 22:19:33, msw wrote:
> q: can you remove this TODO now that we're updating the device name?
> (ditto for the TODO in the header)

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:199:
view()->OnOptionUpdated(index);
On 2016/08/12 22:19:33, msw wrote:
> Do we want to report that the option was updated if the name wasn't changed?
Why
> would we ever supply false for |should_update_name|?

Regarding the first question, if the |device_name| is the same as the
|previous_device_name|, it still needs to call view()->OnOptionUpdated(index);
since its connection or paired status or RSSI may be changed.

Regarding the second question, the |device_name| gets value from:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=bl...

And sometimes when a Bluetooth device stops advertising, it will return a string
like "Unknown device" or "Unsupported device". So in this case,
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=bl...
will return that the name is not valid, so it will pass false to
|should_update_name|, and it will not update the device's name.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:176:
EXPECT_EQ(-1, table_view_.selectedRow);
On 2016/08/12 22:19:34, msw wrote:
> q: Can you check that the second item is now "d" instead of "b"?

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:213:
EXPECT_EQ(-1, table_view_.selectedRow);
On 2016/08/12 22:19:34, msw wrote:
> ditto q: can you check the values in the list?

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:342:
ASSERT_TRUE(connect_button_.enabled);
On 2016/08/12 22:19:33, msw wrote:
> ditto q: can you check the name of the selected option?

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:102:
table_view_->OnItemsChanged(static_cast<int>(index), 1);
On 2016/08/12 22:19:34, msw wrote:
> nit q: can you use or [D]CHECK something from base/numerics/safe_conversions.h
> here and elsewhere static casting size_t -> int?

Done.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
File chrome/browser/ui/views/chooser_content_view_unittest.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view_unittest.cc:338: EXPECT_EQ(1,
table_view_->SelectedRowCount());
On 2016/08/12 22:19:34, msw wrote:
> ditto q: can you check that the selected item's name was updated?

Done.

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

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/chooser_dialog_view_unittest.cc:191:
EXPECT_TRUE(cancel_button_->enabled());
On 2016/08/12 22:19:34, msw wrote:
> optional nit: remove "d" after this and check that ok_button is disabled.

Done.

[commit-bot: I haz the power, 2016-08-15 18:50:32]

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

[commit-bot: I haz the power, 2016-08-15 18:50:33]

Dry run: This issue passed the CQ dry run.

[msw, 2016-08-15 19:04:58]

nice! lgtm with a decl comment for |should_update_name|.

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:199:
view()->OnOptionUpdated(index);
On 2016/08/15 18:11:19, juncai wrote:
> On 2016/08/12 22:19:33, msw wrote:
> > Do we want to report that the option was updated if the name wasn't changed?
> Why
> > would we ever supply false for |should_update_name|?
> 
> Regarding the first question, if the |device_name| is the same as the
> |previous_device_name|, it still needs to call view()->OnOptionUpdated(index);
> since its connection or paired status or RSSI may be changed.

Okay, good to know; thanks.

> Regarding the second question, the |device_name| gets value from:
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=bl...
> 
> And sometimes when a Bluetooth device stops advertising, it will return a
string
> like "Unknown device" or "Unsupported device". So in this case,
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=bl...
> will return that the name is not valid, so it will pass false to
> |should_update_name|, and it will not update the device's name.

Ah, okay, please add a comment to that effect at the function decl.

[Robert Sesek, 2016-08-15 19:05:29]

cocoa LGTM

[Jeffrey Yasskin, 2016-08-15 21:33:53]

Definitely fix the checked_cast comment before committing. device_name_map_
could be a separate CL.

LGTM

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

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:186:
device_name_map_.erase(it);
I wish device_name_map_ had a more descriptive name. It's a count of devices
with a given name, right? So device_name_counts_ or
device_name_repetition_counts_?

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:178:
EXPECT_EQ(base::ASCIIToUTF16("d"), chooser_controller_->GetOption(1));
For the cocoa controller test, can you check that the table_view_ is showing a
"d", instead of just that the chooser_controller_ thinks it should be showing a
"d"?

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

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:68: base::checked_cast<int,
size_t>(chooser_controller_->NumOptions()), 1);
Don't specify the second template parameter to checked_cast. That would cause an
implicit unchecked cast to that type, followed by a checked type to the target
type, while we just want a single checked cast.

[juncai, 2016-08-15 23:50:31]

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

[commit-bot: I haz the power, 2016-08-15 23:50:52]

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

[juncai, 2016-08-15 23:53:33]

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

[juncai, 2016-08-15 23:53:35]

jam@chromium.org: Please review changes in 

//content/public/browser/bluetooth_chooser.h

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2242943002/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:199:
view()->OnOptionUpdated(index);
On 2016/08/15 19:04:58, msw wrote:
> On 2016/08/15 18:11:19, juncai wrote:
> > On 2016/08/12 22:19:33, msw wrote:
> > > Do we want to report that the option was updated if the name wasn't
changed?
> > Why
> > > would we ever supply false for |should_update_name|?
> > 
> > Regarding the first question, if the |device_name| is the same as the
> > |previous_device_name|, it still needs to call
view()->OnOptionUpdated(index);
> > since its connection or paired status or RSSI may be changed.
> 
> Okay, good to know; thanks.
> 
> > Regarding the second question, the |device_name| gets value from:
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=bl...
> > 
> > And sometimes when a Bluetooth device stops advertising, it will return a
> string
> > like "Unknown device" or "Unsupported device". So in this case,
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=bl...
> > will return that the name is not valid, so it will pass false to
> > |should_update_name|, and it will not update the device's name.
> 
> Ah, okay, please add a comment to that effect at the function decl.

Done.

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

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:186:
device_name_map_.erase(it);
On 2016/08/15 21:33:53, Jeffrey Yasskin wrote:
> I wish device_name_map_ had a more descriptive name. It's a count of devices
> with a given name, right? So device_name_counts_ or
> device_name_repetition_counts_?

Yes, it is a count of devices with a given name.

Done.

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:178:
EXPECT_EQ(base::ASCIIToUTF16("d"), chooser_controller_->GetOption(1));
On 2016/08/15 21:33:53, Jeffrey Yasskin wrote:
> For the cocoa controller test, can you check that the table_view_ is showing a
> "d", instead of just that the chooser_controller_ thinks it should be showing
a
> "d"?

Done.

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

https://codereview.chromium.org/2242943002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:68: base::checked_cast<int,
size_t>(chooser_controller_->NumOptions()), 1);
On 2016/08/15 21:33:53, Jeffrey Yasskin wrote:
> Don't specify the second template parameter to checked_cast. That would cause
an
> implicit unchecked cast to that type, followed by a checked type to the target
> type, while we just want a single checked cast.

Done.

[commit-bot: I haz the power, 2016-08-16 01:21:35]

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

[commit-bot: I haz the power, 2016-08-16 01:21:36]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-08-16 17:28:09]

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

[juncai, 2016-08-16 17:28:10]

Noticed that jam@ is OOO, added avi@chromium.org as the reviewer.

avi@chromium.org, Please review changes in 
//content/public/browser/bluetooth_chooser.h

Thanks!

[Avi (use Gerrit), 2016-08-16 17:29:59]

content api change lgtm

[juncai, 2016-08-16 17:56:47]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-08-16 17:56:48]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
msw@chromium.org, jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/2242943002/#ps40001
(title: "address more comments")

[commit-bot: I haz the power, 2016-08-16 17:57:10]

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-08-16 18:01:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-16 18:04:48]

Description was changed from

==========
Update device name in chooser when device changes name (desktops)

This CL modified code to update device name in chooser when device
changes its name. Also added test code to cover the changed code.

BUG=634366
==========

to

==========
Update device name in chooser when device changes name (desktops)

This CL modified code to update device name in chooser when device
changes its name. Also added test code to cover the changed code.

BUG=634366

Committed: https://crrev.com/bb5ba23019e1c6cd772c51c34a35104a3e39dd99
Cr-Commit-Position: refs/heads/master@{#412290}
==========

[commit-bot: I haz the power, 2016-08-16 18:04:49]

Patchset 3 (id:??) landed as
https://crrev.com/bb5ba23019e1c6cd772c51c34a35104a3e39dd99
Cr-Commit-Position: refs/heads/master@{#412290}