Implement a bluetooth picker dialog for Android

Implement a bluetooth picker dialog for Android.

Also refactor the ItemChooserDialog just a bit, to account for an error status (for example when the bluetooth adapter is turned off).

BUG=498016, 508293
Committed: https://crrev.com/af43c88d506f641363b9d6a36a55bffbf2104003
Cr-Commit-Position: refs/heads/master@{#347378}

[Finnur, 2015-08-26 14:50:00]

Patchset #1 (id:1) has been deleted

[Finnur, 2015-08-26 14:50:06]

Patchset #1 (id:20001) has been deleted

[Finnur, 2015-08-26 14:50:12]

Patchset #1 (id:40001) has been deleted

[Finnur, 2015-08-26 15:04:15]

Patchset #1 (id:60001) has been deleted

[Finnur, 2015-08-26 15:04:20]

Patchset #1 (id:80001) has been deleted

[Finnur, 2015-08-28 15:49:11]

Patchset #2 (id:120001) has been deleted

[Finnur, 2015-08-28 15:54:00]

Patchset #1 (id:100001) has been deleted

[Finnur, 2015-08-28 15:54:06]

Patchset #1 (id:140001) has been deleted

[Finnur, 2015-08-28 17:29:19]

finnur@chromium.org changed reviewers:
+ jam@chromium.org, jyasskin@chromium.org, newt@chromium.org

[Finnur, 2015-08-28 17:30:11]

Jeffrey: Can you (at a minimum) take bluetooth_dispatcher_host.cc and
bluetooth_chooser.h.

Newton: Can you review the rest?

John: Need OWNERS approval for one-liner in
content/public/browser/bluetooth_chooser.h.

[Jeffrey Yasskin, 2015-08-28 18:16:33]

I haven't done a rigorous review of the Java code, but here are some comments:

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/android...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/android...
chrome/browser/android/chrome_web_contents_delegate_android.cc:114: origin,
web_contents, event_handler));
Unless there's a reason not to, keep these in the same order as in
RunBluetoothChooser()?

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:35:
BluetoothChooserAndroid::~BluetoothChooserAndroid() {
I think this destructor needs to call into Java to make sure it drops its
reference to this object.

The destructor can run before receiving a SELECTED or CANCELED event during
shutdown (in ~BluetoothDispatcherHost), and future maintenance might introduce
other cases, and I'd rather not risk a use-after-free.

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:52:
ScopedJavaLocalRef<jstring> java_device_id = ConvertUTF16ToJavaString(
Use ConvertUTF8ToJavaString() for this one.

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:71: Event::SELECTED,
base::android::ConvertJavaStringToUTF8(env, device_id));
It looks like we're missing a way to report that the user has canceled the
dialog?

https://codereview.chromium.org/1315093002/diff/200001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1315093002/diff/200001/content/public/browser...
content/public/browser/bluetooth_chooser.h:26: RESTART,
Can we call this "RESCAN" or "START_SCAN" or "START_DISCOVERY"? It's not going
to completely reset the dialog, but rather just start a new Bluetooth discovery
session.

[jam, 2015-08-28 20:22:40]

On 2015/08/28 17:30:11, Finnur wrote:
> Jeffrey: Can you (at a minimum) take bluetooth_dispatcher_host.cc and
> bluetooth_chooser.h.
> 
> Newton: Can you review the rest?
> 
> John: Need OWNERS approval for one-liner in
> content/public/browser/bluetooth_chooser.h.

lgtm

[scheib, 2015-08-29 20:15:18]

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

[scheib, 2015-08-29 20:15:19]

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:28:
* A general-purpose dialog for presenting a list of things to pick from.
Update comment to reflect this being the Bluetooth specific impl.

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1013: <message
name="IDS_BLUETOOTH_DIALOG_TITLE" desc="The header message for the dialog.">
As pointed out in chromium dev [1], we should be clearer and provide ample
context in desc messages. 
[1]
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/QmM7_pUHcXk

https://codereview.chromium.org/1315093002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1315093002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:46: const int kDelayTime
= 20;        // 20 seconds for scanning and discovering
This constant also affects the delay for GetPrimaryService calling
OnServicesDiscovered. We shouldn't change the latter to 20 seconds.

Also, on systems using the UI free first match chooser, we should still use 5
seconds.

[Finnur, 2015-08-31 15:25:22]

PTAL

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:28:
* A general-purpose dialog for presenting a list of things to pick from.
On 2015/08/29 20:15:18, scheib wrote:
> Update comment to reflect this being the Bluetooth specific impl.

Done.

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1013: <message
name="IDS_BLUETOOTH_DIALOG_TITLE" desc="The header message for the dialog.">
Fair enough. Is this clearer?

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/android...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/android...
chrome/browser/android/chrome_web_contents_delegate_android.cc:114: origin,
web_contents, event_handler));
On 2015/08/28 18:16:33, Jeffrey Yasskin wrote:
> Unless there's a reason not to, keep these in the same order as in
> RunBluetoothChooser()?

