WebUsb Android chooser UI

chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser;
+
+import android.content.Context;
+import android.graphics.Color;
+import android.text.SpannableString;
+import android.text.TextPaint;
+import android.text.TextUtils;
+import android.text.style.ClickableSpan;
+import android.view.View;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.omnibox.OmniboxUrlEmphasizer;
+import org.chromium.chrome.browser.profiles.Profile;
+import org.chromium.ui.base.WindowAndroid;
+import org.chromium.ui.text.SpanApplier;
+import org.chromium.ui.text.SpanApplier.SpanInfo;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * A dialog for showing available USB devices. This dialog is shown when a website requests to
+ * connect to a USB device (e.g. through a usb.requestDevice Javascript call).
+ */
+public class UsbChooserDialog
+        implements ItemChooserDialog.ItemSelectedCallback, WindowAndroid.PermissionCallback {
+    // Values passed to nativeOnDialogFinished:eventType, and only used in the native function.
+    private static final int DIALOG_FINISHED_CANCELLED = 0;
+    private static final int DIALOG_FINISHED_SELECTED = 1;
+
+    // The window that owns this dialog.
+    final WindowAndroid mWindowAndroid;
+
+    // Always equal to mWindowAndroid.getActivity().get(), but stored separately to make sure it's
+    // not GC'ed.

[Yaron, 2016/03/07 21:51:49]

This seems like the exact reason to *not* do this. It's only ever used in the
c-tor and show (which is called immediately after construction). Leaking a
Context can be very expensive and I don't see why this needs to hold onto it.
That's one of the reasons why WindowAndroid exists (which I'll note you're also
storing above and currently never using)

[juncai, 2016/03/09 01:26:50]

Done.

+    final Context mContext;
+
+    // The dialog to show to let the user pick a device.
+    ItemChooserDialog mItemChooserDialog;
+
+    // The origin for the site wanting to connect to the USB device.
+    String mOrigin;

[Yaron, 2016/03/07 21:51:49]

I don't see why mOrigin and mSecurityLevel are member variables. Just pass them
to |show|

[juncai, 2016/03/09 01:26:50]

Done.

+
+    // The security level of the connection to the site wanting to connect to the
+    // USB device. For valid values see SecurityStateModel::SecurityLevel.
+    int mSecurityLevel;
+
+    // A pointer back to the native part of the implementation for this dialog.
+    long mNativeUsbChooserDialogPtr;
+
+    /**
+     * Creates the UsbChooserDialog and displays it (and starts waiting for data).
+     *
+     * @param context Context which is used for launching a dialog.
+     */
+    private UsbChooserDialog(WindowAndroid windowAndroid, String origin, int securityLevel,
+            long nativeUsbChooserDialogPtr) {
+        mWindowAndroid = windowAndroid;
+        mContext = windowAndroid.getActivity().get();
+        assert mContext != null;
+        mOrigin = origin;
+        mSecurityLevel = securityLevel;
+        mNativeUsbChooserDialogPtr = nativeUsbChooserDialogPtr;
+    }
+
+    /**
+     * Show the UsbChooserDialog.
+     */
+    private void show() {
+        // Emphasize the origin.
+        Profile profile = Profile.getLastUsedProfile();
+        SpannableString origin = new SpannableString(mOrigin);
+        OmniboxUrlEmphasizer.emphasizeUrl(origin, mContext.getResources(), profile, mSecurityLevel,
+                false /* isInternalPage */, true /* useDarkColors */,
+                true /* emphasizeHttpsScheme */);
+        // Construct a full string and replace the origin text with emphasized version.
+        SpannableString title = new SpannableString(
+                mContext.getString(R.string.usb_chooser_dialog_prompt, mOrigin));
+        int start = title.toString().indexOf(mOrigin);
+        TextUtils.copySpansFrom(origin, 0, origin.length(), Object.class, title, start);
+
+        SpannableString searching = new SpannableString(new String(""));
+        SpannableString noneFound = new SpannableString(
+                mContext.getString(R.string.usb_chooser_dialog_no_devices_found_prompt));
+        SpannableString statusActive = SpanApplier.applySpans(
+                mContext.getString(R.string.usb_chooser_dialog_footnote_text),
+                new SpanInfo("<link>", "</link>", new NoUnderlineClickableSpan()));
+        SpannableString statusIdleNoneFound = statusActive;
+        SpannableString statusIdleSomeFound = statusActive;
+        String positiveButton = mContext.getString(R.string.usb_chooser_dialog_connect_button_text);
+
+        ItemChooserDialog.ItemChooserLabels labels =
+                new ItemChooserDialog.ItemChooserLabels(title, searching, noneFound, statusActive,
+                        statusIdleNoneFound, statusIdleSomeFound, positiveButton);
+        mItemChooserDialog = new ItemChooserDialog(mContext, this, labels);
+    }
+
+    @Override
+    public void onItemSelected(String id) {
+        if (mNativeUsbChooserDialogPtr != 0) {
+            if (id.isEmpty()) {
+                nativeOnDialogFinished(mNativeUsbChooserDialogPtr, DIALOG_FINISHED_CANCELLED, "");
+            } else {
+                nativeOnDialogFinished(mNativeUsbChooserDialogPtr, DIALOG_FINISHED_SELECTED, id);
+            }
+        }
+    }
+
+    @Override
+    public void onRequestPermissionsResult(String[] permissions, int[] grantResults) {}

