Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1861213002:
Convert host names in ACE to Unicode in ElideHost (Closed)
|
Created:
4 years, 8 months ago by jungshik at Google Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert hostnames in ACE to Unicode in ElideHost
Call IDNToUnicode to convert host (full fqdn) and domain to
Unicode (per the IDN display policy) in ElideHost
BUG=593177
TEST=components_unittests --gtest_filter= *HostEliding
Committed: https://crrev.com/ee46c77dbdf32a39912ba6589aabd8db51eaed29
Cr-Commit-Position: refs/heads/master@{#385598}
Patch Set 1 #Patch Set 2 : add an IDN test to ElideHost test #
Total comments: 2
Patch Set 3 : a bit of simplication #Patch Set 4 : ace => puny; comment added #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Convert Punycode to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding* ========== to ========== Convert Punycode to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ==========
Description was changed from ========== Convert Punycode to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ========== to ========== Convert Punycode to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ==========
jshin@chromium.org changed reviewers: + palmer@chromium.org, pkasting@google.com
Description was changed from ========== Convert Punycode to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ========== to ========== Convert hostnames in ACE to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ==========
PTAL. Thanks
Shouldn't we do this if and only if the language the hostname is in (e.g. Korean) is in the Profile's known languages list? (I.e. people who don't read Korean see Punycode; people who do read Korean see Korean). (Isn't that also the Omnibox logic? I hope so, right?)
On 2016/04/06 20:00:13, palmer wrote: > Shouldn't we do this if and only if the language the hostname is in (e.g. > Korean) is in the Profile's known languages list? (I.e. people who don't read > Korean see Punycode; people who do read Korean see Korean). (Isn't that also the > Omnibox logic? I hope so, right?) IDN display policy (in ToT) is not dependent on Accept-language any more :-)
On 2016/04/06 21:27:48, jshin (jungshik at google) wrote: > On 2016/04/06 20:00:13, palmer wrote: > > Shouldn't we do this if and only if the language the hostname is in (e.g. > > Korean) is in the Profile's known languages list? (I.e. people who don't read > > Korean see Punycode; people who do read Korean see Korean). (Isn't that also > the > > Omnibox logic? I hope so, right?) > > IDN display policy (in ToT) is not dependent on Accept-language any more :-) To be clear, I landed that change last week. As a result, IDNToUnicode does not take 'languages' argument any more.
On 2016/04/06 20:00:13, palmer wrote: > Shouldn't we do this if and only if the language the hostname is in (e.g. > Korean) is in the Profile's known languages list? (I.e. people who don't read > Korean see Punycode; people who do read Korean see Korean). (Isn't that also the > Omnibox logic? I hope so, right?) The omnibox logic recently dropped the languages portion as part of changing to a new algorithm that aims for Firefox compatibility and broader Unicode hostname support.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
LGTM https://codereview.chromium.org/1861213002/diff/20001/components/url_formatte... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1861213002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:81: // Get Host in ACE(ASCII Compatible Encoding). Since we don't use "ACE" most other places in Chrome, I would avoid changing the existing comment or variable names here, and then instead add a comment on the IDNToUnicode() calls below: GURL stores IDN hostnames in punycode. Convert back to Unicode for display to the user. (IDNToUnicode() will only perform this conversion if it's safe to display this host/domain in Unicode.)
OK, LGTM too.
Peter, thanks for the review. My change (PS#3) and your comment crossed each other in transit. It does not matter much, but can you take another look if you like PS#4(PS#3) better than PS#2? Thanks again https://codereview.chromium.org/1861213002/diff/20001/components/url_formatte... File components/url_formatter/elide_url.cc (right): https://codereview.chromium.org/1861213002/diff/20001/components/url_formatte... components/url_formatter/elide_url.cc:81: // Get Host in ACE(ASCII Compatible Encoding). On 2016/04/06 21:38:39, Peter Kasting wrote: > Since we don't use "ACE" most other places in Chrome, I would avoid changing the > existing comment or variable names here, and then instead add a comment on the > IDNToUnicode() calls below: > > GURL stores IDN hostnames in punycode. Convert back to Unicode for display to > the user. (IDNToUnicode() will only perform this conversion if it's safe to > display this host/domain in Unicode.) Ok. (I regret not having used ACE before :-)).
PS4 seems fine.
The CQ bit was checked by jshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, palmer@chromium.org Link to the patchset: https://codereview.chromium.org/1861213002/#ps60001 (title: "ace => puny; comment added")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861213002/60001
Message was sent while issue was closed.
Description was changed from ========== Convert hostnames in ACE to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ========== to ========== Convert hostnames in ACE to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Convert hostnames in ACE to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding ========== to ========== Convert hostnames in ACE to Unicode in ElideHost Call IDNToUnicode to convert host (full fqdn) and domain to Unicode (per the IDN display policy) in ElideHost BUG=593177 TEST=components_unittests --gtest_filter= *HostEliding Committed: https://crrev.com/ee46c77dbdf32a39912ba6589aabd8db51eaed29 Cr-Commit-Position: refs/heads/master@{#385598} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee46c77dbdf32a39912ba6589aabd8db51eaed29 Cr-Commit-Position: refs/heads/master@{#385598} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
