Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*

Committed: https://crrev.com/78809c4d824d07d465be71140c7951f3af84f023
Cr-Commit-Position: refs/heads/master@{#423646}

[jungshik at Google, 2016-09-28 19:55:22]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

BUG=650760
TEST=...
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[jungshik at Google, 2016-09-28 20:19:03]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

For now, continue to use punycode if there's any RTL characters in the domain
name. See the bug for more details on this issue.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[jungshik at Google, 2016-09-28 20:19:55]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

For now, continue to use punycode if there's any RTL characters in the domain
name. See the bug for more details on this issue.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

For now, continue to use punycode if there's any RTL characters in the
domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[jungshik at Google, 2016-09-28 20:20:00]

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

[commit-bot: I haz the power, 2016-09-28 20:20:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 20:23:56]

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

[commit-bot: I haz the power, 2016-09-28 20:23:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jungshik at Google, 2016-09-28 22:42:13]

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

[commit-bot: I haz the power, 2016-09-28 22:42:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 23:48:33]

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

[commit-bot: I haz the power, 2016-09-28 23:48:34]

Dry run: This issue passed the CQ dry run.

[jungshik at Google, 2016-09-29 17:14:10]

jshin@chromium.org changed reviewers:
+ ellyjones@chromium.org, felt@chromium.org, pkasting@google.com

[jungshik at Google, 2016-09-29 17:14:11]

Hello, can you take a look? Thanks

[jungshik at Google, 2016-09-29 17:15:09]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

For now, continue to use punycode if there's any RTL characters in the
domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

For now, continue to use punycode if there's any RTL characters in the
domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[jungshik at Google, 2016-09-29 17:15:09]

jshin@chromium.org changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[jungshik at Google, 2016-09-29 18:29:59]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay.

For now, continue to use punycode if there's any RTL characters in the
domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[Elly Fong-Jones, 2016-09-30 11:18:24]

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
File components/url_formatter/elide_url.h (right):

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be iin ACE/punycode for
now. ( http://crbug.com/650760 )
nit: iin -> in

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
components/url_formatter/elide_url.h:70: // See |url_formatter::FormatUrl| for
more details on the algorithm).
nit: -)

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
File components/url_formatter/elide_url_unittest.cc (right):

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
components/url_formatter/elide_url_unittest.cc:262:
L"http://\x4e2d\x56fd.icom.museum"},
is there a test covering the behavior for strong RTL chars? (ie, checking that
they remain in punycode)

[jungshik at Google, 2016-09-30 19:22:14]

Thank you, Elly. Comments addressed in the latest PS.

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
File components/url_formatter/elide_url.h (right):

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be iin ACE/punycode for
now. ( http://crbug.com/650760 )
On 2016/09/30 11:18:24, Elly Fong-Jones wrote:
> nit: iin -> in

Done.

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
components/url_formatter/elide_url.h:70: // See |url_formatter::FormatUrl| for
more details on the algorithm).
On 2016/09/30 11:18:24, Elly Fong-Jones wrote:
> nit: -)

Done.

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
File components/url_formatter/elide_url_unittest.cc (right):

https://codereview.chromium.org/2375803002/diff/40001/components/url_formatte...
components/url_formatter/elide_url_unittest.cc:262:
L"http://\x4e2d\x56fd.icom.museum"},
On 2016/09/30 11:18:24, Elly Fong-Jones wrote:
> is there a test covering the behavior for strong RTL chars? (ie, checking that
> they remain in punycode)

Yes, there are two of them right below this line :-). Hebrew and Arabic IDNs.
(they were there already so that I don't have to add :-) ).

[Elly Fong-Jones, 2016-10-03 10:22:49]

lgtm

[felt, 2016-10-03 18:24:45]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[felt, 2016-10-03 18:24:45]

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

[felt, 2016-10-03 18:25:23]

lgtm, but I admittedly don't know much about RTL. 

mgiuca, could you also review since I know you've worked a lot on origin display
with RTL characters?

[Peter Kasting, 2016-10-04 07:56:26]

Will try to get to this Tue 10/4

[jungshik at Google, 2016-10-04 17:40:01]

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

[commit-bot: I haz the power, 2016-10-04 17:40:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 18:48:34]

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

[commit-bot: I haz the power, 2016-10-04 18:48:35]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-10-05 01:22:01]

