WebUsb Android chooser UI

WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=591735
Committed: https://crrev.com/4cbad6ed5522e60cda2aa728c08e3aa80545e360
Cr-Commit-Position: refs/heads/master@{#383351}

[juncai, 2016-02-25 23:28:33]

Description was changed from

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

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

to

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=492204
==========

[juncai, 2016-03-02 18:14:32]

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

[juncai, 2016-03-02 18:14:33]

Please take a look.

[Reilly Grant (use Gerrit), 2016-03-02 18:54:57]

Can you rebase this change on top of
https://codereview.chromium.org/1742753002/?

[juncai, 2016-03-02 19:18:18]

Sure, I'll set this patch depend on
https://codereview.chromium.org/1742753002/

[juncai, 2016-03-02 21:23:45]

Rebase done, please take a look.

[Reilly Grant (use Gerrit), 2016-03-03 01:42:20]

Can you file a bug for the Android UI and include screenshots of the dialog from
this patch?

[Reilly Grant (use Gerrit), 2016-03-03 01:57:56]

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:82: }  // namespace
This is copy-pasted from usb_chooser_bubble_controller.cc. Please find a place
for this common code.

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:88: :
render_frame_host_(render_frame_host),
Like the desktop version we should be careful to tear down the chooser if the
RFH is destroyed.

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.h (right):

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.h:31: // Represents a way to ask
the user to select a Usb device from a list of
s/Usb/USB/

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_chooser_service.h (right):

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_chooser_service.h:52: #endif  // defined(OS_ANDROID)
It seems like there should just be an Android version of this header.

[juncai, 2016-03-03 18:36:41]

Description was changed from

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=492204
==========

to

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=492204, 591735
==========

[juncai, 2016-03-04 01:36:39]

bug created and screenshots uploaded.

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:82: }  // namespace
On 2016/03/03 01:57:56, Reilly Grant wrote:
> This is copy-pasted from usb_chooser_bubble_controller.cc. Please find a place
> for this common code.

Done.

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:88: :
render_frame_host_(render_frame_host),
On 2016/03/03 01:57:55, Reilly Grant wrote:
> Like the desktop version we should be careful to tear down the chooser if the
> RFH is destroyed.

Done.

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.h (right):

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.h:31: // Represents a way to ask
the user to select a Usb device from a list of
On 2016/03/03 01:57:56, Reilly Grant wrote:
> s/Usb/USB/

Done.

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_chooser_service.h (right):

https://codereview.chromium.org/1739523002/diff/160001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_chooser_service.h:52: #endif  // defined(OS_ANDROID)
On 2016/03/03 01:57:56, Reilly Grant wrote:
> It seems like there should just be an Android version of this header.

Done.

[Reilly Grant (use Gerrit), 2016-03-04 20:53:39]

Since you have a bug tracking this feature development you can drop issue 492204
from the description to avoid spamming that single bug.

