Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
|origin_string| at BluetoothChooserAndroid constructor.

BUG=593120
Committed: https://crrev.com/dae99160f5ed47a199367e9f2a400c6b66f51384
Cr-Commit-Position: refs/heads/master@{#380469}

[juncai, 2016-03-08 20:56:10]

Description was changed from

==========
use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
origin_string at BluetoothChooserAndroid constructor.

BUG=593120
==========

to

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
origin_string at BluetoothChooserAndroid constructor.

BUG=593120
==========

[juncai, 2016-03-08 20:59:33]

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

[juncai, 2016-03-08 20:59:33]

This patch is the follow-up of comment at:
https://codereview.chromium.org/1739523002/diff/280001/chrome/browser/ui/andr...

Please take a look.

[juncai, 2016-03-09 01:30:04]

Description was changed from

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
origin_string at BluetoothChooserAndroid constructor.

BUG=593120
==========

to

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
|origin_string| at BluetoothChooserAndroid constructor.

BUG=593120
==========

[Yaron, 2016-03-09 04:15:04]

yfriedman@chromium.org changed reviewers:
+ palmer@chromium.org, yfriedman@chromium.org

[Yaron, 2016-03-09 04:15:04]

+palmer to confirm this is necessary. I think it might but he's the expert

[Yaron, 2016-03-09 04:20:00]

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:55:
GURL(origin.Serialize()), languages));
err, serializing the origin and then creating a GURL isn't the right thing to
do.

It's either the case that you use:
1) frame->GetLastCommittedOrigin()  directly (i.e. I"m mistaken)
2)  url_formatter::FormatUrlForSecurityDisplay(frame->GetLastCommittedURL())

[Finnur, 2016-03-09 13:31:32]

palmer@ is probably the right person to weigh in, but on the face of it (from
reading the comments for FormatUrlForSecurityDisplay) it seems like a good thing
to use because the origin will be displayed in the dialog and the user will make
a security decision based on it.

The only question in my mind is: What happens if I allow Bluetooth for one site
over HTTPS and then access the same site via HTTP (or the other way around)?
Does it also have Bluetooth access? I forget how it works (scheib@ or jyasskin@
would know), but *if* we're granting the permission to the site (independent of
connection type) then I suspect FormatUrlForSecurityDisplayOmitScheme might be a
better function to use.

[Jeffrey Yasskin, 2016-03-09 16:40:09]

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

[Jeffrey Yasskin, 2016-03-09 16:40:10]

On 2016/03/09 13:31:32, Finnur wrote:
> palmer@ is probably the right person to weigh in, but on the face of it (from
> reading the comments for FormatUrlForSecurityDisplay) it seems like a good
thing
> to use because the origin will be displayed in the dialog and the user will
make
> a security decision based on it.
> 
> The only question in my mind is: What happens if I allow Bluetooth for one
site
> over HTTPS and then access the same site via HTTP (or the other way around)?
> Does it also have Bluetooth access?

No, only HTTPS sites have Bluetooth access. Even if we didn't have the
secure-context restriction, we'll be persisting permissions by origin, and the
origin includes the scheme.

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:55:
GURL(origin.Serialize()), languages));
On 2016/03/09 04:19:59, Yaron wrote:
> err, serializing the origin and then creating a GURL isn't the right thing to
> do.
> 
> It's either the case that you use:
> 1) frame->GetLastCommittedOrigin()  directly (i.e. I"m mistaken)
> 2)  url_formatter::FormatUrlForSecurityDisplay(frame->GetLastCommittedURL())

Hm, do we want to be switching to url::Origins everywhere that's using URLs for
security reasons, or not? If we do, there should be a
FormatOriginForSecurityDisplay.

[palmer, 2016-03-09 20:19:39]

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:55:
GURL(origin.Serialize()), languages));
> Hm, do we want to be switching to url::Origins everywhere that's using URLs
for
> security reasons, or not? If we do, there should be a
> FormatOriginForSecurityDisplay.

Ultimately, yes, we want to use url::Origin where appropriate (I am in the
middle of 1 of the many necessary large refactorings, for example). Thus we will
need a FormatOriginForSecurityDisplay. Until then, using
url_formatter::FormatUrlForSecurityDisplay(frame->GetLastCommittedURL()) is
probably cleanest.

[juncai, 2016-03-10 00:13:55]

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1771353004/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:55:
GURL(origin.Serialize()), languages));
On 2016/03/09 20:19:39, palmer wrote:
> > Hm, do we want to be switching to url::Origins everywhere that's using URLs
> for
> > security reasons, or not? If we do, there should be a
> > FormatOriginForSecurityDisplay.
> 
> Ultimately, yes, we want to use url::Origin where appropriate (I am in the
> middle of 1 of the many necessary large refactorings, for example). Thus we
will
> need a FormatOriginForSecurityDisplay. Until then, using
> url_formatter::FormatUrlForSecurityDisplay(frame->GetLastCommittedURL()) is
> probably cleanest.

Done.

[Yaron, 2016-03-10 03:33:38]

lgtm

[Finnur, 2016-03-10 12:15:17]

Thanks for the clarification, Jeffrey.

LGTM.

[juncai, 2016-03-10 21:16:12]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2016-03-10 21:16:52]

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

[commit-bot: I haz the power, 2016-03-10 21:22:27]

Description was changed from

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
|origin_string| at BluetoothChooserAndroid constructor.

BUG=593120
==========

to

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
|origin_string| at BluetoothChooserAndroid constructor.

BUG=593120
==========

[commit-bot: I haz the power, 2016-03-10 21:22:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-10 21:24:15]

Description was changed from

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
|origin_string| at BluetoothChooserAndroid constructor.

BUG=593120
==========

to

==========
Use FormatUrlForSecurityDisplay at BluetoothChooserAndroid constructor

This patch uses url_formatter::FormatUrlForSecurityDisplay to construct
|origin_string| at BluetoothChooserAndroid constructor.

BUG=593120

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

[commit-bot: I haz the power, 2016-03-10 21:24:16]

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