Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894313002:
Block Tifinagh + Latin mix (Closed)
|
Created:
3 years, 7 months ago by jungshik at Google Modified:
3 years, 7 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlock Tifinagh + Latin mix
BUG=chromium:722639
TEST=components_unittests --gtest_filter=*IDNToU*
Review-Url: https://codereview.chromium.org/2894313002
Cr-Commit-Position: refs/heads/master@{#474199}
Committed: https://chromium.googlesource.com/chromium/src/+/ae6f339fba0736224fdca0b96d2bb1cda42d8ad3
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebased #
Messages
Total messages: 22 (11 generated)
Description was changed from ========== Block Tifinagh + Latin mix BUG=chromium:722639 TEST=components_unittests --gtest_filter=*IDNToU* ========== to ========== Block Tifinagh + Latin mix Blocking any Tifinagh character from mixing with Latin is a bit too broad a brush for a single character of U+.... in the script confusable with lowercase L. BUG=chromium:722639 TEST=components_unittests --gtest_filter=*IDNToU* ==========
Description was changed from ========== Block Tifinagh + Latin mix Blocking any Tifinagh character from mixing with Latin is a bit too broad a brush for a single character of U+.... in the script confusable with lowercase L. BUG=chromium:722639 TEST=components_unittests --gtest_filter=*IDNToU* ========== to ========== Block Tifinagh + Latin mix BUG=chromium:722639 TEST=components_unittests --gtest_filter=*IDNToU* ==========
Blocking any Tifinagh characters from mixing with Latin is a bit too broad brush because there's only one character in the script that is confusable with Latin. However, it's done this way to facilitate merging to branches. A bit more complex regex (as used for o-like and g-like Armenian letters could have been used), but I chose a simpler one.
On 2017/05/22 06:18:44, jungshik at Google wrote: > Blocking any Tifinagh characters from mixing with Latin is a bit too broad brush > because there's only one character in the script that is confusable with Latin. > However, it's done this way to facilitate merging to branches. A bit more > complex regex (as used for o-like and g-like Armenian letters could have been > used), but I chose a simpler one. Not a single domain (out of ~ 1 million registered in .com as of March 2017) is affected by this change. I guess some domains (for testing) have registered since that would be filtered by this change.
jshin@chromium.org changed reviewers: + pkasting@chromium.org
PTAL
LGTM https://codereview.chromium.org/2894313002/diff/1/components/url_formatter/id... File components/url_formatter/idn_spoof_checker.cc (right): https://codereview.chromium.org/2894313002/diff/1/components/url_formatter/id... components/url_formatter/idn_spoof_checker.cc:170: // - Disalow mixing of Latin and Tifinagh. Are there other "Latin + <script>" combos we should pre-emptively block? https://codereview.chromium.org/2894313002/diff/1/components/url_formatter/id... components/url_formatter/idn_spoof_checker.cc:186: R"([\p{sc=tfng}].*[a-z]|[a-z].*[\p{sc=tfng}])", Nit: Feels like there's maybe a less redundant way to write this regex, but it's probably less readable.
On 2017/05/22 18:47:46, Peter Kasting wrote: > LGTM Thanks > > https://codereview.chromium.org/2894313002/diff/1/components/url_formatter/id... > File components/url_formatter/idn_spoof_checker.cc (right): > > https://codereview.chromium.org/2894313002/diff/1/components/url_formatter/id... > components/url_formatter/idn_spoof_checker.cc:170: // - Disalow mixing of Latin > and Tifinagh. > Are there other "Latin + <script>" combos we should pre-emptively block? With https://codereview.chromium.org/2897873002, the need for that became pretty small. (top-domain-look-alikes are blocked by that CL). Miao has one character that is confusable with one of [a-z]. We have blocked the worst confusables in Armenian ('g-like' and 'o-like') to be next to Latin for a couple of years. Armenian has others (rather distinct in most fonts), but they'd be blocked by https://codereview.chromium.org/2897873002 for top domains. For branches, I may as well block mixing of Latin and {Miao, Armenian} > https://codereview.chromium.org/2894313002/diff/1/components/url_formatter/id... > components/url_formatter/idn_spoof_checker.cc:186: > R"([\p{sc=tfng}].*[a-z]|[a-z].*[\p{sc=tfng}])", > Nit: Feels like there's maybe a less redundant way to write this regex, but it's > probably less readable. I couldn't come up with one. There may be. I'll think about it.
The CQ bit was checked by jshin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 Link to the patchset: https://codereview.chromium.org/2894313002/#ps20001 (title: "rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jshin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495607598432040,
"parent_rev": "a1db6b7d87922f09904037e82447170cc97c0a2e", "commit_rev":
"ae6f339fba0736224fdca0b96d2bb1cda42d8ad3"}
Message was sent while issue was closed.
Description was changed from ========== Block Tifinagh + Latin mix BUG=chromium:722639 TEST=components_unittests --gtest_filter=*IDNToU* ========== to ========== Block Tifinagh + Latin mix BUG=chromium:722639 TEST=components_unittests --gtest_filter=*IDNToU* Review-Url: https://codereview.chromium.org/2894313002 Cr-Commit-Position: refs/heads/master@{#474199} Committed: https://chromium.googlesource.com/chromium/src/+/ae6f339fba0736224fdca0b96d2b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ae6f339fba0736224fdca0b96d2b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
