bluetooth: Change the status of the chooser once discovery has ended

bluetooth: Change the status of the chooser once discovery has ended

This adds a new function to ItemChooserDialog to notify it that the discovery
has ended and no new devices will be added. This also adds a new string to show
when the discovery has ended.

BUG=580346
Committed: https://crrev.com/1fb63967d0f5627010a41b9e84b4fa39add9fad8
Cr-Commit-Position: refs/heads/master@{#372998}

[ortuno, 2016-01-29 22:20:01]

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

[ortuno, 2016-01-29 22:20:01]

jyasskin: PTAL

[ortuno, 2016-01-29 22:21:07]

Description was changed from

==========
bluetooth: Change the status of the chooser once discovery has ended

BUG=
==========

to

==========
bluetooth: Change the status of the chooser once discovery has ended

This adds a new function to ItemChooserDialog to notify it that the discovery
has ended and no new devices will be added. This also adds a new string to show
when the discovery has ended.

BUG=580346
==========

[Jeffrey Yasskin, 2016-01-29 22:46:28]

LGTM, but +Jun to make sure this works for the USB chooser too.

https://codereview.chromium.org/1653533002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1653533002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1050: <message
name="IDS_BLUETOOTH_NOT_SEEING_IT" desc="The message to show at the bottom of
the dialog when Bluetooth scanning has uncovered some items.">
"…has uncovered some items but is still scanning for others."

[ortuno, 2016-01-29 23:16:00]

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

[ortuno, 2016-01-29 23:16:00]

newt: PTAL

[ortuno, 2016-01-30 01:12:57]

ah now that I realize that there are tests I need some more time to fix them.
Will ping back when finished.

[Finnur, 2016-02-01 12:16:42]

finnur@chromium.org changed reviewers:
+ finnur@chromium.org

[Finnur, 2016-02-01 12:16:43]

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

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:77:
// The message to show while there are no results.
This comment that you are changing was carefully worded to be technically
correct. As I recall, we show this message until we know there's no more data
coming. Maybe things have changed, but that at least used to mean we could get a
fast response from one device and a slow response from another and this message
would show throughout. Therefore, 'while there are no results' is misleading,
no? Or is this now changed/changing?

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(left):

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:139:
assertFalse(button.isEnabled());
You CL description makes it sound like you are only changing the end-state, yet
this indicates you seem to be changing start-state as well...

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

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:153:

nit: Extra line.

[ortuno, 2016-02-01 17:00:31]

Thanks for looking into the CL finnur!

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

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:77:
// The message to show while there are no results.
On 2016/02/01 at 12:16:43, Finnur wrote:
> This comment that you are changing was carefully worded to be technically
correct. As I recall, we show this message until we know there's no more data
coming. Maybe things have changed, but that at least used to mean we could get a
fast response from one device and a slow response from another and this message
would show throughout. Therefore, 'while there are no results' is misleading,
no? Or is this now changed/changing?

Interesting, that's not how it's implemented right now or shown in the mocks. As
soon as you find a device the status changes to mStatus, "Not seeing your
device? Search again or Get help." Which seemed weird (what you describe makes
more sense) so I opened an issue http://crbug.com/580346. In the end we decided
to show a slight variation of mStatus while the scan is still ongoing and show a
what used to be mStatus once the scan is complete.

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(left):

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:139:
assertFalse(button.isEnabled());
On 2016/02/01 at 12:16:43, Finnur wrote:
> You CL description makes it sound like you are only changing the end-state,
yet this indicates you seem to be changing start-state as well...

In this part of the test we sent an empty list to the chooser to indicate that
no devices were found. Since we now have a separate function to indicate that no
more devices will be found I removed this part of the test. Not sure how this
changes the start-state which is tested above.

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

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:153:

On 2016/02/01 at 12:16:43, Finnur wrote:
> nit: Extra line.

Done.

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:180:
assertEquals(View.GONE, items.getVisibility());
quick question: Why do you need to make the list visible? I couldn't see any
differences between keeping it hidden or making it visible.

[newt (away), 2016-02-01 18:14:35]

https://codereview.chromium.org/1653533002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/1653533002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:367:
public void discoveryIdle() {
How about calling this setIdleState(), similar to "setErrorState()" below?

(Or setDiscoveryIdle(). I feel like "set" needs to be in the name in any case;
"discoveryIdle()" sounds like a getter that returns a boolean value.)

https://codereview.chromium.org/1653533002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:417:
mStatus.setText(mLabels.mStatusIdle);
nit: put mStatus.setText() at the top of this case block to mimic the ordering
in the other case blocks.

[ortuno, 2016-02-02 00:11:42]

https://codereview.chromium.org/1653533002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/1653533002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:367:
public void discoveryIdle() {
On 2016/02/01 at 18:14:35, newt wrote:
> How about calling this setIdleState(), similar to "setErrorState()" below?
> 
> (Or setDiscoveryIdle(). I feel like "set" needs to be in the name in any case;
"discoveryIdle()" sounds like a getter that returns a boolean value.)

Done.

https://codereview.chromium.org/1653533002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:417:
mStatus.setText(mLabels.mStatusIdle);
On 2016/02/01 at 18:14:35, newt wrote:
> nit: put mStatus.setText() at the top of this case block to mimic the ordering
in the other case blocks.

Done.

[newt (away), 2016-02-02 00:14:22]

lgtm from my side

[Finnur, 2016-02-02 17:02:52]

LGTM

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

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:77:
// The message to show while there are no results.
OK, the implementation probably changed so the comment is now out of date. No
worries.

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

https://codereview.chromium.org/1653533002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:180:
assertEquals(View.GONE, items.getVisibility());
Not sure (long since forgotten).

[ortuno, 2016-02-02 18:16:30]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-02-02 18:16:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, finnur@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1653533002/#ps140001
(title: "Merge with ToT")

[commit-bot: I haz the power, 2016-02-02 18:18:01]

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

[commit-bot: I haz the power, 2016-02-02 19:02:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-02 19:03:37]

Description was changed from

==========
bluetooth: Change the status of the chooser once discovery has ended

This adds a new function to ItemChooserDialog to notify it that the discovery
has ended and no new devices will be added. This also adds a new string to show
when the discovery has ended.

BUG=580346
==========

to

==========
bluetooth: Change the status of the chooser once discovery has ended

This adds a new function to ItemChooserDialog to notify it that the discovery
has ended and no new devices will be added. This also adds a new string to show
when the discovery has ended.

BUG=580346

Committed: https://crrev.com/1fb63967d0f5627010a41b9e84b4fa39add9fad8
Cr-Commit-Position: refs/heads/master@{#372998}
==========

[commit-bot: I haz the power, 2016-02-02 19:03:38]

Patchset 8 (id:??) landed as
https://crrev.com/1fb63967d0f5627010a41b9e84b4fa39add9fad8
Cr-Commit-Position: refs/heads/master@{#372998}