Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 23642003: Support IDNA 2008 (Closed)

Created:
6 years, 1 month ago by jungshik at Google
Modified:
6 years ago
CC:
chromium-reviews, Mark Larson, abarth-chromium, cbentzel+watch_chromium.org, jshin+watch_chromium.org, bulach, cbentzel
Visibility:
Public.

Description

Support IDNA 2008 with UTS46. UTS 46 provides a migration path from IDNA 2003 to IDNA 2008. 1. Use the up-to-date Unicode data; new characters were added and some case-folding/mapping have changed since Unicode 3.2 on which IDNA 2003 is based. 2. Define a case folding/mapping as is the case with IDNA 2003. 3. Use transitional mechanism for 4 deviant characters : German sharp-S, Greek final-sigma, ZWJ and ZWNJ. That is, the former two are mapped to 'ss' and regular sigma and the latter two are dropped. All the major browsers do this at the moment so allowing them does not do any good. We'll review this later as the consensus builds among browser vendors and registrars. We can also consider handling them separately. For instance, ZWJ/ZWNJ can be allowed with ContextJ rules, which requires a minor change in ICU's UTS 46 implementation. 4. Symbol and punctuations continue to be allowed. We also do the following: 1. Continue to "violate" STD3 rules about non-LDH (Letter, digits and hyphens) by allowing non-LDH's. That is no change from the current implementation. 2. Do not allow unassigned code points any more. With an up-to-date Unicode data, this does not make much difference. And the chance of new characters not yet reflected in our Unicode data popping up in a domain name is extremely low. 3. Continue to use CHECK_BIDI. The Bidi rule in IDNA 2008 is more permissive than in IDNA 2003. References: 1. http://unicode.org/reports/tr46/ and references therein to IDNA 2003 and 2008 RFCs. 2. What IE 10 does : http://goo.gl/3XBhqw 3. Mozilla bug : https://bugzilla.mozilla.org/show_bug.cgi?id=479520 BUG=61328 TEST=url_unittests (URLCanonTest.Host), net_unittests (NetUtilTest.IDNToU*), unittests (X509CertificateModelTest.*) R=brettw@chromium.org, pkasting@chromium.org, rsleevi@chromium.org, thakis@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225878

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 2

Patch Set 9 : #

Total comments: 1

Patch Set 10 : #

Total comments: 11

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Patch Set 15 : #

Total comments: 2

Patch Set 16 : #

Total comments: 2

Patch Set 17 : #

Patch Set 18 : #

Total comments: 3

