Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816363002:
Reimplement ct_objects_extractor.cc without legacy ASN.1 code (Closed)
|
Created:
3 years, 8 months ago by davidben Modified:
3 years, 8 months ago Reviewers:
mattm CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri, net-reviews_chromium.org, martijn+crwatch_martijnc.be Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDecouple ct_objects_extractor.cc without legacy ASN.1 code
BUG=706445
Review-Url: https://codereview.chromium.org/2816363002
Cr-Commit-Position: refs/heads/master@{#466739}
Committed: https://chromium.googlesource.com/chromium/src/+/9587eaf061cc8429228e1e1c5e494e8bf983fc9f
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : use CHECKs #Patch Set 4 : appease MSVC #
Total comments: 8
Patch Set 5 : mattm comments #Patch Set 6 : rebase #Messages
Total messages: 28 (22 generated)
The CQ bit was checked by davidben@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...
The CQ bit was checked by davidben@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...
The CQ bit was checked by davidben@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 ========== Decouple ct_objects_extractor.cc from crypto/x509 and OID table. BUG=706445 ========== to ========== Decouple ct_objects_extractor.cc without legacy ASN.1 code BUG=706445 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) 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 davidben@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
davidben@chromium.org changed reviewers: + mattm@chromium.org
https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... File net/cert/ct_objects_extractor.cc (left): https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:187: // check to match the reference implementation. Should this comment be retained? https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... File net/cert/ct_objects_extractor.cc (right): https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:293: CBB tbs_cert_cbb; I found this function to be a bit hard to follow. Could the naming be clarified about which variables refer to the input cert and which refer to the tbscert that's being generated? https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:375: !SkipElements(&sequence, 1 /* responderStatus */) || responseStatus https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:406: !SkipElements(&response_data, 2 /* responseID, producedAt */) || responderID
The CQ bit was checked by davidben@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...
https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... File net/cert/ct_objects_extractor.cc (left): https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:187: // check to match the reference implementation. On 2017/04/18 03:07:56, mattm wrote: > Should this comment be retained? I'm not sure it's possible for it to matter, but I added an equivalent comment. https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... File net/cert/ct_objects_extractor.cc (right): https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:293: CBB tbs_cert_cbb; On 2017/04/18 03:07:56, mattm wrote: > I found this function to be a bit hard to follow. Could the naming be clarified > about which variables refer to the input cert and which refer to the tbscert > that's being generated? The ones that end in _cbb are the ones being generated. But I suppose that's a little obtuse. I switched the _cbb suffix with a new_ prefix. I also reordered things to avoid interleaving parsing and serializing. It is still convenient to switch back and forth a bit to avoid an extra copy of |tbs_cert| though. Is this better? https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:375: !SkipElements(&sequence, 1 /* responderStatus */) || On 2017/04/18 03:07:56, mattm wrote: > responseStatus Done. https://codereview.chromium.org/2816363002/diff/60001/net/cert/ct_objects_ext... net/cert/ct_objects_extractor.cc:406: !SkipElements(&response_data, 2 /* responseID, producedAt */) || On 2017/04/18 03:07:56, mattm wrote: > responderID Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
lgtm (sorry about the merge conflicts from https://codereview.chromium.org/2824983002/)
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org Link to the patchset: https://codereview.chromium.org/2816363002/#ps100001 (title: "rebase")
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": 100001, "attempt_start_ts": 1493060773756770,
"parent_rev": "7ddccb994a410a624be528ca2143c869afb67683", "commit_rev":
"9587eaf061cc8429228e1e1c5e494e8bf983fc9f"}
Message was sent while issue was closed.
Description was changed from ========== Decouple ct_objects_extractor.cc without legacy ASN.1 code BUG=706445 ========== to ========== Decouple ct_objects_extractor.cc without legacy ASN.1 code BUG=706445 Review-Url: https://codereview.chromium.org/2816363002 Cr-Commit-Position: refs/heads/master@{#466739} Committed: https://chromium.googlesource.com/chromium/src/+/9587eaf061cc8429228e1e1c5e49... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9587eaf061cc8429228e1e1c5e49... |
