WebUsb Android chooser UI

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

Index: chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java b/chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
new file mode 100644
index 0000000000000000000000000000000000000000..bf07c14704b532b7e8751641fc0a56a1ec329a19
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
@@ -0,0 +1,140 @@
+// 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.text.SpannableString;
+import android.text.TextUtils;
+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.NoUnderlineClickableSpan;
+import org.chromium.ui.text.SpanApplier;
+import org.chromium.ui.text.SpanApplier.SpanInfo;
+
+/**
+ * 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 {
+    // Values passed to nativeOnDialogFinished:eventType, and only used in the native function.
+    private static final int DIALOG_FINISHED_CANCELLED = 0;

[newt (away), 2016/03/15 19:03:53]

Do you anticipate having lots of ways of finishing the dialog, or just two? If
it's just two or three, then I'd remove these constants and simply have two
separate methods, e.g. nativeOnDialogCancelled() and nativeOnItemSelected().
That'll avoid the need for having shared constants between Java and C++ and
increases type safety a bit.

[juncai, 2016/03/16 00:40:35]

Done.

+    private static final int DIALOG_FINISHED_SELECTED = 1;
+
+    // The dialog to show to let the user pick a device.
+    ItemChooserDialog mItemChooserDialog;
+
+    // A pointer back to the native part of the implementation for this dialog.

[newt (away), 2016/03/15 19:03:53]

Nit: use javadoc style comments /** */ when documenting an individual variable,
method, or class, instead of //

[juncai, 2016/03/16 00:40:36]

Done.