Please get someone from Clank to review the .java sources.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.cc (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.cc:24:
usb_chooser_android_.reset(new UsbChooserAndroid(
Never mind. This is already explicitly owned here so being a WebContentsObserver
is unnecessary. However, is only supporting a single call to GetPermission at
once what we want?

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.h (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.h:22: class
UsbDevice;
This forward declaration is unused.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:100: for (auto it =
devices_.begin(); it != devices_.end(); ++it) {
auto it = devices_.find(devices_begin(), devices_.end(), device);

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:103: size_t index = it -
devices_.begin();
devices_.erase(it) invalidates it so compute the index first.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:118: for (size_t index = 0;
index < device_guids_.size(); ++index) {
Similarly, use device_guids_.find() here.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.h (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.h:35: public
content::WebContentsObserver {
I've been told that WebContentsObservers are expensive. Instead have this object
explicitly owned by the WebUsbChooserServiceAndroid which is in turn explicitly
owned by the UsbTabHelper which will destroy it if the frame navigates.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_common.h (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
chrome/browser/usb/usb_common.h:36: bool FindInAllowedOrigins(const
device::WebUsbAllowedOrigins* allowed_origins,
These functions should have more specific names now that they are no longer
private. I suggest RecordWebUsbChooserClosure and FindInWebUsbAllowedOrigins.
The latter may also be added to device::WebUsbAllowedOrigins. Then this file
could simply be web_usb_histograms.h.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:30: #endif  // defined(OS_ANDROID)
You can avoid these ifdefs by making this a
scoped_ptr<device::usb::ChooserService>.

[juncai, 2016-03-04 21:04:14]

Description was changed from

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=492204, 591735
==========

to

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=591735
==========

[juncai, 2016-03-05 00:18:23]

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

[juncai, 2016-03-05 00:18:25]

yfriedman@chromium.org: Please review changes in 
//chrome/android
//chrome/browser/android
//chrome/browser/ui/android

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.cc (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.cc:24:
usb_chooser_android_.reset(new UsbChooserAndroid(
On 2016/03/04 20:53:39, Reilly Grant wrote:
> Never mind. This is already explicitly owned here so being a
WebContentsObserver
> is unnecessary. However, is only supporting a single call to GetPermission at
> once what we want?

Now WebUsbChooserServiceAndroid has a std::vector<scoped_ptr<UsbChooserAndroid>>
instead of scoped_ptr<UsbChooserAndroid> to allow multiple calls to
GetPermission.

Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.h (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.h:22: class
UsbDevice;
On 2016/03/04 20:53:39, Reilly Grant wrote:
> This forward declaration is unused.

Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:100: for (auto it =
devices_.begin(); it != devices_.end(); ++it) {
On 2016/03/04 20:53:39, Reilly Grant wrote:
> auto it = devices_.find(devices_begin(), devices_.end(), device);

Use std::find() here since std::vector doesn't have find member function.
Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:103: size_t index = it -
devices_.begin();
On 2016/03/04 20:53:39, Reilly Grant wrote:
> devices_.erase(it) invalidates it so compute the index first.

Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:118: for (size_t index = 0;
index < device_guids_.size(); ++index) {
On 2016/03/04 20:53:39, Reilly Grant wrote:
> Similarly, use device_guids_.find() here.

Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.h (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.h:35: public
content::WebContentsObserver {
On 2016/03/04 20:53:39, Reilly Grant wrote:
> I've been told that WebContentsObservers are expensive. Instead have this
object
> explicitly owned by the WebUsbChooserServiceAndroid which is in turn
explicitly
> owned by the UsbTabHelper which will destroy it if the frame navigates.

Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_common.h (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
chrome/browser/usb/usb_common.h:36: bool FindInAllowedOrigins(const
device::WebUsbAllowedOrigins* allowed_origins,
On 2016/03/04 20:53:39, Reilly Grant wrote:
> These functions should have more specific names now that they are no longer
> private. I suggest RecordWebUsbChooserClosure and FindInWebUsbAllowedOrigins.
> The latter may also be added to device::WebUsbAllowedOrigins. Then this file
> could simply be web_usb_histograms.h.

Done.

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1739523002/diff/220001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:30: #endif  // defined(OS_ANDROID)
On 2016/03/04 20:53:39, Reilly Grant wrote:
> You can avoid these ifdefs by making this a
> scoped_ptr<device::usb::ChooserService>.

If making this a scoped_ptr<device::usb::ChooserService>, then line:
frame_usb_services->chooser_service->Bind(std::move(request));
need pointer cast.

[Yaron, 2016-03-07 21:51:48]

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

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

Here's a first pass. In general, it looks like it could use some more
consideration around lifetime of the objects being used.

Please look into adding tests as well

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:40: //
not GC'ed.
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)

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:47:
String mOrigin;
I don't see why mOrigin and mSecurityLevel are member variables. Just pass them
to |show|

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:115:
public void onRequestPermissionsResult(String[] permissions, int[] grantResults)
{}
Remove this and the the corresponding interface declaration. it's never used

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:118: *
A helper class to show a clickable link with underlines turned off.
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?

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:154:
if (deviceName == null) return;
Do this on the native side

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:156:
List<ItemChooserDialog.ItemChooserRow> devices =
move allocation after early-return

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.cc (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.cc:24:
usb_chooser_android_.push_back(make_scoped_ptr(new UsbChooserAndroid(
I see these instances are never cleared so it relies on
WebUsbChooserServiceAndroid's lifecycle. That appears to be lazily init'd but
then held on to for some amount of time that I haven't sorted out. I'm worried
about leaks esp since this is holding some objects that would be bad to leak.

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.h (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.h:42:
std::vector<scoped_ptr<UsbChooserAndroid>> usb_chooser_android_;
Does it really make sense to have multiple of these? How would that happen? It
looks like just creating the chooser shows it so what would the UI look like?

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:41: web_contents_(
Don't think it's worth holding on to this given you're already caching
RenderFrameHost

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:65:
base::android::ScopedJavaLocalRef<jstring> origin_string =
I believe this should use url_formatter::FormatUrlForSecurityDisplay

[newt (away), 2016-03-08 04:19:51]

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

[newt (away), 2016-03-08 04:19:52]

finnur: Could you take a look at the use of ItemChooserDialog here?

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

Not a problem.

Overall it looks fine (just minor issues) but I'd like to see dialog screenshots
of its various stages (once it opens, once it has data, etc).

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:115:
public void onRequestPermissionsResult(String[] permissions, int[] grantResults)
{}
Is there no special permission that is needed for USB, like with Bluetooth?

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:118: *
A helper class to show a clickable link with underlines turned off.
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.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:153:
private void updateOptions(String[] deviceId, String[] deviceName) {
Nit: seems to me like addDevices is a better description. No?

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:155:
mItemChooserDialog.clear();
Are you guaranteeing that you won't receive more calls to add devices? Bluetooth
does this piecemeal as devices are discovered.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:178:
// Help links.
nit: Singular (link).

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2392: <message
name="IDS_USB_CHOOSER_DIALOG_PROMPT" desc="The text that is used to introduce
chooser dialog details to the user.">
s/chooser/the USB chooser/

... and drop the word 'details'.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2395: <message
name="IDS_USB_CHOOSER_DIALOG_NO_DEVICES_FOUND_PROMPT" desc="The text that is
used to inform user that no devices found in the chooser dialog.">
s/chooser/USB/
(same below)

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:65:
base::android::ScopedJavaLocalRef<jstring> origin_string =
Then BluetoothChooserAndroid ctor needs a corresponding change.

[juncai, 2016-03-09 01:26:51]

For the tests, the dialog screenshots of various stages (no device, devices
found, device selected) were uploaded at bug link:
https://bugs.chromium.org/p/chromium/issues/detail?id=591735

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:40: //
not GC'ed.
On 2016/03/07 21:51:49, Yaron wrote:
> 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)

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:47:
String mOrigin;
On 2016/03/07 21:51:49, Yaron wrote:
> I don't see why mOrigin and mSecurityLevel are member variables. Just pass
them
> to |show|

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:115:
public void onRequestPermissionsResult(String[] permissions, int[] grantResults)
{}
On 2016/03/07 21:51:49, Yaron wrote:
> Remove this and the the corresponding interface declaration. it's never used

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:115:
public void onRequestPermissionsResult(String[] permissions, int[] grantResults)
{}
On 2016/03/08 10:26:20, Finnur wrote:
> Is there no special permission that is needed for USB, like with Bluetooth?

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.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:118: *
A helper class to show a clickable link with underlines turned off.
On 2016/03/07 21:51:49, Yaron wrote:
> 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?

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:118: *
A helper class to show a clickable link with underlines turned off.
On 2016/03/08 10:26:20, Finnur wrote:
> 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.

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:153:
private void updateOptions(String[] deviceId, String[] deviceName) {
On 2016/03/08 10:26:20, Finnur wrote:
> Nit: seems to me like addDevices is a better description. No?
> 
> Also I think the params should be plural (deviceIds and deviceNames).

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:154:
if (deviceName == null) return;
On 2016/03/07 21:51:49, Yaron wrote:
> Do this on the native side

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:155:
mItemChooserDialog.clear();
On 2016/03/08 10:26:20, Finnur wrote:
> Are you guaranteeing that you won't receive more calls to add devices?
Bluetooth
> does this piecemeal as devices are discovered.

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.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:156:
List<ItemChooserDialog.ItemChooserRow> devices =
On 2016/03/07 21:51:49, Yaron wrote:
> move allocation after early-return

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:178:
// Help links.
On 2016/03/08 10:26:20, Finnur wrote:
> nit: Singular (link).

Done.

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2392: <message
name="IDS_USB_CHOOSER_DIALOG_PROMPT" desc="The text that is used to introduce
chooser dialog details to the user.">
On 2016/03/08 10:26:20, Finnur wrote:
> s/chooser/the USB chooser/
> 
> ... and drop the word 'details'.

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2395: <message
name="IDS_USB_CHOOSER_DIALOG_NO_DEVICES_FOUND_PROMPT" desc="The text that is
used to inform user that no devices found in the chooser dialog.">
On 2016/03/08 10:26:20, Finnur wrote:
> s/chooser/USB/
> (same below)

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.cc (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.cc:24:
usb_chooser_android_.push_back(make_scoped_ptr(new UsbChooserAndroid(
On 2016/03/07 21:51:49, Yaron wrote:
> I see these instances are never cleared so it relies on
> WebUsbChooserServiceAndroid's lifecycle. That appears to be lazily init'd but
> then held on to for some amount of time that I haven't sorted out. I'm worried
> about leaks esp since this is holding some objects that would be bad to leak.

The WebUsbChooserServiceAndroid object is owned by UsbTabHelper which is the
per-tab owner of USB services provided to render frames within that tab.
UsbTabHelper overrides content::WebContentsObserver::RenderFrameDeleted which
will make sure that WebUsbChooserServiceAndroid is released once a RenderFrame
for |render_frame_host| is deleted.

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
File chrome/browser/android/usb/web_usb_chooser_service_android.h (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/android...
chrome/browser/android/usb/web_usb_chooser_service_android.h:42:
std::vector<scoped_ptr<UsbChooserAndroid>> usb_chooser_android_;
On 2016/03/07 21:51:49, Yaron wrote:
> Does it really make sense to have multiple of these? How would that happen? It
> looks like just creating the chooser shows it so what would the UI look like?

By having multiple of these, it can support multiple calls to GetPermission at
once. For example, a website can call GetPermission multiple times by passing
different device filters in order to connect to different kinds of devices. So
in this case, the chooser UI dialogs will overlap, and one will show up after
the one above it is closed.

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:41: web_contents_(
On 2016/03/07 21:51:49, Yaron wrote:
> Don't think it's worth holding on to this given you're already caching
> RenderFrameHost

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:65:
base::android::ScopedJavaLocalRef<jstring> origin_string =
On 2016/03/07 21:51:49, Yaron wrote:
> I believe this should use url_formatter::FormatUrlForSecurityDisplay

Done.

https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:65:
base::android::ScopedJavaLocalRef<jstring> origin_string =
On 2016/03/08 10:26:20, Finnur wrote:
> Then BluetoothChooserAndroid ctor needs a corresponding change.

A patch was created to fix it at BluetoothChooserAndroid ctor:
https://codereview.chromium.org/1771353004/

[Finnur, 2016-03-09 13:47:03]

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:115:
public void onRequestPermissionsResult(String[] permissions, int[] grantResults)
{}
Then yeah, removing this is the right thing to do.

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:155:
mItemChooserDialog.clear();
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.

https://codereview.chromium.org/1739523002/diff/320001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/320001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:219: false /*
is_renderer_initiated */));
Nit: Multi-line comments for params are annoying. The look cluttered and make it
painful to comment out code.

Use:

    false));  // is_renderer_initiated.

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:155:
mItemChooserDialog.clear();
On 2016/03/09 13:47:02, Finnur wrote:
> 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.

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.

https://codereview.chromium.org/1739523002/diff/320001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.cc (right):

https://codereview.chromium.org/1739523002/diff/320001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.cc:219: false /*
is_renderer_initiated */));
On 2016/03/09 13:47:02, Finnur wrote:
> Nit: Multi-line comments for params are annoying. The look cluttered and make
it
> painful to comment out code.
> 
> Use:
> 
>     false));  // is_renderer_initiated.

Done.

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:155:
mItemChooserDialog.clear();
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.

[Yaron, 2016-03-11 19:54:47]

I think this is close but should really have newt's approval

https://codereview.chromium.org/1739523002/diff/360001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:34:
final WindowAndroid mWindowAndroid;
This can also just be passed to |show|

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

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:155:
mItemChooserDialog.clear();
On 2016/03/10 12:12:56, Finnur wrote:
> 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.

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

Done.

https://codereview.chromium.org/1739523002/diff/360001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/360001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:34:
final WindowAndroid mWindowAndroid;
On 2016/03/11 19:54:47, Yaron wrote:
> This can also just be passed to |show|

Done.

[Finnur, 2016-03-14 11:36:50]

https://codereview.chromium.org/1739523002/diff/320001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:34:
final WindowAndroid mWindowAndroid;
Since this is a derivative of the Bluetooth chooser, can you make the same
changes there, if possible?

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:368:
}
Hmm... I just discovered that the only (previous) consumer of this class
(Bluetooth) no longer uses this function in the manner it was intended (add a
whole list). This function should therefore be removed because Bluetooth now
adds items one by one, just like the UsbChooser does and having two ways of
doing the same thing is confusing. 

That means this function should be converted to use your new addItemToList:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

And that also makes me wonder if status PROGRESS_UPDATE_AVAILABLE is ever used.
If not, it should also be removed.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:387:
mProgressBar.setVisibility(View.GONE);
Why is this necessary?

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:389:
setState(State.DISCOVERY_IDLE);
What does the dialog look like if you add one item and subsequently remove it?
Does it show "no items", or something?

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:390:
}
These new API functions need to be tested in ItemChooserDialogTest.java.

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2409: <message
name="IDS_USB_CHOOSER_DIALOG_NO_DEVICES_FOUND_PROMPT" desc="The text that is
used to inform user that no devices found in the USB chooser dialog.">
nit: s/user/the user/

[newt (away), 2016-03-15 19:03:54]

I looked at the Java files and strings. Comments below.

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:111:
LinkType.RESTART_SEARCH, mContext)));
fix indentation here and below

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:201:
private class BluetoothNoUnderlineClickableSpan extends NoUnderlineClickableSpan
{
For brevity, I'd just call this BluetoothClickableSpan. The fact that it
disables underlines is not crucial to include in the name.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:27:
private static final int DIALOG_FINISHED_CANCELLED = 0;
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.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:33: //
A pointer back to the native part of the implementation for this dialog.
Nit: use javadoc style comments /** */ when documenting an individual variable,
method, or class, instead of //

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:37: *
Creates the UsbChooserDialog and displays it (and starts waiting for data).
"and displays it"? This doesn't seem accurate.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:46: *
Show the UsbChooserDialog.
s/Show/Shows/  In general, use descriptive verbs in javadoc, instead of
commands.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:51: *
the USB device. For valid values see SecurityStateModel::SecurityLevel.
nit: indent this to line with up "For valid values..." above

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:53:
private void show(WindowAndroid windowAndroid, String origin, int securityLevel)
{
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.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:69:
SpannableString searching = new SpannableString(new String(""));
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 = "";

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:74:
new SpanInfo("<link>", "</link>", new UsbNoUnderlineClickableSpan()));
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.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:83:
mItemChooserDialog.setIdleState();
Why set the IDLE state, instead of keeping the default (STARTING) state?

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:138:
// Help link.
huh?

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2405: <!-- USB Chooser
Dialog Prompt -->
nit: there's probably no need for the individual comments above each string. The
comments just repeat the string ID; the string ID and description should be
enough to identify individual strings.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2407: <ph
name="SITE">%1$s<ex>https://www.google.com</ex></ph> wants to connect
to:</message>
put </message> on the next line, like the other strings in this file.

https://codereview.chromium.org/1739523002/diff/380001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.h (right):

https://codereview.chromium.org/1739523002/diff/380001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.h:33: class UsbChooserAndroid :
public device::UsbService::Observer {
For consistency with the java side of this class, it'd make sense to name this
"UsbChooserDialog" or "UsbChooserDialogAndroid".

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:19:
view.invalidate();
Why is this needed? Are you copying this from somewhere?

Ah, looks like this came from BluetoothChooserDialog.NoUnderlineClickableSpan.
But it's not used in any of the other ClickableSpan subclasses in Chrome. Do we
really it? Maybe ask Finnur, who added it.

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:26:
textPaint.bgColor = Color.TRANSPARENT;
Also, why set the background color to transparent?

[Finnur, 2016-03-15 21:11:26]

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:19:
view.invalidate();
My ears are burning... ;)

It was definitely useful when I wrote it, because, as I recall, at that time
when a link was clicked in a dialog -- a link that *didn't* result in the dialog
closing -- you'd get this background highlight that would just stick. As if the
intent was to select the URL. Invalidating the view would cause it to redraw and
go away. 

If this no longer happens, then perhaps this class isn't needed, I guess. But it
should be easy to test, though, just use a regular span and click a Refresh link
and see if it grows a background.

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:26:
textPaint.bgColor = Color.TRANSPARENT;
Might be related to the above, not sure. Don't remember. :)

Maybe a background color needed to be set for the whole thing to work. Or maybe
I'm just that much of a rebel to set a transparent background needlessly. I can
be crazy like that.

[newt (away), 2016-03-15 21:24:45]

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:19:
view.invalidate();
On 2016/03/15 21:11:26, Finnur wrote:
> My ears are burning... ;)
> 
> It was definitely useful when I wrote it, because, as I recall, at that time
> when a link was clicked in a dialog -- a link that *didn't* result in the
dialog
> closing -- you'd get this background highlight that would just stick. As if
the
> intent was to select the URL. Invalidating the view would cause it to redraw
and
> go away. 
> 
> If this no longer happens, then perhaps this class isn't needed, I guess. But
it
> should be easy to test, though, just use a regular span and click a Refresh
link
> and see if it grows a background.

Ahhhh, thanks for explaining. So this *might* be needed in the item chooser
dialogs, but definitely isn't needed in other uses of NoUnderlineClickableSpan.

In that case, Jun, could you check whether this is still needed in the item
chooser dialog case, and if so, move the code into the choose dialog(s).
Otherwise, delete this altogether. Thanks!

(Likewise for bgColor = TRANSPARENT below)

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

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:111:
LinkType.RESTART_SEARCH, mContext)));
On 2016/03/15 19:03:53, newt wrote:
> fix indentation here and below

I think in browser it is displayed like this but is not like this in the actual
source file. I used: "git cl format" to fix the style.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:201:
private class BluetoothNoUnderlineClickableSpan extends NoUnderlineClickableSpan
{
On 2016/03/15 19:03:53, newt wrote:
> For brevity, I'd just call this BluetoothClickableSpan. The fact that it
> disables underlines is not crucial to include in the name.

Done.

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:368:
}
On 2016/03/14 11:36:50, Finnur wrote:
> Hmm... I just discovered that the only (previous) consumer of this class
> (Bluetooth) no longer uses this function in the manner it was intended (add a
> whole list). This function should therefore be removed because Bluetooth now
> adds items one by one, just like the UsbChooser does and having two ways of
> doing the same thing is confusing. 
> 
> That means this function should be converted to use your new addItemToList:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> And that also makes me wonder if status PROGRESS_UPDATE_AVAILABLE is ever
used.
> If not, it should also be removed.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:387:
mProgressBar.setVisibility(View.GONE);
On 2016/03/14 11:36:50, Finnur wrote:
> Why is this necessary? 

Removed.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:389:
setState(State.DISCOVERY_IDLE);
On 2016/03/14 11:36:50, Finnur wrote:
> What does the dialog look like if you add one item and subsequently remove it?
> Does it show "no items", or something?

It shows "No devices found.".

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:390:
}
On 2016/03/14 11:36:49, Finnur wrote:
> These new API functions need to be tested in ItemChooserDialogTest.java.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:27:
private static final int DIALOG_FINISHED_CANCELLED = 0;
On 2016/03/15 19:03:53, newt wrote:
> 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.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:33: //
A pointer back to the native part of the implementation for this dialog.
On 2016/03/15 19:03:53, newt wrote:
> Nit: use javadoc style comments /** */ when documenting an individual
variable,
> method, or class, instead of //

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:37: *
Creates the UsbChooserDialog and displays it (and starts waiting for data).
On 2016/03/15 19:03:53, newt wrote:
> "and displays it"? This doesn't seem accurate.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:46: *
Show the UsbChooserDialog.
On 2016/03/15 19:03:53, newt wrote:
> s/Show/Shows/  In general, use descriptive verbs in javadoc, instead of
> commands.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:51: *
the USB device. For valid values see SecurityStateModel::SecurityLevel.
On 2016/03/15 19:03:53, newt wrote:
> nit: indent this to line with up "For valid values..." above

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:53:
private void show(WindowAndroid windowAndroid, String origin, int securityLevel)
{
On 2016/03/15 19:03:53, newt wrote:
> 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.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:69:
SpannableString searching = new SpannableString(new String(""));
On 2016/03/15 19:03:53, newt wrote:
> 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 = "";

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:74:
new SpanInfo("<link>", "</link>", new UsbNoUnderlineClickableSpan()));
On 2016/03/15 19:03:53, newt wrote:
> 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.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:83:
mItemChooserDialog.setIdleState();
On 2016/03/15 19:03:53, newt wrote:
> Why set the IDLE state, instead of keeping the default (STARTING) state?

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

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:138:
// Help link.
On 2016/03/15 19:03:53, newt wrote:
> huh?

Updated comments.

Done.

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2405: <!-- USB Chooser
Dialog Prompt -->
On 2016/03/15 19:03:53, newt wrote:
> nit: there's probably no need for the individual comments above each string.
The
> comments just repeat the string ID; the string ID and description should be
> enough to identify individual strings.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2407: <ph
name="SITE">%1$s<ex>https://www.google.com</ex></ph> wants to connect
to:</message>
On 2016/03/15 19:03:53, newt wrote:
> put </message> on the next line, like the other strings in this file.

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2409: <message
name="IDS_USB_CHOOSER_DIALOG_NO_DEVICES_FOUND_PROMPT" desc="The text that is
used to inform user that no devices found in the USB chooser dialog.">
On 2016/03/14 11:36:50, Finnur wrote:
> nit: s/user/the user/

Done.

https://codereview.chromium.org/1739523002/diff/380001/chrome/browser/ui/andr...
File chrome/browser/ui/android/usb_chooser_android.h (right):

https://codereview.chromium.org/1739523002/diff/380001/chrome/browser/ui/andr...
chrome/browser/ui/android/usb_chooser_android.h:33: class UsbChooserAndroid :
public device::UsbService::Observer {
On 2016/03/15 19:03:54, newt wrote:
> For consistency with the java side of this class, it'd make sense to name this
> "UsbChooserDialog" or "UsbChooserDialogAndroid".

Done.

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:19:
view.invalidate();
On 2016/03/15 21:24:45, newt wrote:
> On 2016/03/15 21:11:26, Finnur wrote:
> > My ears are burning... ;)
> > 
> > It was definitely useful when I wrote it, because, as I recall, at that time
> > when a link was clicked in a dialog -- a link that *didn't* result in the
> dialog
> > closing -- you'd get this background highlight that would just stick. As if
> the
> > intent was to select the URL. Invalidating the view would cause it to redraw
> and
> > go away. 
> > 
> > If this no longer happens, then perhaps this class isn't needed, I guess.
But
> it
> > should be easy to test, though, just use a regular span and click a Refresh
> link
> > and see if it grows a background.
> 
> Ahhhh, thanks for explaining. So this *might* be needed in the item chooser
> dialogs, but definitely isn't needed in other uses of
NoUnderlineClickableSpan.
> 
> In that case, Jun, could you check whether this is still needed in the item
> chooser dialog case, and if so, move the code into the choose dialog(s).
> Otherwise, delete this altogether. Thanks!
> 
> (Likewise for bgColor = TRANSPARENT below)

I did some test and it seems that this is needed for the chooser dialog case.
Otherwise, like Finnur pointed out, if a link click doesn't result in the dialog
closing, the link click would cause the background highlight to stick as if the
link text was selected.

So I removed this function here and moved it to chooser dialog.

Done.

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:26:
textPaint.bgColor = Color.TRANSPARENT;
On 2016/03/15 21:11:26, Finnur wrote:
> Might be related to the above, not sure. Don't remember. :)
> 
> Maybe a background color needed to be set for the whole thing to work. Or
maybe
> I'm just that much of a rebel to set a transparent background needlessly. I
can
> be crazy like that.

I did some test and it seems that with or without this line, view.invalidate();
works correctly and removes the link click background highlight. So I just
removed this line here.

Done.

[Finnur, 2016-03-16 13:35:14]

Seems like you have a minor test failure in your latest patch.

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:259:
super.onClick(view);
Should you not call super.onClick also?

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:26:
textPaint.bgColor = Color.TRANSPARENT;
Like I said. Coo coo crazy. ;)

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

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:71:
ItemChooserRow item = (ItemChooserRow) obj;
Perhaps consider adding:
  if (!(obj instanceof ItemChooserRow)) return false;
  if (this == obj) return true;
... above line 71.

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(left):

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:149:
// After discovery stops the list should still be visible,
Why is this changing (line 141 and below)?

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:212:
// Add item1.
nit: s/item1/item 1/
(same with item2 and incidents below)

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:239:
assertTrue(itemAdapter.isEmpty());
I would add verification that the list is showing 'no items' text and right
controls are visible/hidden (similar to test above).

[newt (away), 2016-03-16 17:17:17]

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java
(right):

https://codereview.chromium.org/1739523002/diff/380001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/text/NoUnderlineClickableSpan.java:26:
textPaint.bgColor = Color.TRANSPARENT;
On 2016/03/16 13:35:14, Finnur wrote:
> Like I said. Coo coo crazy. ;)

I'm sure there was a reason at the time :)

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

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:106:
new BluetoothClickableSpan(LinkType.RESTART_SEARCH, mContext)));
indentation is still off

Note that "git cl format" on Java files is experimental and doesn't work 100%
(It's actually based on clang-format, so it's kind of amazing that it
understands any Java at all).

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

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:375:
mItemAdapter.remove(item);
Why does addItemToList() set the progress bar visibility to GONE, but
removeItemFromList() doesn't? (This is just a question; I'm not sure whether
they should match, but is seems likely.)

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:142: *
Shows USB Get help center link.
I think you can remove this comment and just rename the method to, e.g.
"nativeLoadUsbHelpPage()"

[juncai, 2016-03-16 21:01:35]

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

https://codereview.chromium.org/1739523002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:259:
super.onClick(view);
On 2016/03/16 13:35:14, Finnur wrote:
> Should you not call super.onClick also?

I made the modification at Patch set 22 and didn't call super.onClick here. It
moved: view.invalidate(); back here instead of at base class
NoUnderlineClickableSpan since it is needed by chooser dialog only.

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

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:106:
new BluetoothClickableSpan(LinkType.RESTART_SEARCH, mContext)));
On 2016/03/16 17:17:16, newt wrote:
> indentation is still off
> 
> Note that "git cl format" on Java files is experimental and doesn't work 100%
> (It's actually based on clang-format, so it's kind of amazing that it
> understands any Java at all).

Done. Hope I get the indentation right this time, :).

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

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:71:
ItemChooserRow item = (ItemChooserRow) obj;
On 2016/03/16 13:35:14, Finnur wrote:
> Perhaps consider adding:
>   if (!(obj instanceof ItemChooserRow)) return false;
>   if (this == obj) return true;
> ... above line 71.

Done.

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:375:
mItemAdapter.remove(item);
On 2016/03/16 17:17:16, newt wrote:
> Why does addItemToList() set the progress bar visibility to GONE, but
> removeItemFromList() doesn't? (This is just a question; I'm not sure whether
> they should match, but is seems likely.)

I think when removeItemFromList() is called, it means some items are already
added by calling addItemToList(), so mProgressBar.setVisibility(View.GONE); is
already called.

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java
(right):

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/UsbChooserDialog.java:142: *
Shows USB Get help center link.
On 2016/03/16 17:17:16, newt wrote:
> I think you can remove this comment and just rename the method to, e.g.
> "nativeLoadUsbHelpPage()"

Done.

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(left):

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:149:
// After discovery stops the list should still be visible,
On 2016/03/16 13:35:14, Finnur wrote:
> Why is this changing (line 141 and below)?

I think since State.PROGRESS_UPDATE_AVAILABLE was removed, and
mStatus.setText(mLabels.mStatusActive); is only called when the state is
PROGRESS_UPDATE_AVAILABLE. So the "statusActive" is never shown as the status
text at the bottom of the chooser dialog. And the removed test code checks:
assertEquals("statusActive", statusView.getText().toString());
So the test code fails and it needs to be removed.

And the member variable mStatusActive at ItemChooserLabels is not used any more,
I will remove it and also remove the related strings.

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java
(right):

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:212:
// Add item1.
On 2016/03/16 13:35:14, Finnur wrote:
> nit: s/item1/item 1/
> (same with item2 and incidents below)

Done.

https://codereview.chromium.org/1739523002/diff/420001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ItemChooserDialogTest.java:239:
assertTrue(itemAdapter.isEmpty());
On 2016/03/16 13:35:14, Finnur wrote:
> I would add verification that the list is showing 'no items' text and right
> controls are visible/hidden (similar to test above).

Done.

[Finnur, 2016-03-16 22:10:41]

LGTM

[newt (away), 2016-03-17 01:53:21]

Java files lgtm, after two small comments

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

https://codereview.chromium.org/1739523002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:110:
new SpanInfo("<link>", "</link>",
nit: The indentation was correct to begin with (before your CL). No need to
change it :)

The rule is: indent by 8 when wrapping. Additional lines don't need extra
indentation unless they're wrapped inside an additional level of parentheses.

Also, Eclipse will generally get the indentation correct for you. Just highlight
the code you want to format and press Ctrl+Shift+F. (I don't recommend
Ctrl+Shift+F on an entire file though, as it can sometimes reformat things that
shouldn't be changed. One day, hopefully, we'll have a tool like clang-format or
go format that "just works" for Java).

https://codereview.chromium.org/1739523002/diff/440001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java
(right):

https://codereview.chromium.org/1739523002/diff/440001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:156:
private static String removeMultipleLinkTags(String message) {
Why not combine this into the method above?

[juncai, 2016-03-17 18:21:35]

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

https://codereview.chromium.org/1739523002/diff/440001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:110:
new SpanInfo("<link>", "</link>",
On 2016/03/17 01:53:20, newt wrote:
> nit: The indentation was correct to begin with (before your CL). No need to
> change it :)
> 
> The rule is: indent by 8 when wrapping. Additional lines don't need extra
> indentation unless they're wrapped inside an additional level of parentheses.
> 
> Also, Eclipse will generally get the indentation correct for you. Just
highlight
> the code you want to format and press Ctrl+Shift+F. (I don't recommend
> Ctrl+Shift+F on an entire file though, as it can sometimes reformat things
that
> shouldn't be changed. One day, hopefully, we'll have a tool like clang-format
or
> go format that "just works" for Java).

Done.

https://codereview.chromium.org/1739523002/diff/440001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java
(right):

https://codereview.chromium.org/1739523002/diff/440001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/BluetoothChooserDialogTest.java:156:
private static String removeMultipleLinkTags(String message) {
On 2016/03/17 01:53:20, newt wrote:
> Why not combine this into the method above?

Done.

[juncai, 2016-03-23 18:18:11]

kindly ping yfriedman@, :).

[Yaron, 2016-03-24 15:33:47]

lgtm

[Reilly Grant (use Gerrit), 2016-03-25 17:37:18]

lgtm

[juncai, 2016-03-25 17:38:23]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-03-25 17:38:23]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1739523002/#ps500001
(title: " override hashCode() for ItemChooserRow to fix Android compile error.")

[commit-bot: I haz the power, 2016-03-25 17:38:36]

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

[commit-bot: I haz the power, 2016-03-25 18:02:32]

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

[commit-bot: I haz the power, 2016-03-25 18:02:34]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[juncai, 2016-03-25 20:32:29]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-03-25 20:32:29]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
yfriedman@chromium.org, reillyg@chromium.org, newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1739523002/#ps520001
(title: "rebase and added NoUnderlineClickableSpan.java to ui/android/BUILD.gn")

[commit-bot: I haz the power, 2016-03-25 20:33:05]

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

[commit-bot: I haz the power, 2016-03-25 20:40:43]

Description was changed from

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=591735
==========

to

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=591735
==========

[commit-bot: I haz the power, 2016-03-25 20:40:45]

Committed patchset #27 (id:520001)

[commit-bot: I haz the power, 2016-03-25 20:42:28]

Description was changed from

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=591735
==========

to

==========
WebUsb Android chooser UI

This patch added code to display a chooser UI on Android for WebUsb.

BUG=591735

Committed: https://crrev.com/4cbad6ed5522e60cda2aa728c08e3aa80545e360
Cr-Commit-Position: refs/heads/master@{#383351}
==========

[commit-bot: I haz the power, 2016-03-25 20:42:30]

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