bluetooth: Add URLs for Web Bluetooth help links.

bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
  https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_ChooserPhone#%2F02_Loading.jpg
* Updates text on
  https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_ChooserPhone#%2F02_Loading.jpg
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
  https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_ChooserPhone#%2F03_NoneAfterTimeOut.jpg
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613
Committed: https://crrev.com/d52c2c72c58d3d566bc2bd8c2bf4835f2cc721e4
Cr-Commit-Position: refs/heads/master@{#376522}

[ortuno, 2016-02-18 00:42:52]

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

[ortuno, 2016-02-18 00:42:53]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-02-18 01:41:51]

Description was changed from

==========
bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613
==========

to

==========
bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613
==========

[Jeffrey Yasskin, 2016-02-18 01:42:26]

The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710443002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710443002/40001

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

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Jeffrey Yasskin, 2016-02-18 02:17:24]

LGTM with the below changes.

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.cc:28: web_contents_ =
content::WebContents::FromRenderFrameHost(frame);
Use the constructor-initializer-list for this.

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.cc:162: void
BluetoothChooserAndroid::OpenURL(const char url[]) {
"const char*" here too (mentioning it because changing just the other won't give
you a compiler error)

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/bluetooth_chooser_android.h (right):

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.h:20:
BluetoothChooserAndroid(content::RenderFrameHost* frame,
Could you add a comment here that the event_handler and frame must outlive the
BluetoothChooserAndroid? It's guaranteed by the requirements on
WebContentsDelegate::RunBluetoothChooser, and it's nice to have a tight chain of
lifetime requirements like this.

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.h:55: void OpenURL(const
char url[]);
Use "const char* url". The behavior of [] in argument lists is equivalent but
confuses everyone.

https://codereview.chromium.org/1710443002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1710443002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1197: case
BluetoothChooser::Event::DENIED_PERMISSION:
Could you comment explicitly that all of the remaining events result in the
chooser closing?

I'm worried about the change in lifetime guarantees here, compared to
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/br....
Definitely update that file to say something like "The BluetoothChooser object
may be destroyed synchronously after any call to the EventHandler." Do we have
any problems with this in the Views or Mac implementation?

[ortuno, 2016-02-18 18:26:33]

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.cc:28: web_contents_ =
content::WebContents::FromRenderFrameHost(frame);
On 2016/02/18 at 02:17:24, Jeffrey Yasskin wrote:
> Use the constructor-initializer-list for this.

Done.

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.cc:162: void
BluetoothChooserAndroid::OpenURL(const char url[]) {
On 2016/02/18 at 02:17:24, Jeffrey Yasskin wrote:
> "const char*" here too (mentioning it because changing just the other won't
give you a compiler error)

Done.

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/bluetooth_chooser_android.h (right):

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.h:20:
BluetoothChooserAndroid(content::RenderFrameHost* frame,
On 2016/02/18 at 02:17:24, Jeffrey Yasskin wrote:
> Could you add a comment here that the event_handler and frame must outlive the
BluetoothChooserAndroid? It's guaranteed by the requirements on
WebContentsDelegate::RunBluetoothChooser, and it's nice to have a tight chain of
lifetime requirements like this.

Done.

https://codereview.chromium.org/1710443002/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.h:55: void OpenURL(const
char url[]);
On 2016/02/18 at 02:17:24, Jeffrey Yasskin wrote:
> Use "const char* url". The behavior of [] in argument lists is equivalent but
confuses everyone.

Done.

https://codereview.chromium.org/1710443002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1710443002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1197: case
BluetoothChooser::Event::DENIED_PERMISSION:
On 2016/02/18 at 02:17:24, Jeffrey Yasskin wrote:
> Could you comment explicitly that all of the remaining events result in the
chooser closing?
> 
> I'm worried about the change in lifetime guarantees here, compared to
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/br....
Definitely update that file to say something like "The BluetoothChooser object
may be destroyed synchronously after any call to the EventHandler."

The only change in lifetime guarantees is that now SHOW_* events also close the
chooser. Modified the existing comment about Event::CANCELLED and
Event::SELECTED. Do you think I should add the synchronous part?

> Do we have any problems with this in the Views or Mac implementation?

The Views and Mac implementations never send SHOW_* events so this shouldn't
affect them. I checked Jun's patch and when you click on the help link,
BluetoothDispatcherHost gets a CANCELLED event.

[ortuno, 2016-02-19 16:36:41]

ortuno@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org, tedchoc@chromium.org

[ortuno, 2016-02-19 16:36:42]

tedchoc: PTAL at chrome/browser/ui/android and chrome/android/

[Ilya Sherman, 2016-02-19 16:38:53]

histograms.xml lgtm

[ortuno, 2016-02-19 16:40:56]

jam: PTAL: content/public

[jam, 2016-02-19 17:00:04]

lgtm

[Ted C, 2016-02-19 17:54:01]

lgtm w/ a couple nits.

I suspect my general questions at the end are handled already but just wanted to
make sure.

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

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:110:
new NoUnderlineClickableSpan(LinkType.RESTART_SEARCH, mContext)));
the previous was indenting was correct...but i assume this was by git cl format
(which still has problems w/ java)

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

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:76:
public final SpannableString mTitle;
fwiw since these are all public, they shouldn't start with m, but should just be
title, searching, etc...

https://source.android.com/source/code-style.html#follow-field-naming-convent...

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:422:
: mLabels.mStatusIdleSomeFound);
should be 8 spaces from the start of the line.  Probably would look better if
you put but ternary options on the second line.