LGTM pending decision on the RTL issue.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
File components/url_formatter/elide_url.h (right):

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:67: // Internationalized domain names (IDN)
may be presented in Unicode if
Nit: Should "may" be "will" here?

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be in ACE/punycode for
now. ( http://crbug.com/650760 )
Definitely get Matt's comments on the RTL stuff.  I'm fine with whatever he
wants to do.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
File components/url_formatter/url_formatter.h (right):

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/url_formatter.h:133: // return the ASCII source so it
is still usable.
Nit: This comment makes it sound like we always convert to Unicode, and doesn't
really mention the safety checks.  Perhaps this comment should link to
http://dev.chromium.org/developers/design-documents/idn-in-google-chrome in
addition to, or instead of, the place you touched in elide_url.h.

[Matt Giuca, 2016-10-05 04:58:21]

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
File components/url_formatter/elide_url.cc (right):

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.cc:131: // TODO(jshin): Come up with a way to
show BiDi URLs 'safely' (e.g. wrap up
Nit: Bidi (not BiDi)

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.cc:133: // supported in the native UI.).  See
http://crbug.com/650760  or
nit: Replace ".)." with ")."

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.cc:134: // http://www.macchiato.com/bidi-url
.
I don't think it's worth linking to an external page from source code. Why not
paste this link into the bug and then just link to the crbug from here.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
File components/url_formatter/elide_url.h (right):

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be in ACE/punycode for
now. ( http://crbug.com/650760 )
nit: Remove spaces around the parentheses.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be in ACE/punycode for
now. ( http://crbug.com/650760 )
On 2016/10/05 01:22:00, Peter Kasting wrote:
> Definitely get Matt's comments on the RTL stuff.  I'm fine with whatever he
> wants to do.

I asked some questions on the bug (notably, where does this actually show up in
UI). Maybe we can hash it out there.

My thoughts are:
1. lgtm - I'm happy to land this now, as it is a conservative step in the right
direction, although not ideal.
2. I do want to change our RTL rendering to be less confusing and ambiguous (see
https://crbug.com/351639). But I don't want to block this on that.
3. For a medium-term solution, I am ambivalent about whether we want to keep it
in punycode or display it as the omnibox. I think the answer depends on exactly
what security contexts it will be shown in.

[jungshik at Google, 2016-10-06 00:27:45]

Thank you for taking a look. I addressed review comments and answered questions
below.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
File components/url_formatter/elide_url.cc (right):

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.cc:131: // TODO(jshin): Come up with a way to
show BiDi URLs 'safely' (e.g. wrap up
On 2016/10/05 04:58:20, Matt Giuca wrote:
> Nit: Bidi (not BiDi)

well... done :-)

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.cc:133: // supported in the native UI.).  See
http://crbug.com/650760  or
On 2016/10/05 04:58:20, Matt Giuca wrote:
> nit: Replace ".)." with ")."

Done.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.cc:134: // http://www.macchiato.com/bidi-url
.
On 2016/10/05 04:58:20, Matt Giuca wrote:
> I don't think it's worth linking to an external page from source code. Why not
> paste this link into the bug and then just link to the crbug from here.

Removed. That url is already in the bug.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
File components/url_formatter/elide_url.h (right):

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:67: // Internationalized domain names (IDN)
may be presented in Unicode if
On 2016/10/05 01:22:00, Peter Kasting wrote:
> Nit: Should "may" be "will" here?

Done.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be in ACE/punycode for
now. ( http://crbug.com/650760 )
On 2016/10/05 01:22:00, Peter Kasting wrote:
> Definitely get Matt's comments on the RTL stuff.  I'm fine with whatever he
> wants to do.

Acknowledged.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be in ACE/punycode for
now. ( http://crbug.com/650760 )
On 2016/10/05 04:58:21, Matt Giuca wrote:
> nit: Remove spaces around the parentheses.

Done.

https://codereview.chromium.org/2375803002/diff/60001/components/url_formatte...
components/url_formatter/elide_url.h:69: // will still be in ACE/punycode for
now. ( http://crbug.com/650760 )
On 2016/10/05 04:58:20, Matt Giuca wrote:
> On 2016/10/05 01:22:00, Peter Kasting wrote:
> > Definitely get Matt's comments on the RTL stuff.  I'm fine with whatever he
> > wants to do.
> 
> I asked some questions on the bug (notably, where does this actually show up
in
> UI). Maybe we can hash it out there.
> 
> My thoughts are:
> 1. lgtm - I'm happy to land this now, as it is a conservative step in the
right
> direction, although not ideal.
> 2. I do want to change our RTL rendering to be less confusing and ambiguous
(see
> https://crbug.com/351639). But I don't want to block this on that.

Yup, I'm aware of that bug. 

> 3. For a medium-term solution, I am ambivalent about whether we want to keep
it
> in punycode or display it as the omnibox. I think the answer depends on
exactly
> what security contexts it will be shown in.

I'm also ambivalent about that, which is why I took the most conservative
approach in this CL although I'm tempted to do what omnibox does. I also need to
test how our native UI codes (gfx/..) will handle Bidi formatting characters
(especially newer ones for 'isolation') before using them. And, if they're are
used in WebUI, we'd better use an HTML machinery for isolation instead of
formatting characters. So, I punted Bidi handling in this CL. 

Anyway, they'll be used in cases like :

'foobar.com asks for permission to use your location' 

or

 The proxy <ph name="DOMAIN">$1<ex>google.com</ex></ph> requires a username and
password.

See https://goo.gl/ZRBnfP for other examples (it's not super-convenient to look
up because clicking on 'IDS_..*' will not get you grd/grdp files due to
http://crbug.com/613347).

[jungshik at Google, 2016-10-06 18:53:10]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2016-10-06 18:53:11]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org,
ellyjones@chromium.org, pkasting@chromium.org, mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/2375803002/#ps100001
(title: "mention {LSI, PDI} as well")

[commit-bot: I haz the power, 2016-10-06 18:53:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 20:16:08]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

[commit-bot: I haz the power, 2016-10-06 20:16:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 20:18:08]

Description was changed from

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*
==========

to

==========
Use Unicode in Format{Origin,URL}forSecurityDisplay

Use Unicode instead of punycode for 'host' part of URL
in Format{Origin,URL}ForSecurityDisplay. This would only change
the way IDN(internationalized domain name) is displayed.

To be very conservative, continue to use punycode if there's any RTL
characters in the domain name. See the bug for more details on this issue.

Besides, change IDNToUnicode() to accept StringPiece instead of
|const string&|.

BUG=650760
TEST=components_unittests --gtest_filter=Format*Security*

Committed: https://crrev.com/78809c4d824d07d465be71140c7951f3af84f023
Cr-Commit-Position: refs/heads/master@{#423646}
==========

[commit-bot: I haz the power, 2016-10-06 20:18:09]

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