bluetooth-chooser: Refactor and clean up bluetooth chooser

bluetooth-chooser: Refactor and clean up bluetooth chooser

In preparation for adding icons, this patch:

 1. No longer creates a new object for each update, instead passes the
    information directly to ItemChooserDialog.
 2. Sets choice mode to CHOICE_MODE_SINGLE.
 3. Uses color sets to control text and background color.
 4. Uses RelativeLayout for rows in preparation of adding an ImageView to the layout.

BUG=629689
Committed: https://crrev.com/af8b667fa26cc740acd20d13fa56ce9fce8a4ad6
Cr-Commit-Position: refs/heads/master@{#426710}

[ortuno, 2016-10-18 23:14:59]

Description was changed from

==========
bluetooth-chooser: Refactor and clean up bluetooth chooser

In preparation for adding icons, this patch:

 1. No longer creates a new object for each update, instead passes the
    information directly to ItemChooserDialog.
 2. Sets choice mode to CHOICE_MODE_SINGLE.
 3. Uses color sets to control text and background color.

BUG=629689
==========

to

==========
bluetooth-chooser: Refactor and clean up bluetooth chooser

In preparation for adding icons, this patch:

 1. No longer creates a new object for each update, instead passes the
    information directly to ItemChooserDialog.
 2. Sets choice mode to CHOICE_MODE_SINGLE.
 3. Uses color sets to control text and background color.
 4. Uses RelativeLayout for rows in preparation of adding an ImageView to the
layout.

BUG=629689
==========

[ortuno, 2016-10-18 23:15:32]

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

[ortuno, 2016-10-18 23:15:33]

ianwen: The refactoring was getting big so I split it up. PTAL

[ortuno, 2016-10-18 23:15:43]

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

[commit-bot: I haz the power, 2016-10-18 23:16:30]

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-10-19 00:23:34]

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

[commit-bot: I haz the power, 2016-10-19 00:23:34]

Dry run: This issue passed the CQ dry run.

[Ian Wen, 2016-10-19 00:28:46]

Wow bravo! Awesome work!!

I only have some nits to say.

LGTM!

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