Patch Set 19 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+260 lines, -68 lines) Patch
M chrome/common/net/x509_certificate_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +3 lines, -23 lines 0 comments Download
M net/base/net_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +62 lines, -22 lines 0 comments Download
M url/url.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M url/url_canon_icu.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +56 lines, -10 lines 0 comments Download
M url/url_canon_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +138 lines, -13 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
jungshik at Google
6 years, 1 month ago (2013-09-17 17:32:19 UTC) #1
abarth-chromium
The description for this CL just describes what the CL does, but it doesn't explain ...
6 years, 1 month ago (2013-09-17 17:51:45 UTC) #2
bulach
I haven't been working on the net/ layer for quite a while now, adding rsleevi ...
6 years, 1 month ago (2013-09-17 18:26:32 UTC) #3
jungshik at Google
Accidentally, I've just lost a long reply. I'll try to restore that from my memory. ...
6 years, 1 month ago (2013-09-17 22:52:57 UTC) #4
abarth-chromium
If we're moving in concert with IE and Firefox, then I'm much less worried. Thanks ...
6 years, 1 month ago (2013-09-17 22:58:26 UTC) #5
brettw
https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc File url/url_canon_icu.cc (right): https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc#newcode132 url/url_canon_icu.cc:132: static UIDNA* uidna = NULL; // will be leaked. ...
6 years ago (2013-09-18 19:39:00 UTC) #6
jungshik at Google
https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc File url/url_canon_icu.cc (right): https://codereview.chromium.org/23642003/diff/35001/url/url_canon_icu.cc#newcode132 url/url_canon_icu.cc:132: static UIDNA* uidna = NULL; // will be leaked. ...
6 years ago (2013-09-18 21:50:13 UTC) #7
brettw
If the object has weird requirements, generally I'd wrap in it a struct or class ...
6 years ago (2013-09-18 22:01:50 UTC) #8
jungshik at Google
Forgot to note that the CL was updated. Thanks, Brett, for the tip. Does this ...
6 years ago (2013-09-19 16:08:55 UTC) #9
jungshik at Google
https://codereview.chromium.org/23642003/diff/47001/url/url_canon_icu.cc File url/url_canon_icu.cc (right): https://codereview.chromium.org/23642003/diff/47001/url/url_canon_icu.cc#newcode12 url/url_canon_icu.cc:12: #include "base/synchronization/lock.h" oops. I'll remove this line in both ...
6 years ago (2013-09-19 16:09:40 UTC) #10
brettw
https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc File url/url_canon_icu.cc (right): https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc#newcode152 url/url_canon_icu.cc:152: DCHECK(uidna != NULL); Under what cases might we get ...
6 years ago (2013-09-19 17:44:44 UTC) #11
jungshik at Google
https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc File url/url_canon_icu.cc (right): https://codereview.chromium.org/23642003/diff/54001/url/url_canon_icu.cc#newcode152 url/url_canon_icu.cc:152: DCHECK(uidna != NULL); On 2013/09/19 17:44:45, brettw wrote: > ...
6 years ago (2013-09-19 19:16:16 UTC) #12
brettw
Sounds good. url/* LGTM
6 years ago (2013-09-19 20:05:44 UTC) #13
Ryan Sleevi
While I'm interested to learn more about IDN, I don't know if I'm qualified to ...
6 years ago (2013-09-19 20:21:01 UTC) #14
jungshik at Google
On 2013/09/19 20:21:01, Ryan Sleevi wrote: > While I'm interested to learn more about IDN, ...
6 years ago (2013-09-19 20:37:20 UTC) #15
Peter Kasting
https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newcode401 net/base/net_util.cc:401: struct uidna_wrapper { Struct names should be CamelCase. https://codereview.chromium.org/23642003/diff/54001/net/base/net_util.cc#newcode404 ...
6 years ago (2013-09-19 20:55:36 UTC) #16
jungshik at Google
Peter, can you take another look? Ryan, I dropped a ProcessIDN change from the CL. ...
6 years ago (2013-09-20 21:33:41 UTC) #17
Ryan Sleevi
On 2013/09/20 21:33:41, Jungshik Shin wrote: > Peter, can you take another look? > > ...
6 years ago (2013-09-20 22:10:16 UTC) #18
jungshik at Google
On 2013/09/20 22:10:16, Ryan Sleevi wrote: > On 2013/09/20 21:33:41, Jungshik Shin wrote: > > ...
6 years ago (2013-09-21 00:37:46 UTC) #19
Ryan Sleevi
https://chromiumcodereview.appspot.com/23642003/diff/82001/chrome/common/net/x509_certificate_model.cc File chrome/common/net/x509_certificate_model.cc (right): https://chromiumcodereview.appspot.com/23642003/diff/82001/chrome/common/net/x509_certificate_model.cc#newcode21 chrome/common/net/x509_certificate_model.cc:21: string16 output16 = IDNToUnicode(input, std::string()); IWYU: include net/base/net_util.h on ...
6 years ago (2013-09-21 00:46:13 UTC) #20
jungshik at Google
Thank you for looking at it. Updated. https://codereview.chromium.org/23642003/diff/82001/chrome/common/net/x509_certificate_model.cc File chrome/common/net/x509_certificate_model.cc (right): https://codereview.chromium.org/23642003/diff/82001/chrome/common/net/x509_certificate_model.cc#newcode21 chrome/common/net/x509_certificate_model.cc:21: string16 output16 ...
6 years ago (2013-09-23 17:54:53 UTC) #21
Peter Kasting
Code-wise, net/ LGTM, but I am not qualified to really determine whether this is the ...
6 years ago (2013-09-23 21:12:43 UTC) #22
jungshik at Google
Updated per Peter's comments. Ryan, can you take another look?
6 years ago (2013-09-24 17:47:55 UTC) #23
Ryan Sleevi
I'm not familiar enough with the IDNA aspect to know whether it's correct, but one ...
6 years ago (2013-09-24 23:21:24 UTC) #24
Ryan Sleevi
I'm not familiar enough with the IDNA aspect to know whether it's correct, but one ...
6 years ago (2013-09-24 23:21:27 UTC) #25
Peter Kasting
https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc#newcode467 net/base/net_util.cc:467: output_length, &info, &status); On 2013/09/24 23:21:24, Ryan Sleevi wrote: ...
6 years ago (2013-09-24 23:28:07 UTC) #26
Ryan Sleevi
net/ LGTM. https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/23642003/diff/97001/net/base/net_util.cc#newcode467 net/base/net_util.cc:467: output_length, &info, &status); On 2013/09/24 23:28:08, Peter ...
6 years ago (2013-09-24 23:53:56 UTC) #27
jungshik at Google
Thank you, Ryan, for reivew and Peter for answering his questions. I need chrome/* owner ...
6 years ago (2013-09-25 00:37:39 UTC) #28
jungshik at Google
Nico, would you approve for chrome/common?
6 years ago (2013-09-26 00:07:36 UTC) #29
Nico
lgtm
6 years ago (2013-09-26 00:09:56 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/23642003/97001
6 years ago (2013-09-26 01:11:25 UTC) #31
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
6 years ago (2013-09-26 05:41:38 UTC) #32
jungshik at Google
The component build failed due to the lack of an explicit dependency declaration for dynamic_annotation. ...
6 years ago (2013-09-26 20:02:35 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/23642003/130001
6 years ago (2013-09-27 07:05:09 UTC) #34
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=82205
6 years ago (2013-09-27 09:05:20 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/23642003/130001
6 years ago (2013-09-27 18:57:26 UTC) #36
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=82593
6 years ago (2013-09-27 22:57:10 UTC) #37
jungshik at Google
On 2013/09/27 22:57:10, I haz the power (commit-bot) wrote: > Retried try job too often ...
6 years ago (2013-09-28 00:09:51 UTC) #38
jungshik at Google
6 years ago (2013-09-29 10:52:53 UTC) #39
Message was sent while issue was closed.
Committed patchset #19 manually as r225878.

Powered by Google App Engine
This is Rietveld 408576698