Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
778543004:
Add sanity check to generateKey to make sure usages aren't empty (Closed)
DescriptionAdd sanity check to generateKey to make sure usages aren't empty
BUG=425645
Committed: https://crrev.com/3e2062be7c36cea30981cac6f681d5c6f030d1bd
Cr-Commit-Position: refs/heads/master@{#307133}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Messages
Total messages: 11 (2 generated)
nharper@chromium.org changed reviewers: + eroman@chromium.org
https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... File content/child/webcrypto/algorithm_dispatch.cc (right): https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:115: if (status.IsSuccess()) { style: Can you write this in the opposite direction: if (status.IsError()) return status; That is the style used through-out the rest of the code, and I find it is easier to follow the code flow when you use early-returns (fire and forget so to speak!) https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:116: const blink::WebCryptoKey* key = nullptr; Does this compile on all platforms? Not sure that nullptr can be used in Chromium yet, but I may be wrong (using NULL for the rest of webcrypto code). https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:127: << "Key usages for generateKey() must not be empty"; Great. Can you also return an ErrorUnexpected here? (DCHECK only triggers in developpers build, so the return would catch badness from propagating in the release builds). https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... File content/child/webcrypto/algorithm_dispatch.h (right): https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.h:12: #include "content/child/webcrypto/generate_key_result.h" Please move the header to the .cc file and leave this file unchanged. In Chromium we prefer to use forward declarations to reduce header bloat from slowing down compiles (headers in C++ are not very efficient)
https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... File content/child/webcrypto/algorithm_dispatch.cc (right): https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:126: DCHECK(key->usages() != 0) also: DCHECK_NE(0, key->usages());
https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... File content/child/webcrypto/algorithm_dispatch.cc (right): https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:115: if (status.IsSuccess()) { On 2014/12/05 19:41:55, eroman wrote: > style: Can you write this in the opposite direction: > > if (status.IsError()) > return status; > > That is the style used through-out the rest of the code, and I find it is easier > to follow the code flow when you use early-returns (fire and forget so to > speak!) Done. https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:116: const blink::WebCryptoKey* key = nullptr; On 2014/12/05 19:41:55, eroman wrote: > Does this compile on all platforms? Not sure that nullptr can be used in > Chromium yet, but I may be wrong (using NULL for the rest of webcrypto code). Codesearch shows nullptr used in thousands of places (some are comments) in chromium, including use in the quic code. If the style in the webcrypto code is to use NULL instead, I'm fine with that. https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:126: DCHECK(key->usages() != 0) On 2014/12/05 20:52:45, eroman wrote: > also: DCHECK_NE(0, key->usages()); Since I need to do other things when the DCHECK condition is met, I ended up changing it to if (key->usages() == 0) { DCHECK(false)... do other things } https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.cc:127: << "Key usages for generateKey() must not be empty"; On 2014/12/05 19:41:55, eroman wrote: > Great. Can you also return an ErrorUnexpected here? (DCHECK only triggers in > developpers build, so the return would catch badness from propagating in the > release builds). Done. https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... File content/child/webcrypto/algorithm_dispatch.h (right): https://codereview.chromium.org/778543004/diff/1/content/child/webcrypto/algo... content/child/webcrypto/algorithm_dispatch.h:12: #include "content/child/webcrypto/generate_key_result.h" On 2014/12/05 19:41:55, eroman wrote: > Please move the header to the .cc file and leave this file unchanged. > > In Chromium we prefer to use forward declarations to reduce header bloat from > slowing down compiles (headers in C++ are not very efficient) Done.
lgtm https://codereview.chromium.org/778543004/diff/20001/content/child/webcrypto/... File content/child/webcrypto/algorithm_dispatch.cc (right): https://codereview.chromium.org/778543004/diff/20001/content/child/webcrypto/... content/child/webcrypto/algorithm_dispatch.cc:132: // Even though this is unexpected, I chose not to use Remove this comment. "I" will become unclear over time. I also think the TODO is unnecessary. I think the comment above is sufficient.
https://codereview.chromium.org/778543004/diff/20001/content/child/webcrypto/... File content/child/webcrypto/algorithm_dispatch.cc (right): https://codereview.chromium.org/778543004/diff/20001/content/child/webcrypto/... content/child/webcrypto/algorithm_dispatch.cc:132: // Even though this is unexpected, I chose not to use On 2014/12/05 23:49:48, eroman wrote: > Remove this comment. "I" will become unclear over time. I also think the TODO is > unnecessary. > I think the comment above is sufficient. Done.
The CQ bit was checked by nharper@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/778543004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3e2062be7c36cea30981cac6f681d5c6f030d1bd Cr-Commit-Position: refs/heads/master@{#307133} |