Done.

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:35:
BluetoothChooserAndroid::~BluetoothChooserAndroid() {
Done. Look ok?

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:52:
ScopedJavaLocalRef<jstring> java_device_id = ConvertUTF16ToJavaString(
On 2015/08/28 18:16:33, Jeffrey Yasskin wrote:
> Use ConvertUTF8ToJavaString() for this one.

Done.

https://codereview.chromium.org/1315093002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:71: Event::SELECTED,
base::android::ConvertJavaStringToUTF8(env, device_id));
And a way to report that the help link was pressed, etc. 

It is coming (next CL).

https://codereview.chromium.org/1315093002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1315093002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:46: const int kDelayTime
= 20;        // 20 seconds for scanning and discovering
OK, reverted and I'll leave this up to you guys with the added comment that 5
seconds is a little short when you've realize that your Bluetooth device is off
at the start of the scan (especially since the re-scanning hasn't been
implemented). But, whatever. This is all work in progress. :)

https://codereview.chromium.org/1315093002/diff/200001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1315093002/diff/200001/content/public/browser...
content/public/browser/bluetooth_chooser.h:26: RESTART,
On 2015/08/28 18:16:33, Jeffrey Yasskin wrote:
> Can we call this "RESCAN" or "START_SCAN" or "START_DISCOVERY"? It's not going
> to completely reset the dialog, but rather just start a new Bluetooth
discovery
> session.

Done.

[Jeffrey Yasskin, 2015-08-31 17:03:53]

The C++ side lgtm.

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:220:
mItemChooserDialog.dismiss();
Dumb question: why doesn't this call this.dismiss() or otherwise null out
mNativeBluetoothChooserDialogPtr?

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

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:342:
mItemAdapter.remove(row);
When the Bluetooth adapter reports than a device has been removed, I think we
don't want to remove it from the dialog, since that'll make everything else jump
around. Instead, we need to mark it in some way, and prevent folks from
selecting it. https://code.google.com/p/chromium/issues/detail?id=508904#c9 has
some ideas, but no great answers. :-/

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1018: <message
name="IDS_BLUETOOTH_CONFIRM_BUTTON" desc="The button to confirm association of a
website with a Bluetooth device.">
In English, we're using the same term Bluetooth uses for associating two
Bluetooth devices. So maybe include, "Use the same term as in 'Pair this
Bluetooth headset with your phone.'"

[scheib, 2015-08-31 17:26:12]

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1315093002/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1013: <message
name="IDS_BLUETOOTH_DIALOG_TITLE" desc="The header message for the dialog.">
On 2015/08/31 15:25:22, Finnur wrote:
> Fair enough. Is this clearer?

Yes, Thanks.

[Finnur, 2015-09-01 11:53:50]

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:220:
mItemChooserDialog.dismiss();
Not a dumb question at all. Fixed.

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

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:342:
mItemAdapter.remove(row);
Changed it (for now) so that it grays it out instead.

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1315093002/diff/220001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1018: <message
name="IDS_BLUETOOTH_CONFIRM_BUTTON" desc="The button to confirm association of a
website with a Bluetooth device.">
On 2015/08/31 17:03:53, Jeffrey Yasskin wrote:
> In English, we're using the same term Bluetooth uses for associating two
> Bluetooth devices. So maybe include, "Use the same term as in 'Pair this
> Bluetooth headset with your phone.'"

Done.

[Finnur, 2015-09-01 15:48:31]

Due to the time difference I also had time to implement the plumbing for showing
the help links.

PTAL.

[Jeffrey Yasskin, 2015-09-01 17:09:49]