https://codereview.chromium.org/1710443002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1710443002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.cc:164:
ui::PAGE_TRANSITION_AUTO_TOPLEVEL, false /* is_renderer_initiated */));
Out of curiosity, what happens if the page that triggered the bluetooth
discovery dialog crashes/renderer dies?

In particular on svelte devices, there is a limit of one renderer so this will
pretty much guarantee the current one goes away.

Does the dialog get dismissed if you click the help link (probably covered by
the TODO in the other file)?

[ortuno, 2016-02-19 18:26:28]

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

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:110:
new NoUnderlineClickableSpan(LinkType.RESTART_SEARCH, mContext)));
On 2016/02/19 at 17:54:00, Ted C wrote:
> the previous was indenting was correct...but i assume this was by git cl
format (which still has problems w/ java)

Done.

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

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:76:
public final SpannableString mTitle;
On 2016/02/19 at 17:54:01, Ted C wrote:
> fwiw since these are all public, they shouldn't start with m, but should just
be title, searching, etc...
> 
>
https://source.android.com/source/code-style.html#follow-field-naming-convent...

Thanks for pointing that out. I opened crbug.com/588158 to get the naming fixed.

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:422:
: mLabels.mStatusIdleSomeFound);
On 2016/02/19 at 17:54:01, Ted C wrote:
> should be 8 spaces from the start of the line.  Probably would look better if
you put but ternary options on the second line.

Done. I think. Can you check I got it right?

https://codereview.chromium.org/1710443002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1710443002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/bluetooth_chooser_android.cc:164:
ui::PAGE_TRANSITION_AUTO_TOPLEVEL, false /* is_renderer_initiated */));
On 2016/02/19 at 17:54:01, Ted C wrote:
> Out of curiosity, what happens if the page that triggered the bluetooth
discovery dialog crashes/renderer dies?
> 
> In particular on svelte devices, there is a limit of one renderer so this will
pretty much guarantee the current one goes away.
> 

Right now the dialog would stay on top. The issue in the TODO in
BluetoothChooserDialog.java is meant to fix cases like this.

> Does the dialog get dismissed if you click the help link (probably covered by
the TODO in the other file)?

It does get dismissed. Right now the event_handler closes the chooser when you
click the help link, but in the future we would want the chooser to be smart
enough to know that a navigation, new tab, crash, etc. means it should be
dismissed, in the same way the chrome_bubble_manager.cc does it for desktop
bubbles.

[Ted C, 2016-02-19 18:39:30]

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

https://codereview.chromium.org/1710443002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:422:
: mLabels.mStatusIdleSomeFound);
On 2016/02/19 18:26:28, ortuno wrote:
> On 2016/02/19 at 17:54:01, Ted C wrote:
> > should be 8 spaces from the start of the line.  Probably would look better
if
> you put but ternary options on the second line.
> 
> Done. I think. Can you check I got it right?

looks good

[ortuno, 2016-02-19 18:39:51]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-02-19 18:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, tedchoc@chromium.org, isherman@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1710443002/#ps100001
(title: "Address tedchoc's comments")

[commit-bot: I haz the power, 2016-02-19 18:40:32]

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

[commit-bot: I haz the power, 2016-02-19 20:12:28]

Description was changed from

==========
bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613
==========

to

==========
bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613
==========

[commit-bot: I haz the power, 2016-02-19 20:12:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-19 20:15:00]

Description was changed from

==========
bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613
==========

to

==========
bluetooth: Add URLs for Web Bluetooth help links.

* Adds URLs to all help links in the Android chooser.
* Removes the link on "pair with":
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Searching for devices... get help" with a help center link on
  'get help'.
* Updates text on
 
https://folio.googleplex.com/chrome-ux/mocks/236-fizz/choosers/032515_Chooser...
  to read "Get help" and link to same help center article.
* Rejects promises when clicking help links.

BUG=570613

Committed: https://crrev.com/d52c2c72c58d3d566bc2bd8c2bf4835f2cc721e4
Cr-Commit-Position: refs/heads/master@{#376522}
==========

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

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