Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637173004:
[Sync] Do not use base::RandBytesAsString to generate random string (Closed)
Description[Sync] Do not use base::RandBytesAsString to generate random string
Protobuf should use UTF-8 string, and base::RandBytesAsString won't generate ASCII/UTF-8 string.
BUG=682881
Review-Url: https://codereview.chromium.org/2637173004
Cr-Commit-Position: refs/heads/master@{#445065}
Committed: https://chromium.googlesource.com/chromium/src/+/20cbb6147f3978751c6cc69a9773dd3e93b2df56
Patch Set 1 #
Total comments: 2
Patch Set 2 : change the way to random string #Messages
Total messages: 28 (22 generated)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add field enable_features into EnableSyncClientToServerCompression BUG=667513 ========== to ========== Add field enable_features into EnableSyncClientToServerCompression BUG=667513 ==========
gangwu@chromium.org changed reviewers: + rkaplow@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add field enable_features into EnableSyncClientToServerCompression BUG=667513 ========== to ========== [Sync] Do not use base::RandBytesAsString to generate random string Should not use base::RandBytesAsString since it won't generate ASCII/UTF-8 string. BUG=682881 ==========
gangwu@chromium.org changed reviewers: + zea@chromium.org - rkaplow@chromium.org
Description was changed from ========== [Sync] Do not use base::RandBytesAsString to generate random string Should not use base::RandBytesAsString since it won't generate ASCII/UTF-8 string. BUG=682881 ========== to ========== [Sync] Do not use base::RandBytesAsString to generate random string Protobuf should use UTF-8 string, and base::RandBytesAsString won't generate ASCII/UTF-8 string. BUG=682881 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gangwu@chromium.org changed reviewers: + maxbogue@chromium.org - zea@chromium.org
PTAL
https://codereview.chromium.org/2637173004/diff/20001/components/sync/engine_... File components/sync/engine_impl/commit.cc (right): https://codereview.chromium.org/2637173004/diff/20001/components/sync/engine_... components/sync/engine_impl/commit.cc:40: std::string RandomString(int length) { I was poking around trying to understand RandInt and sizeof and found this: https://cs.chromium.org/chromium/src/components/history/core/browser/delete_d... What do you think of copying that approach instead of hard-coding kLegalCharacters? I think it's cleaner, though you should keep the .reserve(length) call yours has.
The CQ bit was checked by gangwu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
updated! https://codereview.chromium.org/2637173004/diff/20001/components/sync/engine_... File components/sync/engine_impl/commit.cc (right): https://codereview.chromium.org/2637173004/diff/20001/components/sync/engine_... components/sync/engine_impl/commit.cc:40: std::string RandomString(int length) { On 2017/01/20 01:24:56, maxbogue wrote: > I was poking around trying to understand RandInt and sizeof and found this: > > https://cs.chromium.org/chromium/src/components/history/core/browser/delete_d... > > What do you think of copying that approach instead of hard-coding > kLegalCharacters? I think it's cleaner, though you should keep the > .reserve(length) call yours has. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gangwu@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": 40001, "attempt_start_ts": 1484927197419160,
"parent_rev": "e96488a02be8b2a1722f0194474be7e0cc63c054", "commit_rev":
"20cbb6147f3978751c6cc69a9773dd3e93b2df56"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Do not use base::RandBytesAsString to generate random string Protobuf should use UTF-8 string, and base::RandBytesAsString won't generate ASCII/UTF-8 string. BUG=682881 ========== to ========== [Sync] Do not use base::RandBytesAsString to generate random string Protobuf should use UTF-8 string, and base::RandBytesAsString won't generate ASCII/UTF-8 string. BUG=682881 Review-Url: https://codereview.chromium.org/2637173004 Cr-Commit-Position: refs/heads/master@{#445065} Committed: https://chromium.googlesource.com/chromium/src/+/20cbb6147f3978751c6cc69a9773... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/20cbb6147f3978751c6cc69a9773... |
