bluetooth: Update the item's description on android.

bluetooth: Update the item's description on android.

For example when a device changes names we want to show the new name.

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

[ortuno, 2016-08-17 21:46:22]

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

[commit-bot: I haz the power, 2016-08-17 21:46:46]

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, 2016-08-17 22:34:30]

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

[commit-bot: I haz the power, 2016-08-17 22:34:31]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-17 22:34:53]

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

[ortuno, 2016-08-17 22:34:54]

juncai: PTAL

[juncai, 2016-08-17 23:55:36]

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:166:
ItemChooserRow old_item = mKeyToItemMap.get(item.mKey);
s/old_item/oldItem
Java local variable uses lowerCamelCase.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:177:
notifyDataSetChanged();
Move notifyDataSetChanged(); inside the above if statement?

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:206:
public void testAddOrUpdateItemAndRemoveItemFromList() throws
InterruptedException {
In general, I am wondering if it needs to test the button enabled/disabled state
when the selected item is removed/updated? Will the "PAIR" button get disabled
if a selected item is removed?

If so, can do it in a separate CL.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:268:
mChooserDialog.removeItemFromList(item1);
Here it needs to pass |item1_again| instead of |item1| since its description was
changed. The |mKeyToItemMap| is updated, but |mItemDescriptionMap| isn't updated
correctly.

[ortuno, 2016-08-18 20:48:35]

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

[ortuno, 2016-08-18 20:48:37]

Thanks!

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:166:
ItemChooserRow old_item = mKeyToItemMap.get(item.mKey);
On 2016/08/17 at 23:55:35, juncai wrote:
> s/old_item/oldItem
> Java local variable uses lowerCamelCase.

Done.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:177:
notifyDataSetChanged();
On 2016/08/17 at 23:55:35, juncai wrote:
> Move notifyDataSetChanged(); inside the above if statement?

I was thinking that this code would eventually look like the following:

if (oldItem != null) {
   if (oldItem.equals(item)) {
     return;
   }
   if (!oldItem.mDescription.equals(item.mDescription)) {
     // Update description.
   }
   if (!oldItem.mIsPaired.equals(item.mIsPaired)) {
     // Update paired.
   }
   if (!oldItem.mIcon.equals(item.mIcon)) {
     // Update rssi.
   }
   notifyDataSetChanged();
}

That's why I just kept that call outside of the if statement.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:206:
public void testAddOrUpdateItemAndRemoveItemFromList() throws
InterruptedException {
On 2016/08/17 at 23:55:35, juncai wrote:
> In general, I am wondering if it needs to test the button enabled/disabled
state when the selected item is removed/updated? Will the "PAIR" button get
disabled if a selected item is removed?
> 
> If so, can do it in a separate CL.

Opened issue http://crbug.com/639057

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:268:
mChooserDialog.removeItemFromList(item1);
On 2016/08/17 at 23:55:35, juncai wrote:
> Here it needs to pass |item1_again| instead of |item1| since its description
was changed. The |mKeyToItemMap| is updated, but |mItemDescriptionMap| isn't
updated correctly.

Actually item1.mDescription is "desc1_again" because we don't replace the
previous item with the new item we just update its fields. But  we wouldn't have
caught a bug in which mDescription was incorrect. I've changed remove() to
remove the item by using only the key and changed IsEmpty to assert that if the
ArrayAdapter is empty all the other data structures are empty and vice versa.

[commit-bot: I haz the power, 2016-08-18 20:49:05]

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, 2016-08-18 21:21:20]

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

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

Dry run: This issue passed the CQ dry run.

[juncai, 2016-08-19 00:41:21]

LGTM.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:177:
notifyDataSetChanged();
On 2016/08/18 20:48:37, ortuno wrote:
> On 2016/08/17 at 23:55:35, juncai wrote:
> > Move notifyDataSetChanged(); inside the above if statement?
> 
> I was thinking that this code would eventually look like the following:
> 
> if (oldItem != null) {
>    if (oldItem.equals(item)) {
>      return;
>    }
>    if (!oldItem.mDescription.equals(item.mDescription)) {
>      // Update description.
>    }
>    if (!oldItem.mIsPaired.equals(item.mIsPaired)) {
>      // Update paired.
>    }
>    if (!oldItem.mIcon.equals(item.mIcon)) {
>      // Update rssi.
>    }
>    notifyDataSetChanged();
> }
> 
> That's why I just kept that call outside of the if statement.

I see. Acknowledged.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:206:
public void testAddOrUpdateItemAndRemoveItemFromList() throws
InterruptedException {
On 2016/08/18 20:48:37, ortuno wrote:
> On 2016/08/17 at 23:55:35, juncai wrote:
> > In general, I am wondering if it needs to test the button enabled/disabled
> state when the selected item is removed/updated? Will the "PAIR" button get
> disabled if a selected item is removed?
> > 
> > If so, can do it in a separate CL.
> 
> Opened issue http://crbug.com/639057

Acknowledged.

https://codereview.chromium.org/2256893003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:268:
mChooserDialog.removeItemFromList(item1);
On 2016/08/18 20:48:37, ortuno wrote:
> On 2016/08/17 at 23:55:35, juncai wrote:
> > Here it needs to pass |item1_again| instead of |item1| since its description
> was changed. The |mKeyToItemMap| is updated, but |mItemDescriptionMap| isn't
> updated correctly.
> 
> Actually item1.mDescription is "desc1_again" because we don't replace the
> previous item with the new item we just update its fields. But  we wouldn't
have
> caught a bug in which mDescription was incorrect. I've changed remove() to
> remove the item by using only the key and changed IsEmpty to assert that if
the
> ArrayAdapter is empty all the other data structures are empty and vice versa.

Acknowledged.

[ortuno, 2016-08-19 00:55:13]

ortuno@chromium.org changed reviewers:
+ tedchoc@chromium.org

[ortuno, 2016-08-19 00:55:14]

tedchoc: PTAL

[Ted C, 2016-08-19 21:04:48]

lgtm

https://codereview.chromium.org/2256893003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2256893003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:167:
if (super.isEmpty()) {
I would cache super.isEmpty() before this and return it at the end.

https://codereview.chromium.org/2256893003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:204:
return;
in java, you can put the condition and statement all on a single line if it fits

[ortuno, 2016-08-23 00:35:31]

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

[commit-bot: I haz the power, 2016-08-23 00:36:21]

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, 2016-08-23 01:35:24]

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

[commit-bot: I haz the power, 2016-08-23 01:35:25]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-23 01:42:00]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-08-23 01:42:00]

