permissions prompt: use unicode hostname, not IDN

permissions prompt: use unicode hostname, not IDN

The origin in a GURL is always IDN, which means that the permissions prompt
shows punycode instead of the actual unicode hostname.

BUG=630667

[Elly Fong-Jones, 2016-09-16 11:19:32]

Description was changed from

==========
permissions prompt: use unicode hostname, not IDN

The origin in a GURL is always IDN, which means that the permissions prompt
shows punycode instead of the actual unicode hostname.

BUG=630667
==========

to

==========
permissions prompt: use unicode hostname, not IDN

The origin in a GURL is always IDN, which means that the permissions prompt
shows punycode instead of the actual unicode hostname.

BUG=630667
==========

[Elly Fong-Jones, 2016-09-16 11:19:32]

ellyjones@chromium.org changed reviewers:
+ felt@chromium.org

[Elly Fong-Jones, 2016-09-16 11:19:54]

felt: ptal? I'm not sure if it's always safe to do this unconditional IDN ->
unicode conversion here.

[Elly Fong-Jones, 2016-09-21 15:16:40]

ping? :)

[Elly Fong-Jones, 2016-09-23 17:09:57]

ellyjones@chromium.org changed reviewers:
+ rsesek@chromium.org
- felt@chromium.org

[Elly Fong-Jones, 2016-09-23 17:10:09]

rsesek: ptal? :)

[Robert Sesek, 2016-09-23 17:13:11]

1 nit

https://codereview.chromium.org/2341213004/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2341213004/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:557:
base::string16 display_url = url_formatter::FormatUrlForSecurityDisplay(
naming: camelCase in ObjC

[Elly Fong-Jones, 2016-09-23 17:18:53]

ellyjones@chromium.org changed reviewers:
+ felt@chromium.org

[Elly Fong-Jones, 2016-09-23 17:19:08]

rsesek, ptal for cocoa owner :)
felt, ptal for views owner :)

https://codereview.chromium.org/2341213004/diff/1/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2341213004/diff/1/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:557:
base::string16 display_url = url_formatter::FormatUrlForSecurityDisplay(
On 2016/09/23 17:13:11, Robert Sesek wrote:
> naming: camelCase in ObjC

Done.

[Robert Sesek, 2016-09-23 17:21:31]

LGTM

[felt, 2016-09-27 11:23:07]

lgtm

[felt, 2016-09-27 11:26:16]

This looks convincingly right... but to be honest I have no experience with
unicode and IDN. So even though I already hit the l-g-t-m button out of
enthusiasm for bugfixes, could you add some tests?

Presumably this same issue comes up where URLs are displayed elsewhere, too?
Should this be done inside FormatUrlForSecurityDisplay so that other callers can
get the same benefit?

[Elly Fong-Jones, 2016-09-27 16:05:48]

On 2016/09/27 11:26:16, felt wrote:
> This looks convincingly right... but to be honest I have no experience with
> unicode and IDN. So even though I already hit the l-g-t-m button out of
> enthusiasm for bugfixes, could you add some tests?
> 
> Presumably this same issue comes up where URLs are displayed elsewhere, too?
> Should this be done inside FormatUrlForSecurityDisplay so that other callers
can
> get the same benefit?

Hm, it looks like the existing behavior of FormatUrlForSecurityDisplay (always
returning IDN) is by design and enforced by a bunch of unit tests. I'll follow
up with jshin@ to see what's up with that / why it would be desirable to use
IDN.

[jungshik at Google, 2016-09-27 17:51:16]

On 2016/09/27 16:05:48, Elly Fong-Jones wrote:
> On 2016/09/27 11:26:16, felt wrote:
> > This looks convincingly right... but to be honest I have no experience with
> > unicode and IDN. So even though I already hit the l-g-t-m button out of
> > enthusiasm for bugfixes, could you add some tests?
> > 
> > Presumably this same issue comes up where URLs are displayed elsewhere, too?
> > Should this be done inside FormatUrlForSecurityDisplay so that other callers
> can
> > get the same benefit?
> 
> Hm, it looks like the existing behavior of FormatUrlForSecurityDisplay (always
> returning IDN) is by design and enforced by a bunch of unit tests. I'll follow
> up with jshin@ to see what's up with that / why it would be desirable to use
> IDN.

Thanks for taking on this issue. I noticed this behavior (showing punycode
instead of Unicode in the permission bubble; 
please do "s/IDN/punycode/" in the CL description. I know our function names are
not 'accurate') and meant to change it. 

I was a bit puzzled at the behavior because I thought I had fixed in April. (
either https://codereview.chromium.org/1841653003 or
https://codereview.chromium.org/1861213002), but apparently I overlooked it. 

Eventually, I believe that we have to change FormatUrlForSecurityDisplay to show
Unicode (after a security review). 
I'll file a bug on that. 

In the meantime, this one-spot fix looks good to me.

[jungshik at Google, 2016-09-27 17:57:59]

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

[jungshik at Google, 2016-09-27 17:58:00]

I have a second thought (I should have looked at it more carefully). Let's
change FormatUrlForSecurityDisplay after sec review.

https://codereview.chromium.org/2341213004/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2341213004/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:559: if
(base::IsStringASCII(displayUrl))
Here does the url have any query or path part or is it just 'scheme + hostname'?
 Hmm...   even if it does not have path/query, calling IDNToUnicode over a whole
URL is odd (although likely to work in this case)  because IDNToUnicode only
deals with a hostname. 

So, I think we'd better tackle this issue in FormatUrlForSecurityDisplay.