+    long mNativeUsbChooserDialogPtr;
+
+    /**
+     * Creates the UsbChooserDialog and displays it (and starts waiting for data).

[newt (away), 2016/03/15 19:03:53]

"and displays it"? This doesn't seem accurate.

[juncai, 2016/03/16 00:40:36]

Done.

+     *
+     * @param context Context which is used for launching a dialog.
+     */
+    private UsbChooserDialog(long nativeUsbChooserDialogPtr) {
+        mNativeUsbChooserDialogPtr = nativeUsbChooserDialogPtr;
+    }
+
+    /**
+     * Show the UsbChooserDialog.

[newt (away), 2016/03/15 19:03:53]

s/Show/Shows/  In general, use descriptive verbs in javadoc, instead of
commands.

[juncai, 2016/03/16 00:40:35]

Done.

+     *
+     * @param windowAndroid The window that owns this dialog.
+     * @param origin The origin for the site wanting to connect to the USB device.
+     * @param securityLevel The security level of the connection to the site wanting to connect to
+     * the USB device. For valid values see SecurityStateModel::SecurityLevel.

[newt (away), 2016/03/15 19:03:53]

nit: indent this to line with up "For valid values..." above

[juncai, 2016/03/16 00:40:36]

Done.

+     */
+    private void show(WindowAndroid windowAndroid, String origin, int securityLevel) {

[newt (away), 2016/03/15 19:03:53]

Instead of taking a WindowAndroid here, I'd take a Context. The create() method
below should be responsible for getting the Context from the WindowAndroid. If
the Context is null, then create() should return null instead of returning a
valid UsbChooserDialog.

[juncai, 2016/03/16 00:40:36]

Done.

+        Context context = windowAndroid.getActivity().get();
+        assert context != null;
+        // Emphasize the origin.
+        Profile profile = Profile.getLastUsedProfile();
+        SpannableString originSpannableString = new SpannableString(origin);
+        OmniboxUrlEmphasizer.emphasizeUrl(originSpannableString, context.getResources(), profile,
+                securityLevel, false /* isInternalPage */, true /* useDarkColors */,
+                true /* emphasizeHttpsScheme */);
+        // Construct a full string and replace the origin text with emphasized version.
+        SpannableString title =
+                new SpannableString(context.getString(R.string.usb_chooser_dialog_prompt, origin));
+        int start = title.toString().indexOf(origin);
+        TextUtils.copySpansFrom(originSpannableString, 0, originSpannableString.length(),
+                Object.class, title, start);
+
+        SpannableString searching = new SpannableString(new String(""));

[newt (away), 2016/03/15 19:03:53]

s/new String("")/""/

no need for an object allocation here

Even better, I'd suggest changing the member variables in ItemChooserLabels from
SpannableStrings to CharSequences (which is a superclass of both String and
SpannableString). TextView.setText() takes a CharSequence, so this'll work fine,
and then you can avoid creating SpannableStrings, except where actually needed,
e.g.

    String searching = "";

[juncai, 2016/03/16 00:40:35]

Done.

+        SpannableString noneFound = new SpannableString(
+                context.getString(R.string.usb_chooser_dialog_no_devices_found_prompt));
+        SpannableString statusActive =
+                SpanApplier.applySpans(context.getString(R.string.usb_chooser_dialog_footnote_text),
+                        new SpanInfo("<link>", "</link>", new UsbNoUnderlineClickableSpan()));

[newt (away), 2016/03/15 19:03:53]

suggestion: just inline the definition of UsbNoUnderlineClickableSpan here.
Since UsbNoUnderlineClickableSpan is only used once, I think it's clearer to put
it here and reduce indirection.

[juncai, 2016/03/16 00:40:36]

Done.

+        SpannableString statusIdleNoneFound = statusActive;
+        SpannableString statusIdleSomeFound = statusActive;
+        String positiveButton = context.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(context, this, labels);
+        mItemChooserDialog.setIdleState();

[newt (away), 2016/03/15 19:03:53]

Why set the IDLE state, instead of keeping the default (STARTING) state?

[juncai, 2016/03/16 00:40:36]

This line is removed.
Added code to set idle state at: UsbChooserAndroid::GotUsbDeviceList.

Done.

+    }
+
+    @Override
+    public void onItemSelected(String id) {
+        if (mNativeUsbChooserDialogPtr != 0) {
+            if (id.isEmpty()) {
+                nativeOnDialogFinished(mNativeUsbChooserDialogPtr, DIALOG_FINISHED_CANCELLED, "");
+            } else {
+                nativeOnDialogFinished(mNativeUsbChooserDialogPtr, DIALOG_FINISHED_SELECTED, id);
+            }
+        }
+    }
+
+    private class UsbNoUnderlineClickableSpan extends NoUnderlineClickableSpan {
+        @Override
+        public void onClick(View view) {
+            if (mNativeUsbChooserDialogPtr == 0) {
+                return;
+            }
+
+            nativeShowUsbOverviewLink(mNativeUsbChooserDialogPtr);
+
+            super.onClick(view);
+        }
+    }
+
+    @CalledByNative
+    private static UsbChooserDialog create(WindowAndroid windowAndroid, String origin,
+            int securityLevel, long nativeUsbChooserDialogPtr) {
+        UsbChooserDialog dialog = new UsbChooserDialog(nativeUsbChooserDialogPtr);
+        dialog.show(windowAndroid, origin, securityLevel);
+        return dialog;
+    }
+
+    @CalledByNative
+    private void addDevice(String deviceId, String deviceName) {
+        mItemChooserDialog.addItemToList(
+                new ItemChooserDialog.ItemChooserRow(deviceId, deviceName));
+    }
+
+    @CalledByNative
+    private void removeDevice(String deviceId, String deviceName) {
+        mItemChooserDialog.removeItemFromList(
+                new ItemChooserDialog.ItemChooserRow(deviceId, deviceName));
+    }
+
+    @CalledByNative
+    private void closeDialog() {
+        mNativeUsbChooserDialogPtr = 0;
+        mItemChooserDialog.dismiss();
+    }
+
+    private native void nativeOnDialogFinished(
+            long nativeUsbChooserAndroid, int eventType, String deviceId);
+    // Help link.

[newt (away), 2016/03/15 19:03:53]

huh?

[juncai, 2016/03/16 00:40:36]

Updated comments.

Done.

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