[Yaron, 2016/03/07 21:51:49]

Remove this and the the corresponding interface declaration. it's never used

[Finnur, 2016/03/08 10:26:20]

Is there no special permission that is needed for USB, like with Bluetooth?

[juncai, 2016/03/09 01:26:50]

Done.

[juncai, 2016/03/09 01:26:50]

I think for Bluetooth on Android, Chromium needs the Location permission to scan
for Bluetooth devices. For USB, no scanning needed, so I think no special
permission needed.

[Finnur, 2016/03/09 13:47:02]

Then yeah, removing this is the right thing to do.

+
+    /**
+     * A helper class to show a clickable link with underlines turned off.

[Yaron, 2016/03/07 21:51:49]

I'm not a UI-guy (+newt - should probably look at this CL more generally) but
does it really make sense to have this one-off UI styling?

Oh! Now I see this is mostly copied from BluetoothChooserDialog. I haven't
looked too deeply but is there opportunity for re-use?

[Finnur, 2016/03/08 10:26:20]

It is not a huge amount of savings, but we should still re-use. It will at least
get rid of the copying of updateDrawState, and hopefully we can move the
view.invalidate() call into the base class also.

[juncai, 2016/03/09 01:26:50]

Done.

[juncai, 2016/03/09 01:26:50]

Added a class NoUnderlineClickableSpan that both Bluetooth and USB code can use.

+     */
+    private class NoUnderlineClickableSpan extends ClickableSpan {
+        NoUnderlineClickableSpan() {}
+
+        @Override
+        public void onClick(View view) {
+            if (mNativeUsbChooserDialogPtr == 0) {
+                return;
+            }
+
+            nativeShowUsbOverviewLink(mNativeUsbChooserDialogPtr);
+
+            // Get rid of the highlight background on selection.
+            view.invalidate();
+        }
+
+        @Override
+        public void updateDrawState(TextPaint textPaint) {
+            super.updateDrawState(textPaint);
+            textPaint.bgColor = Color.TRANSPARENT;
+            textPaint.setUnderlineText(false);
+        }
+    }
+
+    @CalledByNative
+    private static UsbChooserDialog create(WindowAndroid windowAndroid, String origin,
+            int securityLevel, long nativeUsbChooserDialogPtr) {
+        UsbChooserDialog dialog = new UsbChooserDialog(
+                windowAndroid, origin, securityLevel, nativeUsbChooserDialogPtr);
+        dialog.show();
+        return dialog;
+    }
+
+    @CalledByNative
+    private void updateOptions(String[] deviceId, String[] deviceName) {

[Finnur, 2016/03/08 10:26:20]

Nit: seems to me like addDevices is a better description. No?

Also I think the params should be plural (deviceIds and deviceNames).

[juncai, 2016/03/09 01:26:50]

Done.

+        if (deviceName == null) return;

[Yaron, 2016/03/07 21:51:49]

Do this on the native side

[juncai, 2016/03/09 01:26:50]

Done.

+        mItemChooserDialog.clear();

[Finnur, 2016/03/08 10:26:20]

Are you guaranteeing that you won't receive more calls to add devices? Bluetooth
does this piecemeal as devices are discovered.

[juncai, 2016/03/09 01:26:50]

Receiving more calls to add devices can be handled here, since the parameters
that this function receives are the current device list, and this device list is
updated by UsbChooserAndroid::OnDeviceAdded and
UsbChooserAndroid::OnDeviceRemoved.

[Finnur, 2016/03/09 13:47:02]

Hmmm... Let me rephrase the question. 

Bluetooth works this way (last I checked):
1. Scanning starts, dialog shown.
2. Devices a, b, and c are added via AddDevice and shown in the dialog.
3. A few more devices (d, e, f) are added via another AddDevice call. Shown at
the bottom.
4. Scanning stops, dialog now shows a,b,c,d,e,f.

If I'm reading you correctly, the same could happen for USB. However, since you
call clear(), a, b, and c would be deleted when d, e, and f are added.

[juncai, 2016/03/10 01:22:22]

ah, I see, the USB and Bluetooth dialog implementation is a little different
here.

For Bluetooth, when a new device is found, it calls addDevice with the device's
id and name to add it to the chooser:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
And when removing a device, it calls:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
to disable the device in the chooser, it seems that its name is still in the
chooser, but with gray color.

For USB, since the device connection/disconnection is more physical, when a
device is plugged out, removing its name from the chooser is better than
disabling it in the chooser.

The ItemChooserDialog has addItemsToList:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
But there is no function to remove a certain item from the list. So for USB, I
just store all the device names and ids at UsbChooserAndroid class, and when a
device is added/removed, the device names and ids are updated and then are
passed to the UsbChooserDialog's addDevices function which take an array of
device names and ids as parameter. So it needs to clear all items previously
shown in the chooser, then show all the currently connected devices' names.

[Finnur, 2016/03/10 12:12:56]

I see. 

Yeah, the Bluetooth devices could drop in and out of range and we didn't want
the list to be fluctuating too much because of that.

However, re-creating the list from scratch is not ideal and while it is true
that there's no Remove function in the adapter, that's simply because we haven't
had a need for one yet. But I think it would be simple and worthwhile to add,
though, and for you to start using. Not only would that potentially benefit the
next person who writes a picker, but also alleviate the need for you to keep
track of the devices in your usb_chooser_android.cc, which in turn leads to code
simplifications there.

[juncai, 2016/03/11 23:32:33]

Thanks for the comments. I added addItemToList and removeItemFromList to
ItemChooserDialog that are used by UsbChooserDialog.

Done.

+        List<ItemChooserDialog.ItemChooserRow> devices =

[Yaron, 2016/03/07 21:51:49]

move allocation after early-return

[juncai, 2016/03/09 01:26:50]

Done.

+                new ArrayList<ItemChooserDialog.ItemChooserRow>();
+        int len = deviceId.length;
+        if (len == 0) {
+            mItemChooserDialog.setIdleState();
+            return;
+        }
+
+        for (int i = 0; i < len; ++i) {
+            devices.add(new ItemChooserDialog.ItemChooserRow(deviceId[i], deviceName[i]));
+        }
+        mItemChooserDialog.addItemsToList(devices);
+    }
+
+    @CalledByNative
+    private void closeDialog() {
+        mNativeUsbChooserDialogPtr = 0;
+        mItemChooserDialog.dismiss();
+    }
+
+    private native void nativeOnDialogFinished(
+            long nativeUsbChooserAndroid, int eventType, String deviceId);
+    // Help links.

[Finnur, 2016/03/08 10:26:20]

nit: Singular (link).

[juncai, 2016/03/09 01:26:50]

Done.

+    private native void nativeShowUsbOverviewLink(long nativeUsbChooserAndroid);
+}