https://codereview.chromium.org/2426573005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:73:
public boolean hasContents(String key, String description) {
Nit: how about renaming the function to "sameAs" or "hasContentAs" or
"hasSameContent"?

https://codereview.chromium.org/2426573005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:115:
// Item holder for performance boost.
Nit: use javadoc style instead of //.

https://codereview.chromium.org/2426573005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:188:
ItemChooserRow result = mKeyToItemMap.put(key, newItem);
Nit: remove #188 and #189 as it's apparent. Or change to assert
mKeyToItemMap.containsKey(key) == false and put it above #187;

https://codereview.chromium.org/2426573005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:473:
* was not in the chooser. Otherwise update the items description.
Nit: s/update/updates

https://codereview.chromium.org/2426573005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:485:
* Remove an item that is shown in the dialog.
Nit: s/Remove/Removes

[ortuno, 2016-10-19 01:59:33]

Thanks!

https://chromiumcodereview.appspot.com/2426573005/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://chromiumcodereview.appspot.com/2426573005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:73:
public boolean hasContents(String key, String description) {
On 2016/10/19 at 00:28:46, Ian Wen wrote:
> Nit: how about renaming the function to "sameAs" or "hasContentAs" or
"hasSameContent"?

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:115:
// Item holder for performance boost.
On 2016/10/19 at 00:28:46, Ian Wen wrote:
> Nit: use javadoc style instead of //.

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:188:
ItemChooserRow result = mKeyToItemMap.put(key, newItem);
On 2016/10/19 at 00:28:46, Ian Wen wrote:
> Nit: remove #188 and #189 as it's apparent. Or change to assert
mKeyToItemMap.containsKey(key) == false and put it above #187;

Maybe I'm missing something but I don't think I can remove #188 since I still
need to add the item to the map. I changed the assert and moved it above #187
let me know if you meant something different.

https://chromiumcodereview.appspot.com/2426573005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:473:
* was not in the chooser. Otherwise update the items description.
On 2016/10/19 at 00:28:46, Ian Wen wrote:
> Nit: s/update/updates

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:485:
* Remove an item that is shown in the dialog.
On 2016/10/19 at 00:28:46, Ian Wen wrote:
> Nit: s/Remove/Removes

Done.

[ortuno, 2016-10-19 01:59:39]

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

[commit-bot: I haz the power, 2016-10-19 02:00:21]

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

[ortuno, 2016-10-19 02:01:12]

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

[ortuno, 2016-10-19 02:01:12]

juncai: PTAL

[commit-bot: I haz the power, 2016-10-19 03:42:18]

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

[commit-bot: I haz the power, 2016-10-19 03:42:18]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-10-19 21:35:23]

LGTM with nits.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:136:
mChooserDialog.addOrUpdateItem("key", "key");
nit: to be consistent with the remaining test code:
("key", "key") -> ("key1", "desc1")

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:137:
mChooserDialog.addOrUpdateItem("key2", "key2");
ditto:
("key2", "key2") -> ("key2", "desc2")

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:194:
mChooserDialog.addOrUpdateItem("key", "key");
ditto.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:195:
mChooserDialog.addOrUpdateItem("key2", "key2");
ditto.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:301:
public void testUpdateItemAndRemove() throws InterruptedException {
nit: to be consistent with the remaining function names, maybe change it to be:
testUpdateItemAndRemoveItemFromList().

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:325:
// Add item 1 with different description.
nit: since the following code is to update the item's description, maybe change
the comment to be: "Update item 1 with different description."

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:349:
public void testAddOrUpdateItemAndRemoveItemFromList() throws
InterruptedException {
nit: this function doesn't test Update item, so maybe change the function name
to be: testAddItemAndRemoveItemFromList().

[ortuno, 2016-10-20 00:24:42]

Thanks!

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:136:
mChooserDialog.addOrUpdateItem("key", "key");
On 2016/10/19 at 21:35:22, juncai wrote:
> nit: to be consistent with the remaining test code:
> ("key", "key") -> ("key1", "desc1")

Done

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:137:
mChooserDialog.addOrUpdateItem("key2", "key2");
On 2016/10/19 at 21:35:23, juncai wrote:
> ditto:
> ("key2", "key2") -> ("key2", "desc2")

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:194:
mChooserDialog.addOrUpdateItem("key", "key");
On 2016/10/19 at 21:35:22, juncai wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:195:
mChooserDialog.addOrUpdateItem("key2", "key2");
On 2016/10/19 at 21:35:23, juncai wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:301:
public void testUpdateItemAndRemove() throws InterruptedException {
On 2016/10/19 at 21:35:23, juncai wrote:
> nit: to be consistent with the remaining function names, maybe change it to
be: testUpdateItemAndRemoveItemFromList().

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:325:
// Add item 1 with different description.
On 2016/10/19 at 21:35:23, juncai wrote:
> nit: since the following code is to update the item's description, maybe
change the comment to be: "Update item 1 with different description."

Done.

https://chromiumcodereview.appspot.com/2426573005/diff/20001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:349:
public void testAddOrUpdateItemAndRemoveItemFromList() throws
InterruptedException {
On 2016/10/19 at 21:35:23, juncai wrote:
> nit: this function doesn't test Update item, so maybe change the function name
to be: testAddItemAndRemoveItemFromList().

Done.

[ortuno, 2016-10-20 00:24:52]

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

[ortuno, 2016-10-20 00:25:10]

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

[ortuno, 2016-10-20 00:25:10]

tedchoc: Owners review please.

[commit-bot: I haz the power, 2016-10-20 00:25:34]

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-10-20 01:32:25]

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

[commit-bot: I haz the power, 2016-10-20 01:32:26]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-10-20 21:50:19]

lgtm w/ nits

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

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:64:
public static class ItemChooserRow {
should/can this class be private now?

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:73:
public boolean hasSameContents(String key, String description) {
non-private methods should have javadoc

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:74:
if (!mKey.equals(key)) return false;
use TextUtils.equals

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:179:
if (!oldItem.mDescription.equals(description)) {
I would use TextUtils.equals here instead (incase we ever want to support a null
mDescription

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:197:
public void remove(String key) {
I would try to avoid overloads of names in the public api for android classes. 
I could see this being confusing.

Maybe this should be removeEntryWithKey to make it clear that it is not the
same.

[ortuno, 2016-10-21 02:14:06]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-10-21 02:14:07]

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

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:64:
public static class ItemChooserRow {
On 2016/10/20 at 21:50:18, Ted C wrote:
> should/can this class be private now?

Done.

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:73:
public boolean hasSameContents(String key, String description) {
On 2016/10/20 at 21:50:18, Ted C wrote:
> non-private methods should have javadoc

Tests use hasSameContents and a follow up patch will introduce a function that
returns an instance of this class:
https://github.com/jollycopper/ListViewColorState/blob/master/app/src/main/ja...

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:74:
if (!mKey.equals(key)) return false;
On 2016/10/20 at 21:50:18, Ted C wrote:
> use TextUtils.equals

Done.

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:179:
if (!oldItem.mDescription.equals(description)) {
On 2016/10/20 at 21:50:18, Ted C wrote:
> I would use TextUtils.equals here instead (incase we ever want to support a
null mDescription

Ah good idea. I didn't know about TextUtils. Done.

https://codereview.chromium.org/2426573005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:197:
public void remove(String key) {
On 2016/10/20 at 21:50:18, Ted C wrote:
> I would try to avoid overloads of names in the public api for android classes.
 I could see this being confusing.
> 
> Maybe this should be removeEntryWithKey to make it clear that it is not the
same.

Done.

[ortuno, 2016-10-21 02:14:08]

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

[commit-bot: I haz the power, 2016-10-21 02:14:41]

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-10-21 03:38:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:26:48]

Description was changed from

==========
bluetooth-chooser: Refactor and clean up bluetooth chooser

In preparation for adding icons, this patch:

 1. No longer creates a new object for each update, instead passes the
    information directly to ItemChooserDialog.
 2. Sets choice mode to CHOICE_MODE_SINGLE.
 3. Uses color sets to control text and background color.
 4. Uses RelativeLayout for rows in preparation of adding an ImageView to the
layout.

BUG=629689
==========

to

==========
bluetooth-chooser: Refactor and clean up bluetooth chooser

In preparation for adding icons, this patch:

 1. No longer creates a new object for each update, instead passes the
    information directly to ItemChooserDialog.
 2. Sets choice mode to CHOICE_MODE_SINGLE.
 3. Uses color sets to control text and background color.
 4. Uses RelativeLayout for rows in preparation of adding an ImageView to the
layout.

BUG=629689

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

[commit-bot: I haz the power, 2016-10-21 13:26:49]

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