https://codereview.chromium.org/1315093002/diff/290001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1315093002/diff/290001/content/public/browser...
content/public/browser/bluetooth_chooser.h:67: void
ShowOverviewLink(content::WebContents* web_contents);
Can you add these in a separate CL so we don't need to get jam to re-review this
one?

He told me not to put implementation in content/public, which these are, so he
may want us to add an Event enumerator to ask the BluetoothDispatcherHost to
show the links.

[newt (away), 2015-09-01 21:51:23]

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:29:
* A dialog for picking available Bluetooth devices.
Could you add a bit more context here. E.g. when is this dialog shown? / how
does the user or website trigger it?

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:42:
int mSecurityLevel;
What are the valid values for this variable? Are they defined in an enum
somewhere? Please document.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:61:
public BluetoothChooserDialog(Context context, String origin,
If this is only called from create(), then make this constructor and the show()
method private.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:83:
int start = title.toString().indexOf(mOrigin);
Instead of indexOf(), I'd use title.getSpanStart(theClickableSpan).

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:146:
if (mNativeBluetoothChooserDialogPtr != 0) {
This if check is redundant, since you already checked
mNativeBluetoothChooserDialogPtr's nullness at the beginning of the method. Same
for the if checks below.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:199:
Activity activity = windowAndroid.getActivity().get();
windowAndroid.getActivity().get() can be null

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:215:
private void closeDialog() {
If the user closes the dialog via Java (e.g. by pressing back, or even by
Android killing the Activity), will the native side of this object be deleted?
It doesn't appear so currently.

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

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:127:
private Set<String> mUnavailableEntries = new HashSet<String>();
For consistent terminology with Android, let's use "enabled/disabled" instead of
"available/unavailable".

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:171:
private boolean isAvailable(String id) {
ArrayAdapter has a method isEnabled(int position), which you should be
overriding (instead of checking isAvailable() in onItemClick()). If isEnabled()
returns false, then the view's state will be set to disabled and the user won't
be able to click the view. This will also work better with TalkBack for
accessibility.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:274:
mEmptyMessage =
remove wrapping (if possible)

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/android...
File chrome/browser/android/chrome_web_contents_delegate_android.h (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/android...
chrome/browser/android/chrome_web_contents_delegate_android.h:45:
scoped_ptr<content::BluetoothChooser> RunBluetoothChooser(
does this method belong in this class?

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:22: content::WebContents*
web_contents, const EventHandler& event_handler,
params should each be on their own line, I believe

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:57: const base::string16&
device_name) {
fix param indentation

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.h (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.h:29: void
ReportDeviceSelected(JNIEnv* env, jobject obj, jstring device_id);
Suggestion: "Report" doesn't seem to add much meaning to these method names. how
about calling this method "OnDeviceSelected()" and the following method
"RestartSearch()"?

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.h:43: EventHandler
event_handler_;
Can you add "BluetoothChooser::" here to make it clearer where this EventHandler
class is coming from?

[Finnur, 2015-09-02 16:01:43]

Updated, PTAL.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:29:
* A dialog for picking available Bluetooth devices.
On 2015/09/01 21:51:23, newt wrote:
> Could you add a bit more context here. E.g. when is this dialog shown? / how
> does the user or website trigger it?

Done.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:42:
int mSecurityLevel;
On 2015/09/01 21:51:23, newt wrote:
> What are the valid values for this variable? Are they defined in an enum
> somewhere? Please document.

Done.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:61:
public BluetoothChooserDialog(Context context, String origin,
It didn't use to be, but is now. Fixed.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:83:
int start = title.toString().indexOf(mOrigin);
I'n not sure how to use getSpanStart to accomplish this. Let me take a step back
to make sure we're on the same page...

I'm starting with something like:
   "http://foo wants to <link-span>pair with</link-span>:"
... and hoping to convert that into:
   "<color-span>http</color-span>://foo wants to <link-span>pair
with</link-span>:"

getSpanStart would work if I were looking for the 'pair with' link-span, but I'm
not. I'm trying to take a static text and converting it into a color-span (so to
speak), while avoiding splitting the string up into two, which will lead to
translation headaches.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:146:
if (mNativeBluetoothChooserDialogPtr != 0) {
Ah. Forgot about this. Fixed.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:199:
Activity activity = windowAndroid.getActivity().get();
Yes, I thought about that. Can we expect it to be null under normal
circumstances and if so, how?

My thinking was that we'd probably want to know when/if it occurs, rather than
silently fail. But, if this occurs normally, e.g. at shutdown, or something,
then we need to silently fail.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:215:
private void closeDialog() {
The native side is owned by the RequestDeviceSession object so it is cleaned up
alongside that. However, this reminded me that the dismiss handler for the
dialog is missing, it is supposed to notify that nothing was selected. Once that
is added, the requestDevice session is destroyed on dismiss and it takes the
native side down along with it.

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

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:127:
private Set<String> mUnavailableEntries = new HashSet<String>();
On 2015/09/01 21:51:23, newt wrote:
> For consistent terminology with Android, let's use "enabled/disabled" instead
of
> "available/unavailable".

Done.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:171:
private boolean isAvailable(String id) {
Ooh, I like that idea. Unfortunately, it makes it disabled but doesn't gray
anything out, but that's easy to fix.

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:274:
mEmptyMessage =
On 2015/09/01 21:51:23, newt wrote:
> remove wrapping (if possible)

Done.

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/android...
File chrome/browser/android/chrome_web_contents_delegate_android.h (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/android...
chrome/browser/android/chrome_web_contents_delegate_android.h:45:
scoped_ptr<content::BluetoothChooser> RunBluetoothChooser(
I struggled with this quite a bit, and believe it belongs here.

content/browser/bluetooth/bluetooth_dispatcher_host.cc calls
WebContentsDelegate::RunBluetoothChooser, which is what I'm overriding here (in
chrome/browser/chrome_web_contents_delegate_android.h). This layer
(chrome/browser) is needed because my override function needs to instantiate
BluetoothChooserAndroid, which depends on chrome/browser.

If there's a better way, I'm willing to re-consider, but I bounced this off a
couple of people already and this was what was settled on.

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:22: content::WebContents*
web_contents, const EventHandler& event_handler,
On 2015/09/01 21:51:23, newt wrote:
> params should each be on their own line, I believe

Done.

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:57: const base::string16&
device_name) {
On 2015/09/01 21:51:23, newt wrote:
> fix param indentation

Done.

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.h (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.h:29: void
ReportDeviceSelected(JNIEnv* env, jobject obj, jstring device_id);
Works for me!

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.h:43: EventHandler
event_handler_;
Sure!

https://codereview.chromium.org/1315093002/diff/290001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1315093002/diff/290001/content/public/browser...
content/public/browser/bluetooth_chooser.h:67: void
ShowOverviewLink(content::WebContents* web_contents);
Shifted this over to BluetoothDispatcherHost. :)

[Ted C, 2015-09-02 17:27:46]

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

[Ted C, 2015-09-02 17:27:47]

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:199:
Activity activity = windowAndroid.getActivity().get();
On 2015/09/02 16:01:43, Finnur wrote:
> Yes, I thought about that. Can we expect it to be null under normal
> circumstances and if so, how?
> 
> My thinking was that we'd probably want to know when/if it occurs, rather than
> silently fail. But, if this occurs normally, e.g. at shutdown, or something,
> then we need to silently fail.

There are a couple cases where it can be null.

1.) WebView where it is not created using an activity context, but this is a
    chrome feature, so that should not apply.

2.) During shutdown.  The activity is dying and it has been garbage collected
    already by android.  It implies that you are holding onto a stale window
    ref, which more often happens on async messages being handled by the android
    looper.  Here you are driven off a webcontents, which should be deleted when
    an activity is destroyed, so this most likely would be a programming error.
    Granted if you have any messages that are being processed outside of the
    webcontents lifecycle then you start introducing chances for this to happen.

I suspect in your case you can either gracefully fail or assert it is not null.

[newt (away), 2015-09-02 17:48:42]

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:83:
int start = title.toString().indexOf(mOrigin);
On 2015/09/02 16:01:43, Finnur wrote:
> I'n not sure how to use getSpanStart to accomplish this. Let me take a step
back
> to make sure we're on the same page...
> 
> I'm starting with something like:
>    "http://foo wants to <link-span>pair with</link-span>:"
> ... and hoping to convert that into:
>    "<color-span>http</color-span>://foo wants to <link-span>pair
> with</link-span>:"
> 
> getSpanStart would work if I were looking for the 'pair with' link-span, but
I'm
> not. I'm trying to take a static text and converting it into a color-span (so
to
> speak), while avoiding splitting the string up into two, which will lead to
> translation headaches.

Ah, I thought the span was wrapped around the origin. I didn't realize it was
actually wrapped around "Pair with".

I still don't love using indexOf() because it's sensitive to the value of the
origin (which we don't really control) and the translation (which we also don't
really control). It's quite unlikely that the origin text would be contained
elsewhere inside the translation, but it's possible. (This would be a much more
realistic problem if we were just showing hostnames since a hostname like "b" or
"go" could easily appear in the translation.)

I'll think about some improvements to SpanApplier to make situations like this
more easily handled. For how, it's probably OK to leave as is.

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

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:171:
private boolean isAvailable(String id) {
On 2015/09/02 16:01:43, Finnur wrote:
> Ooh, I like that idea. Unfortunately, it makes it disabled but doesn't gray
> anything out, but that's easy to fix.

The view gets marked as "disabled". If the text color is a ColorStateList (e.g.
chrome/android/java/res/color/light_mode_tint.xml), then it will use the color
from the "disabled" state defined in the xml file. If the text color is just a
fixed color/integer (as in your case), then it doesn't change when its state is
"disabled".

So, you could use a ColorStateList, but I think the code you have is fine (and
simpler).

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/android...
File chrome/browser/android/chrome_web_contents_delegate_android.h (right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/browser/android...
chrome/browser/android/chrome_web_contents_delegate_android.h:45:
scoped_ptr<content::BluetoothChooser> RunBluetoothChooser(
On 2015/09/02 16:01:43, Finnur wrote:
> I struggled with this quite a bit, and believe it belongs here.
> 
> content/browser/bluetooth/bluetooth_dispatcher_host.cc calls
> WebContentsDelegate::RunBluetoothChooser, which is what I'm overriding here
(in
> chrome/browser/chrome_web_contents_delegate_android.h). This layer
> (chrome/browser) is needed because my override function needs to instantiate
> BluetoothChooserAndroid, which depends on chrome/browser.
> 
> If there's a better way, I'm willing to re-consider, but I bounced this off a
> couple of people already and this was what was settled on.

Ok, if you're overriding a method in web_contents_delegate.h, then this makes
sense. Thanks for explaining.

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

https://codereview.chromium.org/1315093002/diff/30014/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:331:
mItemSelectedCallback.onItemSelected("");
Seems like you'll end up calling onItemSelected() twice if the user clicks the
confirm button: once with the selected item's key and once with the empty
string. Is that what you want?

[Finnur, 2015-09-03 11:51:45]

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/1315093002/diff/290001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:199:
Activity activity = windowAndroid.getActivity().get();
Thanks for the explanation! I think I'll go with an assert for now and we can
change it to a null check if it becomes problematic.

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

https://codereview.chromium.org/1315093002/diff/30014/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:331:
mItemSelectedCallback.onItemSelected("");
Both handlers do, indeed, fire, but the second message never reaches the native
level. However, it isn't necessary to keep the onDismiss handler once the
onClick handler (for the commit button fires), so I now null it there just to
make sure.

[newt (away), 2015-09-03 17:04:26]

lgtm

[Jeffrey Yasskin, 2015-09-03 17:22:01]

C++ side lgtm.

Is there any way to write a test for the Java UI?

https://codereview.chromium.org/1315093002/diff/380001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/1315093002/diff/380001/content/public/browser...
content/public/browser/bluetooth_chooser.h:13: #include
"content/public/browser/web_contents.h"
I think you don't need this #include anymore.

[Finnur, 2015-09-04 09:17:02]

Removed the include. Will look at test today.

Thanks all for reviewing and good comments.

[Finnur, 2015-09-04 09:17:08]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2015-09-04 09:17:08]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
jyasskin@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1315093002/#ps400001
(title: "Remove redundant include")

[commit-bot: I haz the power, 2015-09-04 09:17:19]

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

[commit-bot: I haz the power, 2015-09-04 11:09:27]

Committed patchset #14 (id:400001)

[commit-bot: I haz the power, 2015-09-04 11:10:10]

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