Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19757011:
WebCrypto: Implement digest() using NSS (Closed)
|
Created:
7 years, 5 months ago by Bryan Eyler Modified:
7 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWebCrypto: Implement digest() using NSS
BUG=245025
R=eroman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220470
Patch Set 1 #
Total comments: 28
Patch Set 2 : Initial implementation of SHA1 for NSS. #
Total comments: 30
Patch Set 3 : #
Total comments: 43
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 17
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Another attempt to get green try builds. #
Total comments: 2
Patch Set 9 : Remove overrides, add documentation to digest interface, and small fix to hash result length retrie… #
Total comments: 9
Patch Set 10 : More documentation of interfaces, use of scoped_ptr for better memory management. #
Total comments: 4
Patch Set 11 : Revert to normal pointer. #Patch Set 12 : Rebase: removal of partial processing. #
Total comments: 30
Patch Set 13 : #
Total comments: 1
Patch Set 14 : Fix gyp conditions and dependencies. #
Total comments: 2
Patch Set 15 : Fix leak of hash context. #
Total comments: 18
Patch Set 16 : Remove unnecessary voided out arguments. #Patch Set 17 : Mostly style fixes. #Patch Set 18 : Export WebCryptoImpl instead of getting an instance from WebKitPlatformSupportImpl. #
Total comments: 1
Patch Set 19 : Split out crypto functionality from abstract result class from Blink. #
Messages
Total messages: 76 (0 generated)
Great start! I am following-up with a better API for handling the errors, so hopefully you can clean up the error handling after that lands. I think we should create a new directory to contain these files, since I envision we will be adding a bunch of other files. Perhaps content/renderer/webcrypto/* Also small nit: The changelist description mentions "SHA1", however you actually have implemented all of the SHA* functions. (sweet) https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:21: WebCryptoSHADigest(const WebKit::WebCryptoAlgorithmId); nit: mark as "explicit" https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:23: virtual void process(const unsigned char*, size_t) OVERRIDE; nit: add the parameter names (blink style omits them, however chromium includes them) https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:27: virtual void finish(WebKit::WebCryptoOperationResult*) OVERRIDE; Add the parameter name https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:42: #endif // CONTENT_RENDERER_WEBCRYPTO_IMPL_H_ Update this comment https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_nss.cc:20: crypto::EnsureNSSInit(); Seems this could go in InitializeContext() instead. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_nss.cc:69: if (!context_ && !InitializeContext()) { [after my API change] I suggest doing the InitializeContext() at the start (WebCryptoImpl::digest), and signalling the failure using result->initializationFailed() https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_nss.cc:113: length = 0; [optional] Rather than running the risk of the two switches getting out of sync, I suggest adding the length codings to the earlier switch statement on |algorithm_id|. You could then save it to a member variable, or create the WebArrayBuffer at that point. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:13: const int kNumAlgorithms = 5; Instead of specifying this, let the compiler figure it out: WebKit::WebCryptoAlgorithmId algorithm_ids[] = { ... }; Then when you need to know the length: arraysize(algorithm_ids); defined in base/basictypes.h https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:27: array_buffer_data_hex_ = new char[array_buffer.byteLength() * 2 + 1]; Where is this freed? Try using a std::vector<> instead. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:32: for (unsigned i = 0; i < array_buffer.byteLength(); i++) { use HexEncode() from base/strings/string_number_conversions.h (the only difference is it uses uppercase, whereas you are using lowercase). https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:58: const char* hex_result[kNumAlgorithms] = { easier to let the compiler figure it out. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:78: new TestWebCryptoOperationResult(); I believe this is leaked. Would be better to stack allocate it. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:94: const char* hex_result[kNumAlgorithms] = { easier to let the compiler figure it out. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:130: const int kNumInputSets = 6; easier to let the compiler figure it out. However in this case, because you are using an unnamed struct, you will probably have to use: ARRAYSIZE_UNSAFE()
BTW, I sent this changelist to the try jobs: you can review the errors by clicking on the boxes underneath patchset 1. (compile error on android for instance)
The new interface for errors is now checked in. Please sync your tree and switch to using digest2.
https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:21: WebCryptoSHADigest(const WebKit::WebCryptoAlgorithmId); On 2013/07/19 01:11:13, eroman wrote: > nit: mark as "explicit" Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:23: virtual void process(const unsigned char*, size_t) OVERRIDE; On 2013/07/19 01:11:13, eroman wrote: > nit: add the parameter names (blink style omits them, however chromium includes > them) Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:27: virtual void finish(WebKit::WebCryptoOperationResult*) OVERRIDE; On 2013/07/19 01:11:13, eroman wrote: > Add the parameter name Removed param as part of new interface. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest.h:42: #endif // CONTENT_RENDERER_WEBCRYPTO_IMPL_H_ On 2013/07/19 01:11:13, eroman wrote: > Update this comment Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_nss.cc:20: crypto::EnsureNSSInit(); On 2013/07/19 01:11:13, eroman wrote: > Seems this could go in InitializeContext() instead. Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_nss.cc:69: if (!context_ && !InitializeContext()) { On 2013/07/19 01:11:13, eroman wrote: > [after my API change] > > I suggest doing the InitializeContext() at the start (WebCryptoImpl::digest), > and signalling the failure using result->initializationFailed() Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_nss.cc:113: length = 0; On 2013/07/19 01:11:13, eroman wrote: > [optional] Rather than running the risk of the two switches getting out of sync, > I suggest adding the length codings to the earlier switch statement on > |algorithm_id|. You could then save it to a member variable, or create the > WebArrayBuffer at that point. Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:13: const int kNumAlgorithms = 5; On 2013/07/19 01:11:13, eroman wrote: > Instead of specifying this, let the compiler figure it out: > > WebKit::WebCryptoAlgorithmId algorithm_ids[] = { > ... > }; > > Then when you need to know the length: > > arraysize(algorithm_ids); > > defined in base/basictypes.h Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:27: array_buffer_data_hex_ = new char[array_buffer.byteLength() * 2 + 1]; On 2013/07/19 01:11:13, eroman wrote: > Where is this freed? > > Try using a std::vector<> instead. Using std::string instead since that's the return type from HexEncode and I want to do a EXPECT_STRCASEEQ() on the result. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:32: for (unsigned i = 0; i < array_buffer.byteLength(); i++) { On 2013/07/19 01:11:13, eroman wrote: > use HexEncode() from base/strings/string_number_conversions.h (the only > difference is it uses uppercase, whereas you are using lowercase). Done. I do a case-insensitive compare anyway. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:58: const char* hex_result[kNumAlgorithms] = { On 2013/07/19 01:11:13, eroman wrote: > easier to let the compiler figure it out. I made this, and future ones, refer back to arraysize(algorithm_ids) to help ensure that the sizes match. Should I just let the compiler figure this out instead? https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:78: new TestWebCryptoOperationResult(); On 2013/07/19 01:11:13, eroman wrote: > I believe this is leaked. > Would be better to stack allocate it. Done. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:94: const char* hex_result[kNumAlgorithms] = { On 2013/07/19 01:11:13, eroman wrote: > easier to let the compiler figure it out. See above. https://codereview.chromium.org/19757011/diff/1/content/renderer/webcrypto_sh... content/renderer/webcrypto_sha_digest_unittest.cc:130: const int kNumInputSets = 6; On 2013/07/19 01:11:13, eroman wrote: > easier to let the compiler figure it out. > > However in this case, because you are using an unnamed struct, you will probably > have to use: > > ARRAYSIZE_UNSAFE() > Done. Seems to be fine with just arraysize()?
https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:28: case WebKit::WebCryptoAlgorithmIdSha1: { This is a really weird place for an opening bracket. Are you sure you didn't mean it to go after the Sha512? https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:37: result->initializationFailed(); This is a leak of |operation|. A few alternatives: (1) scoped_ptr<....> operation(new WebCryptoSHA...); if (!operation->Initialize()) { result->initializationFailed(); return; } result->initializationSucceeded(operation.release()); return; (2) Use a factory method: WebCryptoSHADigest* operation = WebCryptoSHADigest::create(algorithm.id(), result); if (operation) { result->initializationSucceeded(operation); } else { result->initializationFailed(); } https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:45: //result->initializationSucceeded(new DummyOperation(result)); Delete this commented-out code https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:51: //result->initializationSucceeded(new DummyOperation(result)); Delete this commented out code https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest.h:26: virtual void process(const unsigned char* bytes, size_t size) OVERRIDE; nit: I would remove the newlines between process/abort/finish https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:45: hash_algorithm_ = SEC_OID_UNKNOWN; This should be unreachable code, put a NOTREACHED(). https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:63: // TODO(bryaneyler): Error out. isn't this handled now? https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:69: // TODO(bryaneyler): Error out. Isn't this handled now? https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:79: LOG(ERROR) << "No valid context initialized."; This shouldn't be reachable, since failure to create a context means that process() will never get called right? In that case use an assertion instead. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:80: // TODO(bryaneyler): Error out. result_->completeWithError(); https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:86: // TODO(bryaneyler): Error out. result_->completeWithError(); https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:96: if (!context_) { This should be an assertion instead https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:110: // TODO(bryaneyler): Error out. result_->completeWithError(); ... however I don't believe this is reachable. allocations crash in Chrome on failure, for security reasons https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:118: // TODO(bryaneyler): Error out. result_->completeWithError(); https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:26: : was_error_encountered_(false) { indentation. also remove the "was_"
Bryan, do you have time to finish this change? If not, I am happy to take it over, or otherwise help out!
On 2013/07/30 19:46:19, eroman wrote: > Bryan, do you have time to finish this change? > > If not, I am happy to take it over, or otherwise help out! Yes, sorry, it was a busy week last week. Should have fixes out today or tomorrow.
Great! I also noticed that it isn't possible to create a WebKit::WebCryptoAlgorithm from the chromium side. I will add that functionality to the blink side today. Since I think the testing should use the same interface as blink. i.e. Create a WebCrypto* implementation, and then call its methods like digest (passing in WebCryptoAlgorithm).
https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:28: case WebKit::WebCryptoAlgorithmIdSha1: { On 2013/07/24 01:33:50, eroman wrote: > This is a really weird place for an opening bracket. Are you sure you didn't > mean it to go after the Sha512? Thanks; legacy from some testing I was doing to get the lack of SHA256 working with the Blink tests. Moved to correct location. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:37: result->initializationFailed(); On 2013/07/24 01:33:50, eroman wrote: > This is a leak of |operation|. > > A few alternatives: > > (1) > > scoped_ptr<....> operation(new WebCryptoSHA...); > if (!operation->Initialize()) { > result->initializationFailed(); > return; > } > > result->initializationSucceeded(operation.release()); > return; > > (2) Use a factory method: > > WebCryptoSHADigest* operation = WebCryptoSHADigest::create(algorithm.id(), > result); > > if (operation) { > result->initializationSucceeded(operation); > } else { > result->initializationFailed(); > } Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:45: //result->initializationSucceeded(new DummyOperation(result)); On 2013/07/24 01:33:50, eroman wrote: > Delete this commented-out code Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:51: //result->initializationSucceeded(new DummyOperation(result)); On 2013/07/24 01:33:50, eroman wrote: > Delete this commented out code Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest.h:26: virtual void process(const unsigned char* bytes, size_t size) OVERRIDE; On 2013/07/24 01:33:50, eroman wrote: > nit: I would remove the newlines between process/abort/finish Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:45: hash_algorithm_ = SEC_OID_UNKNOWN; On 2013/07/24 01:33:50, eroman wrote: > This should be unreachable code, put a NOTREACHED(). Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:63: // TODO(bryaneyler): Error out. On 2013/07/24 01:33:50, eroman wrote: > isn't this handled now? Yes, nothing left to do. Removed comment. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:69: // TODO(bryaneyler): Error out. On 2013/07/24 01:33:50, eroman wrote: > Isn't this handled now? Same. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:79: LOG(ERROR) << "No valid context initialized."; On 2013/07/24 01:33:50, eroman wrote: > This shouldn't be reachable, since failure to create a context means that > process() will never get called right? > > In that case use an assertion instead. Changed to DCHECK. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:80: // TODO(bryaneyler): Error out. On 2013/07/24 01:33:50, eroman wrote: > result_->completeWithError(); Removed. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:86: // TODO(bryaneyler): Error out. On 2013/07/24 01:33:50, eroman wrote: > result_->completeWithError(); Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:96: if (!context_) { On 2013/07/24 01:33:50, eroman wrote: > This should be an assertion instead Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:110: // TODO(bryaneyler): Error out. On 2013/07/24 01:33:50, eroman wrote: > result_->completeWithError(); > > ... however I don't believe this is reachable. allocations crash in Chrome on > failure, for security reasons Should this be a DCHECK then? https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:118: // TODO(bryaneyler): Error out. On 2013/07/24 01:33:50, eroman wrote: > result_->completeWithError(); Done. https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/10001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:26: : was_error_encountered_(false) { On 2013/07/24 01:33:50, eroman wrote: > indentation. also remove the "was_" Done.
LG after these comments. @rsleevi: can you take a look at the use of NSS? Afterwards, we will need to add an OWNER for content/renderer/ https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest.h:32: WebKit::WebCryptoOperationResult& result_; This needs to be a copy, otherwise it can end up being be a use-after-free. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:78: if (PK11_DigestOp(context_, bytes, size) != SECSuccess) { Is it safe to call this when |bytes| is garbage, but size == 0? https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:80: result_.completeWithError(); delete this; (Any time completing the operation, it must be deleted). I am playing with some other models to maybe simplify this in the future, but the current API leaves memory management of the operation in the hands of the embedder. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:103: result_.completeWithError(); delete this; https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:24: class TestWebCryptoOperationResult : style nits: move the colon to the next line. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:25: public WebKit::WebCryptoOperationResultPrivate, indent by 4 https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:98: WebKit::WebCryptoOperationResult result(result_impl.get()); apologies for this complicated API https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:102: ASSERT_TRUE(digest); remove this as it can never trigger in Chrome (allocator enforces no NULL returns, and tons of code depends on that) https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:112: EXPECT_FALSE(result_impl->error_encountered()); [optional]: I suggest doing this test first. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:143: new WebCryptoSHADigest(algorithm_ids[id_index], result); Once https://codereview.chromium.org/21185005/ lands, I suggest not allocating the WebCryptoSHADigest directly, but rather: WebKit::WebCryptoAlgorithm algorithm = WebKit::WebCryptoAlgorithm::adoptParamsAndCreate(algorithm_ids[id_index], "", NULL); crypto->digest(algorithm, result); https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:175: // echo -n -e "\000\001\002\003\004\005" | sha1sum These comments are useful, thanks https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:213: } Another test which I think is worth adding, is what happens when you call process(NULL, 0) a few times. It should be a no-op since no data was consumed.
https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:29: new WebCryptoSHADigest(algorithm.id(), result)); If you're using the HASH model from NSS (or the EVP model from OpenSSL), you'll actually likely end up with only a single class for all the digesting operations. Just FWIW. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:31: if(!operation->Initialize()) { style: if (!operation->Initialize()) { https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest.h:12: #endif nit: Put this header *below* the other headers See http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code However, see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimiz... - it would be better to forward declare PK11Context https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:15: I'm a bit mixed on using the PK11 functions directly. NSS offers a high-level hash interface that hides the PKCS#11 details - namely http://mxr.mozilla.org/nss/source/lib/cryptohi/sechash.h - that more closely matches the OpenSSL interface. However, it means an extra level of function pointer indirection during the call sequencing, so it may not be worth it. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:26: hash_result_length_ = 20; If you keep with the OID formation, all of these constants should be replaced with their NSS header equivalents SHA1_LENGTH SHA224_LENGTH SHA256_LENGTH SHA384_LENGTH SHA512_LENGTH These all come from http://mxr.mozilla.org/nss/source/lib/util/hasht.h#35 https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:63: << hash_algorithm_; Seems that this is unused beyond the constructor - there's no need to store this in a member field (thus there's no reason to need the NSS header in this files header) https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:78: if (PK11_DigestOp(context_, bytes, size) != SECSuccess) { On 2013/07/31 02:23:50, eroman wrote: > Is it safe to call this when |bytes| is garbage, but size == 0? It should be. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:91: unsigned char* digest = NULL; Move this to line 96 (keep declaration and definition close) https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:94: WebKit::WebArrayBuffer::create(hash_result_length_, 1)); Same here - hash_result_length_ is unused outside of this function. It would be better to just use a TU-local function (eg: unnamed namespace) to look up the result length. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:101: != SECSuccess || result_length != hash_result_length_) { Seems like this should match Chromium style (the != would be on the previous line) clang-format to the rescue?
https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:94: WebKit::WebArrayBuffer::create(hash_result_length_, 1)); On 2013/07/31 18:19:19, Ryan Sleevi wrote: > Same here - hash_result_length_ is unused outside of this function. It would be > better to just use a TU-local function (eg: unnamed namespace) to look up the > result length. This was based on my recommendation, so that he have just 1 switch statement during initialization, rather than 2. Initially Bryan had a separate switch to lookup the result length. I am OK with either approach, just feel bad if Bryan is being pulled in opposing directions :)
On 2013/07/31 21:10:35, eroman wrote: > https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... > File content/renderer/webcrypto_sha_digest_nss.cc (right): > > https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... > content/renderer/webcrypto_sha_digest_nss.cc:94: > WebKit::WebArrayBuffer::create(hash_result_length_, 1)); > On 2013/07/31 18:19:19, Ryan Sleevi wrote: > > Same here - hash_result_length_ is unused outside of this function. It would > be > > better to just use a TU-local function (eg: unnamed namespace) to look up the > > result length. > > This was based on my recommendation, so that he have just 1 switch statement > during initialization, rather than 2. Initially Bryan had a separate switch to > lookup the result length. > > I am OK with either approach, just feel bad if Bryan is being pulled in opposing > directions :) I think we should try to minimize the size of these objects as much as possible, just given general web platform.
I'm still trying to figure out the difference between the git cl try results and my local runs, so I think another CL may eventually be coming as well. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:29: new WebCryptoSHADigest(algorithm.id(), result)); On 2013/07/31 18:19:19, Ryan Sleevi wrote: > If you're using the HASH model from NSS (or the EVP model from OpenSSL), you'll > actually likely end up with only a single class for all the digesting > operations. > > Just FWIW. Should this just be WebCryptoDigest then? https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:31: if(!operation->Initialize()) { On 2013/07/31 18:19:19, Ryan Sleevi wrote: > style: if (!operation->Initialize()) { Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest.h (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest.h:12: #endif On 2013/07/31 18:19:19, Ryan Sleevi wrote: > nit: Put this header *below* the other headers > > See http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code > > However, see > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimiz... > - it would be better to forward declare PK11Context Changed to forward declaration. It's not a very pretty declaration though... https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest.h:32: WebKit::WebCryptoOperationResult& result_; On 2013/07/31 02:23:50, eroman wrote: > This needs to be a copy, otherwise it can end up being be a use-after-free. Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:15: On 2013/07/31 18:19:19, Ryan Sleevi wrote: > I'm a bit mixed on using the PK11 functions directly. > > NSS offers a high-level hash interface that hides the PKCS#11 details - namely > http://mxr.mozilla.org/nss/source/lib/cryptohi/sechash.h - that more closely > matches the OpenSSL interface. > > However, it means an extra level of function pointer indirection during the call > sequencing, so it may not be worth it. I didn't know about this interface. Looks a bit cleaner to me. Changed to this interface. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:26: hash_result_length_ = 20; On 2013/07/31 18:19:19, Ryan Sleevi wrote: > If you keep with the OID formation, all of these constants should be replaced > with their NSS header equivalents > > SHA1_LENGTH > SHA224_LENGTH > SHA256_LENGTH > SHA384_LENGTH > SHA512_LENGTH > > These all come from http://mxr.mozilla.org/nss/source/lib/util/hasht.h#35 Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:63: << hash_algorithm_; On 2013/07/31 18:19:19, Ryan Sleevi wrote: > Seems that this is unused beyond the constructor - there's no need to store this > in a member field (thus there's no reason to need the NSS header in this files > header) Make sense. I want to keep the creation of the context within the Initialize() so that the error can be propagated on failure to create, so I've made the WebCryptoAlgorithm a parameter to Initialize() instead of the constructor. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:78: if (PK11_DigestOp(context_, bytes, size) != SECSuccess) { On 2013/07/31 18:19:19, Ryan Sleevi wrote: > On 2013/07/31 02:23:50, eroman wrote: > > Is it safe to call this when |bytes| is garbage, but size == 0? > > It should be. Test added for this. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:80: result_.completeWithError(); On 2013/07/31 02:23:50, eroman wrote: > delete this; > > (Any time completing the operation, it must be deleted). > > I am playing with some other models to maybe simplify this in the future, but > the current API leaves memory management of the operation in the hands of the > embedder. Obsolete now. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:91: unsigned char* digest = NULL; On 2013/07/31 18:19:19, Ryan Sleevi wrote: > Move this to line 96 (keep declaration and definition close) Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:101: != SECSuccess || result_length != hash_result_length_) { On 2013/07/31 18:19:19, Ryan Sleevi wrote: > Seems like this should match Chromium style (the != would be on the previous > line) > > clang-format to the rescue? Obsolete after moving to new interface. Will look into the clang-format tool. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:103: result_.completeWithError(); On 2013/07/31 02:23:50, eroman wrote: > delete this; Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:24: class TestWebCryptoOperationResult : On 2013/07/31 02:23:50, eroman wrote: > style nits: move the colon to the next line. Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:25: public WebKit::WebCryptoOperationResultPrivate, On 2013/07/31 02:23:50, eroman wrote: > indent by 4 Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:102: ASSERT_TRUE(digest); On 2013/07/31 02:23:50, eroman wrote: > remove this as it can never trigger in Chrome (allocator enforces no NULL > returns, and tons of code depends on that) Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:112: EXPECT_FALSE(result_impl->error_encountered()); On 2013/07/31 02:23:50, eroman wrote: > [optional]: I suggest doing this test first. Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:143: new WebCryptoSHADigest(algorithm_ids[id_index], result); On 2013/07/31 02:23:50, eroman wrote: > Once https://codereview.chromium.org/21185005/ lands, I suggest not allocating > the WebCryptoSHADigest directly, but rather: > > WebKit::WebCryptoAlgorithm algorithm = > WebKit::WebCryptoAlgorithm::adoptParamsAndCreate(algorithm_ids[id_index], "", > NULL); > > crypto->digest(algorithm, result); Done. https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:213: } On 2013/07/31 02:23:50, eroman wrote: > Another test which I think is worth adding, is what happens when you call > process(NULL, 0) a few times. It should be a no-op since no data was consumed. Added this into the empty set test.
LGTM (can you also update the changelist description since this is more than just SHA1?) https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:29: new WebCryptoSHADigest(algorithm.id(), result)); On 2013/08/02 00:49:05, Bryan Eyler wrote: > On 2013/07/31 18:19:19, Ryan Sleevi wrote: > > If you're using the HASH model from NSS (or the EVP model from OpenSSL), > you'll > > actually likely end up with only a single class for all the digesting > > operations. > > > > Just FWIW. > > Should this just be WebCryptoDigest then? Yes please https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:17: WebKit::WebCryptoOperationResult& result) Can you make this parameter const? https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:37: } Can you add NOTREACHED() ?
By the way, the compile error is because you need to use ARRAYSIZE_UNSAFE() instead of arraysize() when operating on unnamed structs.
Heads up: next time you sync you will have to add an extra method for the unittest interface (completeWithBoolean()): https://src.chromium.org/viewvc/blink/trunk/public/platform/WebCrypto.h?r1=15...
I updated the description a few patch sets ago, so not sure why that's not propagating here. Is there something I need to do in addition to uploading the CL? I was trying to figure out why my setup isn't throwing the arraysize() error, to hopefully avoid this in the future, but I'll defer that to another time and just fix this issue here and now. Also synced to latest to get the completeWithBoolean(). https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/24001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:29: new WebCryptoSHADigest(algorithm.id(), result)); On 2013/08/02 01:42:58, eroman wrote: > On 2013/08/02 00:49:05, Bryan Eyler wrote: > > On 2013/07/31 18:19:19, Ryan Sleevi wrote: > > > If you're using the HASH model from NSS (or the EVP model from OpenSSL), > > you'll > > > actually likely end up with only a single class for all the digesting > > > operations. > > > > > > Just FWIW. > > > > Should this just be WebCryptoDigest then? > > Yes please Done. https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_nss.cc:17: WebKit::WebCryptoOperationResult& result) On 2013/08/02 01:42:58, eroman wrote: > Can you make this parameter const? Done. https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... File content/renderer/webcrypto_sha_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/39001/content/renderer/webcrypt... content/renderer/webcrypto_sha_digest_unittest.cc:37: } On 2013/08/02 01:42:58, eroman wrote: > Can you add NOTREACHED() ? Actually, I think this is more something that could be reached and that we need to check the error on. I originally didn't check the initialization because of the interface I was using for testing, but after moving to the WebCryptoImpl interface, now this could be reached. I added error checking in the tests as well to make sure that the initialization didn't fail.
After the initial upload, the changelist description must be edited from the web interface. Click on "Edit Issue", it will take you here: https://codereview.chromium.org/19757011/edit Now modify the text and update.
On 2013/08/02 21:24:03, eroman wrote: > After the initial upload, the changelist description must be edited from the web > interface. > > Click on "Edit Issue", it will take you here: > https://codereview.chromium.org/19757011/edit > > Now modify the text and update. Ah, got it. Now just working through these try errors...
LGTM. Once sleevi signs off, add jamesr as a reviewer for content/renderer approval https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:54: hash_result_length_ = SHA512_LENGTH; I think the only member variable you need is the HASHContext*, since you can retrieve the result length during finish() with: unsigned result_length = HASH_ResultLen(HASH_GetType(context_)); https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:57: NOTREACHED(); Can you also add a defensive "return false;" here? https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:35: // There's no need to implement the initialization* routines because the Perhaps this comment is no longer relevant.
https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:54: hash_result_length_ = SHA512_LENGTH; On 2013/08/02 21:41:39, eroman wrote: > I think the only member variable you need is the HASHContext*, since you can > retrieve the result length during finish() with: > > unsigned result_length = HASH_ResultLen(HASH_GetType(context_)); +1 https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:57: NOTREACHED(); On 2013/08/02 21:41:39, eroman wrote: > Can you also add a defensive "return false;" here? +1. This would be a BUG to let it fall down into the AlgNULL case. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:23: WebKit::WebCryptoAlgorithmIdSha512 style nit: four spaces https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:91: TEST(WebCryptoDigest, SampleEmptySet) { As we start implementing layers, I'm a little nervous about possible duplication of tests. WebCrypto itself will definitely need a series of known answer tests / test vectors - eg: the NIST Sample Test Vectors for Byte-Oriented messages ( http://csrc.nist.gov/groups/STM/cavp/index.html#03 ). So what are we trying to test here that those tests would not test? Or what value do these tests bring in particular? Are we testing implementation? Interface? what? https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:7: // TODO(bryaneyler): Also include these in OpenSSL build. nit: Add a BUG # for each TODO - could just be a meta-bug "Implement WebCrypto for OpenSSL builds" or something. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:20: // TODO(bryaneyler): Also include these in OpenSSL build. same
https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:91: TEST(WebCryptoDigest, SampleEmptySet) { On 2013/08/02 23:29:27, Ryan Sleevi wrote: > As we start implementing layers, I'm a little nervous about possible duplication > of tests. > > WebCrypto itself will definitely need a series of known answer tests / test > vectors - eg: the NIST Sample Test Vectors for Byte-Oriented messages ( > http://csrc.nist.gov/groups/STM/cavp/index.html#03 ). > > So what are we trying to test here that those tests would not test? Or what > value do these tests bring in particular? > > Are we testing implementation? Interface? what? tl;dr: Let's keep these unittests here. I asked Bryan for these tests, so I can explain the reasoning. The idea is to exercise the chromium side code, and do some basic sanity checks that it is doing the right cryptographic operation. This gives code coverage to make sure there aren't any egregious use after frees, leaks, or crashes. Extensive layout tests may be the answer later down the road once there is a conformance suite for WebCrypto that can be used. However in the inter-rim, LayoutTests are very kludgy while trying to glue pieces together. I tried going that route initially but it proved frustrating: * The tests live in blink and hence can't be committed atomically with the platform implementation added in Chrome (leading to ugliness) * Require larger changes in order to have it be testable via a LayoutTest (again having to go back and forth with blink). For instance both the NSS and OpenSSL versions of digest would need to be committed before it can be tested via a layout test, or need to mark the tests disabled on some platforms but not others, losing coverage on them. * Quite a bit harder to debug layout tests over unit tests. For these reasons, I have adopted a testing strategy which isolates Blink and Chromium sides, so it is easier to develop them in isolation, and more parallelism. Blink is using a mock Platform layer for WebCrypto, allowing it to exercise the embedder API, and stress all of the error cases (which would otherwise be difficult or impossible to do with the real implementation). And the content/renderer side tests itself with unittest. Once there is a critical mass of stuff working, the LayoutTests can start using the real implementation rather than mock. (Although I could also see the argument to sticking with the mock implementation to allow testing more edge cases).
https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:54: hash_result_length_ = SHA512_LENGTH; On 2013/08/02 21:41:39, eroman wrote: > I think the only member variable you need is the HASHContext*, since you can > retrieve the result length during finish() with: > > unsigned result_length = HASH_ResultLen(HASH_GetType(context_)); Done. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:57: NOTREACHED(); On 2013/08/02 21:41:39, eroman wrote: > Can you also add a defensive "return false;" here? Done. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:23: WebKit::WebCryptoAlgorithmIdSha512 On 2013/08/02 23:29:27, Ryan Sleevi wrote: > style nit: four spaces Done. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:35: // There's no need to implement the initialization* routines because the On 2013/08/02 21:41:39, eroman wrote: > Perhaps this comment is no longer relevant. Removed. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_digest_unittest.cc:91: TEST(WebCryptoDigest, SampleEmptySet) { On 2013/08/02 23:29:27, Ryan Sleevi wrote: > As we start implementing layers, I'm a little nervous about possible duplication > of tests. > > WebCrypto itself will definitely need a series of known answer tests / test > vectors - eg: the NIST Sample Test Vectors for Byte-Oriented messages ( > http://csrc.nist.gov/groups/STM/cavp/index.html#03 ). > > So what are we trying to test here that those tests would not test? Or what > value do these tests bring in particular? > > Are we testing implementation? Interface? what? This certainly could be changed to use the NIST sample test vectors. While that set intends to ensure correctness, I was trying to ensure that the interface would perform properly here under the various use conditions. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:7: // TODO(bryaneyler): Also include these in OpenSSL build. On 2013/08/02 23:29:27, Ryan Sleevi wrote: > nit: Add a BUG # for each TODO - could just be a meta-bug "Implement WebCrypto > for OpenSSL builds" or something. Done. https://codereview.chromium.org/19757011/diff/51001/content/renderer/webcrypt... content/renderer/webcrypto_impl.cc:20: // TODO(bryaneyler): Also include these in OpenSSL build. On 2013/08/02 23:29:27, Ryan Sleevi wrote: > same Done.
NSS lgtm
On 2013/08/03 00:32:57, Ryan Sleevi wrote: > NSS lgtm Thanks all! Will send to jamesr once I figure out these try failures.
The build failures seem to be from linking (not finding the functions you added), which suggests the conditional GYP settings to include the .cc are not working. Also please improve the changelist description. Right now it is "Initial implementation of SHA for NSS.": - Remove "initial" - Mention that it is for WebCrypto (otherwise it sounds like you are adding the functionality to NSS). Perhaps something like: "WebCrypto: Implement digest() using NSS" would be more concise.
On 2013/08/03 20:06:36, eroman wrote: > The build failures seem to be from linking (not finding the functions you > added), which suggests the conditional GYP settings to include the .cc are not > working. > > Also please improve the changelist description. Right now it is "Initial > implementation of SHA for NSS.": > > - Remove "initial" > - Mention that it is for WebCrypto (otherwise it sounds like you are adding > the functionality to NSS). > > Perhaps something like: > > "WebCrypto: Implement digest() using NSS" would be more concise. Just want to put an update here: I've figured out that the symbols aren't being exported and that the builders must be doing a shared build (http://www.chromium.org/developers/how-tos/component-build). Still have not encountered the [original] error locally by building a shared build, which I think might help to resolve the Windows build errors.
On 2013/08/06 21:11:04, Bryan Eyler wrote: > On 2013/08/03 20:06:36, eroman wrote: > > The build failures seem to be from linking (not finding the functions you > > added), which suggests the conditional GYP settings to include the .cc are not > > working. > > > > Also please improve the changelist description. Right now it is "Initial > > implementation of SHA for NSS.": > > > > - Remove "initial" > > - Mention that it is for WebCrypto (otherwise it sounds like you are adding > > the functionality to NSS). > > > > Perhaps something like: > > > > "WebCrypto: Implement digest() using NSS" would be more concise. > > Just want to put an update here: I've figured out that the symbols aren't being > exported and that the builders must be doing a shared build > (http://www.chromium.org/developers/how-tos/component-build). Still have not > encountered the [original] error locally by building a shared build, which I > think might help to resolve the Windows build errors. Yes, the builders do shared builds. I'm guessing you'r not testing on Windows, as this is an MSVC error. https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... Please read the caveats of the associated MSDN article and ensure it's safe. Alternatively, WebKit::WebCrypto may need to be WEBKIT_EXPORT, but someone like abarth@ should chime in there. Or don't make the implementation a CONTENT_EXPORT. Given that you're not affecting the CONTENT public API at all, I'm not sure why you're adding CONTENT_EXPORT.
On 2013/08/06 21:17:06, Ryan Sleevi wrote: > On 2013/08/06 21:11:04, Bryan Eyler wrote: > > On 2013/08/03 20:06:36, eroman wrote: > > > The build failures seem to be from linking (not finding the functions you > > > added), which suggests the conditional GYP settings to include the .cc are > not > > > working. > > > > > > Also please improve the changelist description. Right now it is "Initial > > > implementation of SHA for NSS.": > > > > > > - Remove "initial" > > > - Mention that it is for WebCrypto (otherwise it sounds like you are > adding > > > the functionality to NSS). > > > > > > Perhaps something like: > > > > > > "WebCrypto: Implement digest() using NSS" would be more concise. > > > > Just want to put an update here: I've figured out that the symbols aren't > being > > exported and that the builders must be doing a shared build > > (http://www.chromium.org/developers/how-tos/component-build). Still have not > > encountered the [original] error locally by building a shared build, which I > > think might help to resolve the Windows build errors. > > Yes, the builders do shared builds. > > I'm guessing you'r not testing on Windows, as this is an MSVC error. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... > > Please read the caveats of the associated MSDN article and ensure it's safe. > > Alternatively, WebKit::WebCrypto may need to be WEBKIT_EXPORT, but someone like > abarth@ should chime in there. > > Or don't make the implementation a CONTENT_EXPORT. Given that you're not > affecting the CONTENT public API at all, I'm not sure why you're adding > CONTENT_EXPORT. Got a shared build working properly now, and it is flagging the original error. I see what you're saying about the content API too - I've found that there are external APIs that allow me to grab an instance to WebCryptoImpl, so I think now I should be able to get past this error without the need to change the WebKit exports.
Adding James to review for owners approval.
When do we use NSS vs OpenSSL? https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... content/renderer/webcrypto_digest.h:26: virtual void process(const unsigned char* bytes, size_t size) OVERRIDE; document what interface these are overriding if they're from the interface defined in a third_party/WebKit/... header, don't use OVERRIDE. Having the OVERRIDE annotation on cross-repository dependencies makes the interface much harder to iterate on since you can't atomically change the interface and implementations.
On 2013/08/07 23:25:31, jamesr wrote: > When do we use NSS vs OpenSSL? > !Android vs Android > https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... > File content/renderer/webcrypto_digest.h (right): > > https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... > content/renderer/webcrypto_digest.h:26: virtual void process(const unsigned > char* bytes, size_t size) OVERRIDE; > document what interface these are overriding > > if they're from the interface defined in a third_party/WebKit/... header, don't > use OVERRIDE. Having the OVERRIDE annotation on cross-repository dependencies > makes the interface much harder to iterate on since you can't atomically change > the interface and implementations.
On 2013/08/07 23:27:02, Ryan Sleevi wrote: > On 2013/08/07 23:25:31, jamesr wrote: > > When do we use NSS vs OpenSSL? > > > > !Android vs Android Although we use USE_OPENSSL, rather than OS_ANDROID, in order to be able to build and debug using Linux with the GYP flag use_openssl=1 [mostly for Dev testing, although exists as an FYI bot called linux_redux]
https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... content/renderer/webcrypto_digest_nss.cc:79: unsigned int hash_result_length = HASH_ResultLen(HASH_GetType(context_)); This can be simplified to: HASH_ResultLenContext()
On 2013/08/09 03:52:39, eroman wrote: > https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... > File content/renderer/webcrypto_digest_nss.cc (right): > > https://codereview.chromium.org/19757011/diff/90001/content/renderer/webcrypt... > content/renderer/webcrypto_digest_nss.cc:79: unsigned int hash_result_length = > HASH_ResultLen(HASH_GetType(context_)); > This can be simplified to: HASH_ResultLenContext() Updated to this interface. Also removed overrides and brought over documentation for digest interface.
I'm not very familiar with the API design here but things that delete themselves are pretty unusual in this part of the code. Can you give me a pointer to some information about how this API works? https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest.h:30: virtual void process(const unsigned char* bytes, size_t size); add a documenting what these override. for example, something like: // WebKit::WebCryptoOperation implementation. https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_nss.cc:24: HASH_Destroy(context_); you could also define a HASHDeleter that overrides operator() to call HASH_Destroy and then store this hash in a scoped_ptr<Type, HASHDeleter> https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_nss.cc:73: delete this; this is very unusual. what's the lifecycle of this object? https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_unittest.cc:132: for (unsigned int id_index = 0; why not size_t for this index?
jamesr: the API is explained here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... As far as things deleting themselves, that was an issue which Adam pointed out to me as well, however I haven't found a good alternative yet. On Fri, Aug 9, 2013 at 1:04 PM, <jamesr@chromium.org> wrote: > I'm not very familiar with the API design here but things that delete > themselves > are pretty unusual in this part of the code. Can you give me a pointer to > some > information about how this API works? > > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest.h<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest.h> > File content/renderer/webcrypto_**digest.h (right): > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest.h#newcode30<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest.h#newcode30> > content/renderer/webcrypto_**digest.h:30: virtual void process(const > unsigned char* bytes, size_t size); > add a documenting what these override. for example, something like: > > // WebKit::WebCryptoOperation implementation. > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest_nss.cc<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest_nss.cc> > File content/renderer/webcrypto_**digest_nss.cc (right): > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest_nss.cc#newcode24<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest_nss.cc#newcode24> > content/renderer/webcrypto_**digest_nss.cc:24: HASH_Destroy(context_); > you could also define a HASHDeleter that overrides operator() to call > HASH_Destroy and then store this hash in a scoped_ptr<Type, HASHDeleter> > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest_nss.cc#newcode73<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest_nss.cc#newcode73> > content/renderer/webcrypto_**digest_nss.cc:73: delete this; > this is very unusual. what's the lifecycle of this object? > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest_unittest.cc<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest_unittest.cc> > File content/renderer/webcrypto_**digest_unittest.cc (right): > > https://codereview.chromium.**org/19757011/diff/110001/** > content/renderer/webcrypto_**digest_unittest.cc#newcode132<https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcrypto_digest_unittest.cc#newcode132> > content/renderer/webcrypto_**digest_unittest.cc:132: for (unsigned int > id_index = 0; > why not size_t for this index? > > https://codereview.chromium.**org/19757011/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest.h:30: virtual void process(const unsigned char* bytes, size_t size); On 2013/08/09 20:04:55, jamesr wrote: > add a documenting what these override. for example, something like: > > // WebKit::WebCryptoOperation implementation. Done. https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_nss.cc:24: HASH_Destroy(context_); On 2013/08/09 20:04:55, jamesr wrote: > you could also define a HASHDeleter that overrides operator() to call > HASH_Destroy and then store this hash in a scoped_ptr<Type, HASHDeleter> Ah, very cool. Done. https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_nss.cc:73: delete this; On 2013/08/09 20:04:55, jamesr wrote: > this is very unusual. what's the lifecycle of this object? The way the interface is defined, this object needs to be freed once it has been aborted. The object is created when a WebKit::WebCrypto operation is kicked off, then WebKit::CryptoOperation makes calls into it to process/abort/finish. WebKit::CryptoOperationImpl maintains the state, but does not own the object, so it frees itself and relies on WebKit::CryptoOperationImpl to maintain the proper state. https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest_unittest.cc (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_unittest.cc:132: for (unsigned int id_index = 0; On 2013/08/09 20:04:55, jamesr wrote: > why not size_t for this index? Done.
https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... File content/renderer/webcrypto_digest_nss.cc (right): https://codereview.chromium.org/19757011/diff/110001/content/renderer/webcryp... content/renderer/webcrypto_digest_nss.cc:24: HASH_Destroy(context_); On 2013/08/13 18:22:21, Bryan Eyler wrote: > On 2013/08/09 20:04:55, jamesr wrote: > > you could also define a HASHDeleter that overrides operator() to call > > HASH_Destroy and then store this hash in a scoped_ptr<Type, HASHDeleter> > > Ah, very cool. Done. I will defer to sleevi, however if we go this route probably better to add them to src/crypto/scoped_nss_types.h with the others.
https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... content/renderer/webcrypto_digest.h:46: scoped_ptr<HASHContext, HASHDeleter> context_; style: You should just be able to forward declare HASHDeleter - without having to declare the functions/type, because the scoped_ptr<> template dtor shouldn't be instantiated until the .cc file. BUG? missing DISALLOW_COPY_AND_ASSIGN I'm not convinced the value of the scoped_ptr<> is all that great here for this use case. If it was something widely used, I can see scoped_nss_types, but here, there's only one exit point - seems reasonable to just destroy the hash in the dtor.
https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... content/renderer/webcrypto_digest.h:46: scoped_ptr<HASHContext, HASHDeleter> context_; I get compile-time errors if I try to forward-declare this, so I don't think it's possible. For DISALLOW_COPY_AND_ASSIGN, you'd talking about WebCryptoDigest, correct? Will add to this class. Should we just go back to using a normal pointer? It seemed to produce slightly simpler and cleaner code. On 2013/08/13 18:58:16, Ryan Sleevi wrote: > style: You should just be able to forward declare HASHDeleter - without having > to declare the functions/type, because the scoped_ptr<> template dtor shouldn't > be instantiated until the .cc file. > > BUG? missing DISALLOW_COPY_AND_ASSIGN > > I'm not convinced the value of the scoped_ptr<> is all that great here for this > use case. If it was something widely used, I can see scoped_nss_types, but here, > there's only one exit point - seems reasonable to just destroy the hash in the > dtor.
https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... content/renderer/webcrypto_digest.h:46: scoped_ptr<HASHContext, HASHDeleter> context_; On 2013/08/13 23:24:12, Bryan Eyler wrote: > I get compile-time errors if I try to forward-declare this, so I don't think > it's possible. > > For DISALLOW_COPY_AND_ASSIGN, you'd talking about WebCryptoDigest, correct? Yes > Will add to this class. > > Should we just go back to using a normal pointer? It seemed to produce slightly > simpler and cleaner code. SGTM. > > On 2013/08/13 18:58:16, Ryan Sleevi wrote: > > style: You should just be able to forward declare HASHDeleter - without having > > to declare the functions/type, because the scoped_ptr<> template dtor > shouldn't > > be instantiated until the .cc file. > > > > BUG? missing DISALLOW_COPY_AND_ASSIGN > > > > I'm not convinced the value of the scoped_ptr<> is all that great here for > this > > use case. If it was something widely used, I can see scoped_nss_types, but > here, > > there's only one exit point - seems reasonable to just destroy the hash in the > > dtor. >
https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... File content/renderer/webcrypto_digest.h (right): https://codereview.chromium.org/19757011/diff/119001/content/renderer/webcryp... content/renderer/webcrypto_digest.h:46: scoped_ptr<HASHContext, HASHDeleter> context_; On 2013/08/13 23:24:47, Ryan Sleevi wrote: > On 2013/08/13 23:24:12, Bryan Eyler wrote: > > I get compile-time errors if I try to forward-declare this, so I don't think > > it's possible. > > > > For DISALLOW_COPY_AND_ASSIGN, you'd talking about WebCryptoDigest, correct? > > Yes > > > Will add to this class. > > > > Should we just go back to using a normal pointer? It seemed to produce > slightly > > simpler and cleaner code. > > SGTM. > > > > > On 2013/08/13 18:58:16, Ryan Sleevi wrote: > > > style: You should just be able to forward declare HASHDeleter - without > having > > > to declare the functions/type, because the scoped_ptr<> template dtor > > shouldn't > > > be instantiated until the .cc file. > > > > > > BUG? missing DISALLOW_COPY_AND_ASSIGN > > > > > > I'm not convinced the value of the scoped_ptr<> is all that great here for > > this > > > use case. If it was something widely used, I can see scoped_nss_types, but > > here, > > > there's only one exit point - seems reasonable to just destroy the hash in > the > > > dtor. > > > Done.
Latest patch set rebases onto the simplification from not having to do partial updates of crypto operations. This simplifies it so much that I think that the logic can all go into one implementation file.
lgtm https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:12: #include "base/memory/scoped_ptr.h" unused; remove https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:25: #if defined(USE_NSS) Because this is quickly going to turn into a lot of defines, I suggest splitting it into a separate file from the get-go * webcrypto_impl.h defines the class * webcrypto_impl_nss.cc has the method definitions (for NSS) * webcrypto_impl_openssl.cc has the method definitions (for OpenSSL) * webcrypto_impl_unittest.cc has the tests which apply to WebCryptoImpl (shared by both NSS and openssl). https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:54: LOG(ERROR) << "Could not create digest context for hash algorithm: " I don't think there is value to having these LOG(ERROR) and suggest deleting them. (In a future update of the API I will add a string parameter to completeWithError() so any relevant information can be stuffed into the javascript's exception message.) https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:69: DCHECK(digest); I think the only situation when buffer.data() can be NULL is if (length * size) in the call to WebArrayBuffer::create() above causes 32-bit integer overflow. Probably safest to handle it explicitly instead of the DCHECK: if (!digest) { result.completeWithError(); return; } https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:74: LOG(ERROR) << "Result length invalid; expected " << hash_result_length LOG(ERROR) have very little usefulness in my experience, I suggest deleting (and we will pass the details to completeWithError in the future). https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:20: // Algorithms include SHA1, SHA224, SHA256, SHA384, and SHA512 Delete this comment, doesn't explain anything extra about the code. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:21: WebKit::WebCryptoAlgorithmId algorithm_ids[] = { mark this "const", and rename algorithm_ids --> kAlgorithmIds https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:35: virtual void completeWithError() OVERRIDE { I think it is better practice to omit OVERRIDE on interfaces defined in blink -- it allows refactoring the blink side without breaking chromium compile. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:64: return array_buffer_data_hex_.c_str(); how about returning a const std::string& instead? https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:85: SetRendererClientForTesting(&content_renderer_client_); Can you just directly instantiate the class under test? (i.e. new WebCryptoImpl()). https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:103: // For readability and maintainability of the results, the results are nit: redundant use of the word results. Try: For readability the results are stored as hex strings.
https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:64: unsigned int hash_result_length = HASH_ResultLenContext(context); i think size_t is a better type, but you don't want that at least use an explicitly sized type https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:69: DCHECK(digest); I'm not sure this DCHECK makes sense. WebArrayBuffer::create() will only return a null buffer if you request it to allocate > 2^32 bytes of data, which seems crazy for the length of a hash result. I think it'd be better to DCHECK that your inputs are sane if you aren't sure https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:71: unsigned int result_length = 0; please use a better type https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:35: virtual void completeWithError() OVERRIDE { no OVERRIDE for cross-repository overrides! https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:56: RefCountedThreadSafe<TestWebCryptoResult>::AddRef(); these are a bit odd. what are they implementing?
https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:12: #include "base/memory/scoped_ptr.h" On 2013/08/22 02:25:05, eroman wrote: > unused; remove Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:25: #if defined(USE_NSS) On 2013/08/22 02:25:05, eroman wrote: > Because this is quickly going to turn into a lot of defines, I suggest splitting > it into a separate file from the get-go > > * webcrypto_impl.h defines the class > * webcrypto_impl_nss.cc has the method definitions (for NSS) > * webcrypto_impl_openssl.cc has the method definitions (for OpenSSL) > * webcrypto_impl_unittest.cc has the tests which apply to WebCryptoImpl (shared > by both NSS and openssl). > Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:54: LOG(ERROR) << "Could not create digest context for hash algorithm: " On 2013/08/22 02:25:05, eroman wrote: > I don't think there is value to having these LOG(ERROR) and suggest deleting > them. > > (In a future update of the API I will add a string parameter to > completeWithError() so any relevant information can be stuffed into the > javascript's exception message.) Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:64: unsigned int hash_result_length = HASH_ResultLenContext(context); On 2013/08/22 02:31:01, jamesr wrote: > i think size_t is a better type, but you don't want that at least use an > explicitly sized type Changed to size_t. FWIW, I was trying to match the return type of HASH_ResultLenContext(). https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:69: DCHECK(digest); On 2013/08/22 02:31:01, jamesr wrote: > I'm not sure this DCHECK makes sense. WebArrayBuffer::create() will only return > a null buffer if you request it to allocate > 2^32 bytes of data, which seems > crazy for the length of a hash result. I think it'd be better to DCHECK that > your inputs are sane if you aren't sure DCHECK changed to sanity check hash_result_length. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:71: unsigned int result_length = 0; On 2013/08/22 02:31:01, jamesr wrote: > please use a better type I've made this uint32 for lack of a better type. size_t won't typecast properly to be passed into HASH_End(). https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl.cc:74: LOG(ERROR) << "Result length invalid; expected " << hash_result_length On 2013/08/22 02:25:05, eroman wrote: > LOG(ERROR) have very little usefulness in my experience, I suggest deleting (and > we will pass the details to completeWithError in the future). Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:20: // Algorithms include SHA1, SHA224, SHA256, SHA384, and SHA512 On 2013/08/22 02:25:05, eroman wrote: > Delete this comment, doesn't explain anything extra about the code. Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:21: WebKit::WebCryptoAlgorithmId algorithm_ids[] = { On 2013/08/22 02:25:05, eroman wrote: > mark this "const", and rename algorithm_ids --> kAlgorithmIds Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:35: virtual void completeWithError() OVERRIDE { On 2013/08/22 02:31:01, jamesr wrote: > no OVERRIDE for cross-repository overrides! Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:56: RefCountedThreadSafe<TestWebCryptoResult>::AddRef(); On 2013/08/22 02:31:01, jamesr wrote: > these are a bit odd. what are they implementing? Ref counting as required in the WebCryptoResultPrivate abstract class. I could add comments about this being more typical of the WebKit objects? https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:64: return array_buffer_data_hex_.c_str(); On 2013/08/22 02:25:05, eroman wrote: > how about returning a const std::string& instead? Done. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:85: SetRendererClientForTesting(&content_renderer_client_); On 2013/08/22 02:25:05, eroman wrote: > Can you just directly instantiate the class under test? (i.e. new > WebCryptoImpl()). No, it doesn't link because the WebCryptoImpl class is not exported in the content renderer. Exporting the class makes this work, but previous CL comments indicated that I should not be changing the content API. https://codereview.chromium.org/19757011/diff/134001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:103: // For readability and maintainability of the results, the results are On 2013/08/22 02:25:05, eroman wrote: > nit: redundant use of the word results. Try: > For readability the results are stored as hex strings. Done.
FYI, I'm just trying to figure out what to do about these Try failures on Mac and Windows, since I expected NSS to be used by them. Expect another CL coming at some point to fix these failures.
https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... File content/content_renderer.gypi (right): https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... content/content_renderer.gypi:697: ['use_nss==1', { This is wrong. "use_nss" means "use_nss for everything, including cert verification" ['use_openssl==1', { 'sources': [ 'renderer/webcrypto_impl_openssl.cc', ], 'dependencies': [ '../third_party/openssl/openssl.gyp:openssl', ], }, { 'sources': [ 'renderer/webcrypto_impl_nss.cc', ], 'conditions': [ ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', { 'dependencies': [ '../build/linux/system.gyp:ssl', ], }, { 'dependencies': [ '../third_party/nss/nss.gyp:nspr', '../third_party/nss/nss.gyp:nss', ], }], ], }] Don't forget that you will also need to update the dependencies, since you're using NSS/OpenSSL functions.
On 2013/08/22 21:10:28, Ryan Sleevi wrote: > https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... > File content/content_renderer.gypi (right): > > https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... > content/content_renderer.gypi:697: ['use_nss==1', { > This is wrong. "use_nss" means "use_nss for everything, including cert > verification" > > ['use_openssl==1', { > 'sources': [ > 'renderer/webcrypto_impl_openssl.cc', > ], > 'dependencies': [ > '../third_party/openssl/openssl.gyp:openssl', > ], > }, { > 'sources': [ > 'renderer/webcrypto_impl_nss.cc', > ], > 'conditions': [ > ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', { > 'dependencies': [ > '../build/linux/system.gyp:ssl', > ], > }, { > 'dependencies': [ > '../third_party/nss/nss.gyp:nspr', > '../third_party/nss/nss.gyp:nss', > ], > }], > ], > }] > > Don't forget that you will also need to update the dependencies, since you're > using NSS/OpenSSL functions. Ah, yes, I found similar logic in crypto/crypto.gyp. Thanks!
On 2013/08/22 21:13:27, Bryan Eyler wrote: > On 2013/08/22 21:10:28, Ryan Sleevi wrote: > > > https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... > > File content/content_renderer.gypi (right): > > > > > https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... > > content/content_renderer.gypi:697: ['use_nss==1', { > > This is wrong. "use_nss" means "use_nss for everything, including cert > > verification" > > > > ['use_openssl==1', { > > 'sources': [ > > 'renderer/webcrypto_impl_openssl.cc', > > ], > > 'dependencies': [ > > '../third_party/openssl/openssl.gyp:openssl', > > ], > > }, { > > 'sources': [ > > 'renderer/webcrypto_impl_nss.cc', > > ], > > 'conditions': [ > > ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', { > > 'dependencies': [ > > '../build/linux/system.gyp:ssl', > > ], > > }, { > > 'dependencies': [ > > '../third_party/nss/nss.gyp:nspr', > > '../third_party/nss/nss.gyp:nss', > > ], > > }], > > ], > > }] > > > > Don't forget that you will also need to update the dependencies, since you're > > using NSS/OpenSSL functions. > > Ah, yes, I found similar logic in crypto/crypto.gyp. Thanks! I'm actually a little conflicted on the style here. It makes readability sense to use the method you wrote here, but the style in the gyp file is to list all the possibilities and "sources!" the unused ones out. Any preference here?
On 2013/08/22 21:38:51, Bryan Eyler wrote: > On 2013/08/22 21:13:27, Bryan Eyler wrote: > > On 2013/08/22 21:10:28, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... > > > File content/content_renderer.gypi (right): > > > > > > > > > https://codereview.chromium.org/19757011/diff/156001/content/content_renderer... > > > content/content_renderer.gypi:697: ['use_nss==1', { > > > This is wrong. "use_nss" means "use_nss for everything, including cert > > > verification" > > > > > > ['use_openssl==1', { > > > 'sources': [ > > > 'renderer/webcrypto_impl_openssl.cc', > > > ], > > > 'dependencies': [ > > > '../third_party/openssl/openssl.gyp:openssl', > > > ], > > > }, { > > > 'sources': [ > > > 'renderer/webcrypto_impl_nss.cc', > > > ], > > > 'conditions': [ > > > ['os_posix == 1 and OS != "mac" and OS != "ios" and OS != "android"', { > > > 'dependencies': [ > > > '../build/linux/system.gyp:ssl', > > > ], > > > }, { > > > 'dependencies': [ > > > '../third_party/nss/nss.gyp:nspr', > > > '../third_party/nss/nss.gyp:nss', > > > ], > > > }], > > > ], > > > }] > > > > > > Don't forget that you will also need to update the dependencies, since > you're > > > using NSS/OpenSSL functions. > > > > Ah, yes, I found similar logic in crypto/crypto.gyp. Thanks! > > I'm actually a little conflicted on the style here. It makes readability sense > to use the method you wrote here, but the style in the gyp file is to list all > the possibilities and "sources!" the unused ones out. Any preference here? sources! is correct I provided the above to make sure it was clear as to all the necessary bits. But "sources!" is *definitely* the idiomatic Chromium GYP style.
https://codereview.chromium.org/19757011/diff/161001/content/renderer/webcryp... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/19757011/diff/161001/content/renderer/webcryp... content/renderer/webcrypto_impl_nss.cc:71: I believe the HASHContext* is getting leaked (not calling HASH_Destroy). There are two paths where control returns without freeing it. Either add a scoped deleter, or update those two locations.
https://codereview.chromium.org/19757011/diff/161001/content/renderer/webcryp... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/19757011/diff/161001/content/renderer/webcryp... content/renderer/webcrypto_impl_nss.cc:71: On 2013/08/22 22:59:01, eroman wrote: > I believe the HASHContext* is getting leaked (not calling HASH_Destroy). > > There are two paths where control returns without freeing it. Either add a > scoped deleter, or update those two locations. Done.
lgtm https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_openssl.cc:15: (void)algorithm; What is the purpose of these (void)? If you are getting an unreferenced variable warning from the compiler, I would just delete the names from the argument list above.
https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_openssl.cc:15: (void)algorithm; On 2013/08/22 23:15:12, eroman wrote: > What is the purpose of these (void)? > > If you are getting an unreferenced variable warning from the compiler, I would > just delete the names from the argument list above. This was more just preemptive to avoid a compiler warning. Removed and will see if Try returns any errors.
https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_nss.cc:58: DCHECK(hash_result_length <= HASH_LENGTH_MAX); DCHECK_LE() https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_openssl.cc:15: (void)algorithm; On 2013/08/22 23:15:12, eroman wrote: > What is the purpose of these (void)? > > If you are getting an unreferenced variable warning from the compiler, I would > just delete the names from the argument list above. I don't think we generate such warnings. Just remove these. https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:21: WebKit::WebCryptoAlgorithmIdSha1, why the 4-space indent? https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:41: // converted anyway for better error reporting. +1 for the nul terminator. the comment says +1, but I don't see anything of that nature in the code. https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:46: virtual void completeWithBoolean(bool boolean) { "bool boolean"? is there no better name? https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:47: NOTREACHED(); would NOTIMPLEMENTED() be a better match? https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:86: webkit_platform_support_.reset(new RendererWebKitPlatformSupportImpl); do you need to instantiate an entire RendererWebKitPlatformSupportImpl just to get access to a WebCryptoImpl? Why not just say "new WebCryptoImpl"? https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:170: set_index < ARRAYSIZE_UNSAFE(input_set); set_index++) { nit: since the first and second statements in the for are on different lines i think the third statement should get its own line too
https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_nss.cc:58: DCHECK(hash_result_length <= HASH_LENGTH_MAX); On 2013/08/23 01:27:40, jamesr wrote: > DCHECK_LE() Done. https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:21: WebKit::WebCryptoAlgorithmIdSha1, On 2013/08/23 01:27:40, jamesr wrote: > why the 4-space indent? See comments in patch set 5: https://codereview.chromium.org/19757011/patch/51001/52005 I believe this is correct, as per: http://www.corp.google.com/eng/doc/cppguide.xml?showone=Braced_Initializer_Li... Do you agree? Or is the style different in this module? https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:41: // converted anyway for better error reporting. +1 for the nul terminator. On 2013/08/23 01:27:40, jamesr wrote: > the comment says +1, but I don't see anything of that nature in the code. Sorry, that part of the comment is stale. Removed. https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:46: virtual void completeWithBoolean(bool boolean) { On 2013/08/23 01:27:40, jamesr wrote: > "bool boolean"? is there no better name? Changed to "result". https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:47: NOTREACHED(); On 2013/08/23 01:27:40, jamesr wrote: > would NOTIMPLEMENTED() be a better match? Yes, I think so, since I eventually intend to implement these for unit testing functionality beyond SHA. Changed. https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:86: webkit_platform_support_.reset(new RendererWebKitPlatformSupportImpl); On 2013/08/23 01:27:40, jamesr wrote: > do you need to instantiate an entire RendererWebKitPlatformSupportImpl just to > get access to a WebCryptoImpl? Why not just say "new WebCryptoImpl"? WebCryptoImpl is not exported as part of the content API, so I need to use what resources are available in that API to get this instance. https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:170: set_index < ARRAYSIZE_UNSAFE(input_set); set_index++) { On 2013/08/23 01:27:40, jamesr wrote: > nit: since the first and second statements in the for are on different lines i > think the third statement should get its own line too Done.
https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... content/renderer/webcrypto_impl_unittest.cc:86: webkit_platform_support_.reset(new RendererWebKitPlatformSupportImpl); On 2013/08/23 17:45:31, Bryan Eyler wrote: > On 2013/08/23 01:27:40, jamesr wrote: > > do you need to instantiate an entire RendererWebKitPlatformSupportImpl just to > > get access to a WebCryptoImpl? Why not just say "new WebCryptoImpl"? > > WebCryptoImpl is not exported as part of the content API, so I need to use what > resources are available in that API to get this instance. NAK, if you need access to WebCryptoImpl in a test then just export WebCryptoImpl from content. We export types from content to content_unittests all the time. Don't add a dependency to WebKitPlatformSupportImpl.
On 2013/08/23 18:16:57, jamesr wrote: > https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... > File content/renderer/webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.org/19757011/diff/168001/content/renderer/webcryp... > content/renderer/webcrypto_impl_unittest.cc:86: > webkit_platform_support_.reset(new RendererWebKitPlatformSupportImpl); > On 2013/08/23 17:45:31, Bryan Eyler wrote: > > On 2013/08/23 01:27:40, jamesr wrote: > > > do you need to instantiate an entire RendererWebKitPlatformSupportImpl just > to > > > get access to a WebCryptoImpl? Why not just say "new WebCryptoImpl"? > > > > WebCryptoImpl is not exported as part of the content API, so I need to use > what > > resources are available in that API to get this instance. > > NAK, if you need access to WebCryptoImpl in a test then just export > WebCryptoImpl from content. We export types from content to content_unittests > all the time. Don't add a dependency to WebKitPlatformSupportImpl. Okay. Comment #30 indicates that exporting might not be a great idea, since it also may need the WebKit class exported? Seems to work without this export, but maybe this is a style thing as well.
Comment #30 seems incorrect to me. Exporting the type and using the NON_EXPORTED_BASE annotation would be fine here. CONTENT_EXPORT is for usage outside the content DLL by tests or other modules, not just for public content API.
On 2013/08/23 18:35:02, jamesr wrote: > Comment #30 seems incorrect to me. Exporting the type and using the > NON_EXPORTED_BASE annotation would be fine here. CONTENT_EXPORT is for usage > outside the content DLL by tests or other modules, not just for public content > API. Okay, makes sense.
Thanks. I have some higher-level feedback, but you don't have to consider that blocking for this patch. I still think parts of the Blink API usage here are a bit awkward. In particular, there's a WebCryptoResultPrivate object that is normally implemented inside the Blink repository but are here implemented by a test class in Chromium. This is awkward since there's one ownership model in production - Blink references this class and maintains it via WebCryptoResult instances - and another that's different in unit tests. I don't want to see the ref()/deref() pattern on a public Blink API in general. I also understand that you want to write unit tests for the actual crypto logic on the chromium side since that's where implementations are, and that's reasonable. I think a better way to factor this would be to have the actual implementations of the crypto operations be chromium classes that operate and are tested separately and then have thin wrappers to map the Blink types onto the chromium implementations. Then your chromium unit tests would be implemented without using the Blink crypto APIs and the Blink public API would only have to reflect the intended production usage.
lgtm, but please consider whether it's easier to refactor this before or after landing. From my perspective it seems like the implementations of these are simple enough that it'd be relatively as easy to refactor now or later but I don't know how complex the future implementations will be. I do not want to leave the WebCryptoResultPrivate type in the Blink public API for long since it's a bad pattern that may mislead other people trying to learn how to construct Blink APIs. https://codereview.chromium.org/19757011/diff/181001/content/renderer/webcryp... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/19757011/diff/181001/content/renderer/webcryp... content/renderer/webcrypto_impl_openssl.cc:14: // TODO(bryaneyler): Placeholder for OpenSSL implementation. Issue 267888. could you linkify this? i.e. "http://crbug.com/267888" ?
On 2013/08/23 21:27:44, jamesr wrote: > lgtm, but please consider whether it's easier to refactor this before or after > landing. From my perspective it seems like the implementations of these are > simple enough that it'd be relatively as easy to refactor now or later but I > don't know how complex the future implementations will be. I do not want to > leave the WebCryptoResultPrivate type in the Blink public API for long since > it's a bad pattern that may mislead other people trying to learn how to > construct Blink APIs. > > https://codereview.chromium.org/19757011/diff/181001/content/renderer/webcryp... > File content/renderer/webcrypto_impl_openssl.cc (right): > > https://codereview.chromium.org/19757011/diff/181001/content/renderer/webcryp... > content/renderer/webcrypto_impl_openssl.cc:14: // TODO(bryaneyler): Placeholder > for OpenSSL implementation. Issue 267888. > could you linkify this? i.e. "http://crbug.com/267888" ? I think this is do-able before landing. Uploaded a new CL that splits this functionality out.
lgtm. I think this ends up being nice for the code, too, since the code that interfaces with NSS/whatever is separate from the code that has to know the intricacies of the Blink API.
Hi avi and jochen, Please take a look for content_tests.gypi owners approval. Thanks
lgtm
(For the record I only looked at the gyp files).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryaneyler@google.com/19757011/185001
Message was sent while issue was closed.
Change committed as 220470
Message was sent while issue was closed.
Sorry, ignore patch set 20. That was a mistaken upload that was intended to be a new issue.
Message was sent while issue was closed.
Bryan, for future reference: Note that you can "delete" individual patchsets from codereviews. So in this case, I recommend deleting patchset 20 to avoid any future confusion if someone reads this codereview thread. In general you don't want to delete patchsets after the CL has been submitted for review (for obvious reasons). However it is a great technique to use during development to coalesce patchsets when you forgot to upload something.
Message was sent while issue was closed.
On 2013/09/05 02:10:12, eroman wrote: > Bryan, for future reference: > > Note that you can "delete" individual patchsets from codereviews. > > So in this case, I recommend deleting patchset 20 to avoid any future confusion > if someone reads this codereview thread. > > In general you don't want to delete patchsets after the CL has been submitted > for review (for obvious reasons). However it is a great technique to use during > development to coalesce patchsets when you forgot to upload something. Excellent. Deleted patch set 20. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