Thanks!

https://codereview.chromium.org/2256893003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/2256893003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:167:
if (super.isEmpty()) {
On 2016/08/19 at 21:04:47, Ted C wrote:
> I would cache super.isEmpty() before this and return it at the end.

Done.

https://codereview.chromium.org/2256893003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:204:
return;
On 2016/08/19 at 21:04:47, Ted C wrote:
> in java, you can put the condition and statement all on a single line if it
fits

Done.

[ortuno, 2016-08-23 01:42:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, juncai@chromium.org
Link to the patchset: https://codereview.chromium.org/2256893003/#ps60001
(title: "Address tedchoc's feedback")

[commit-bot: I haz the power, 2016-08-23 01:42:55]

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-23 01:48:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-23 01:50:55]

Description was changed from

==========
bluetooth: Update the item's description on android.

For example when a device changes names we want to show the new name.

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

to

==========
bluetooth: Update the item's description on android.

For example when a device changes names we want to show the new name.

BUG=634366

Committed: https://crrev.com/54227ff72da835d9c6f6d9a46d299fb3cdb79d7f
Cr-Commit-Position: refs/heads/master@{#413622}
==========

[commit-bot: I haz the power, 2016-08-23 01:50:56]

Patchset 4 (id:??) landed as
https://crrev.com/54227ff72da835d9c6f6d9a46d299fb3cdb79d7f
Cr-Commit-Position: refs/heads/master@{#413622}