Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871643005:
Disallow mixing of Canadian Syllabary and [a-z] (Closed)
|
Created:
3 years, 7 months ago by jungshik at Google Modified:
3 years, 7 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallow mixing of Canadian Syllabary and [a-z]
BUG=719199
TEST=components_unittests --gtest_filter=*IDNToUn*
Review-Url: https://codereview.chromium.org/2871643005
Cr-Commit-Position: refs/heads/master@{#471538}
Committed: https://chromium.googlesource.com/chromium/src/+/e2fde40094b4c9b56a7e6342ab1c8bbe75381761
Patch Set 1 #
Total comments: 3
Patch Set 2 : delete unnecesary check #Patch Set 3 : fix unittests #
Messages
Total messages: 17 (8 generated)
jshin@chromium.org changed reviewers: + mgiuca@chromium.org, pkasting@chromium.org
PTAL. Thanks
https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... components/url_formatter/url_formatter.cc:436: CHECK(U_SUCCESS(status)); This will be removed.
https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... components/url_formatter/url_formatter.cc:436: CHECK(U_SUCCESS(status)); On 2017/05/09 20:28:22, jungshik at Google wrote: > This will be removed. What does "This will be removed" mean? Before landing the CL? Add "// DO NOT SUBMIT" to have the CQ force you to remove it. After landing the CL? Add a TODO. Anyway shouldn't we be doing some sort of check that status is successful? Why remove it?
LGTM but I share Matt's question about the CHECK. https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... File components/url_formatter/url_formatter.cc (right): https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... components/url_formatter/url_formatter.cc:436: CHECK(U_SUCCESS(status)); On 2017/05/10 00:25:58, Matt Giuca wrote: > Anyway shouldn't we be doing some sort of check that status is successful? Why > remove it? CHECK is not really intended to "check whether something is successful". It's meant for only two purposes: (1) To assert that something is always true, and that if it were not true, we'd need to crash the process instantly to avoid introducing a fatal security hole. (2) To assert that something is supposed to always be true, but for some reason it seems to be violated in the field, and we're temporarily turning it into a crash to track it down. For assertions not known to be violated in the field, we generally want DCHECK instead. For things that are really conditions (and not assertions), we'd want to use conditionals. For things always true that we don't actually need to write an assertion about because they're obvious, we'd just leave them out.
On 2017/05/10 20:23:53, Peter Kasting wrote: > LGTM but I share Matt's question about the CHECK. > > https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... > File components/url_formatter/url_formatter.cc (right): > > https://codereview.chromium.org/2871643005/diff/1/components/url_formatter/ur... > components/url_formatter/url_formatter.cc:436: CHECK(U_SUCCESS(status)); > On 2017/05/10 00:25:58, Matt Giuca wrote: > > Anyway shouldn't we be doing some sort of check that status is successful? Why > > remove it? > > CHECK is not really intended to "check whether something is successful". It's > meant for only two purposes: > > (1) To assert that something is always true, and that if it were not true, we'd > need to crash the process instantly to avoid introducing a fatal security hole. > (2) To assert that something is supposed to always be true, but for some reason > it seems to be violated in the field, and we're temporarily turning it into a > crash to track it down. > > For assertions not known to be violated in the field, we generally want DCHECK > instead. For things that are really conditions (and not assertions), we'd want > to use conditionals. For things always true that we don't actually need to > write an assertion about because they're obvious, we'd just leave them out. That CHECK was added to make sure that I didn't mess up with the regex I modified during my code development. (I didn't mean to include it in the CL, but accidentally kept it in the CL uploaded). Now that I didn't mess up, it's not necessary in builds we ship to users. Sorry for the confusion and thanks for the review.
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/2871643005/#ps20001 (title: "delete unnecesary check")
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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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/2871643005/#ps40001 (title: "fix unittests")
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": 40001, "attempt_start_ts": 1494635631783430,
"parent_rev": "34f7ec8864cd317f0955b0ac279f54a62e47591f", "commit_rev":
"e2fde40094b4c9b56a7e6342ab1c8bbe75381761"}
Message was sent while issue was closed.
Description was changed from ========== Disallow mixing of Canadian Syllabary and [a-z] BUG=719199 TEST=components_unittests --gtest_filter=*IDNToUn* ========== to ========== Disallow mixing of Canadian Syllabary and [a-z] BUG=719199 TEST=components_unittests --gtest_filter=*IDNToUn* Review-Url: https://codereview.chromium.org/2871643005 Cr-Commit-Position: refs/heads/master@{#471538} Committed: https://chromium.googlesource.com/chromium/src/+/e2fde40094b4c9b56a7e6342ab1c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e2fde40094b4c9b56a7e6342ab1c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
