Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23621050:
Added WebCypto digest OpenSSL implementation. (Closed)
|
Created:
7 years, 3 months ago by padolph Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded WebCypto digest OpenSSL implementation.
BUG=267888
TEST=content_unittests --gtest_filter="WebCryptoImpl*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227175
Patch Set 1 #Patch Set 2 : Trivial change to test cl upload #
Total comments: 19
Patch Set 3 : Fixes for bryaneyler and rsleevi #Patch Set 4 : clang-format reformatting #
Total comments: 7
Patch Set 5 : clang-format -style=Chromium reformatting #Patch Set 6 : Fixes for bryaneyler #Patch Set 7 : rebase only #Patch Set 8 : for openssl, #ifdef disable all tests except digest #
Total comments: 2
Patch Set 9 : rebase only #Patch Set 10 : rebase #Patch Set 11 : fixed minor rebase issues #
Messages
Total messages: 27 (0 generated)
New CL to try to fix upload problems
Also please enable unit tests for OpenSSL on this algorithm. The unit tests are disabled for all OpenSSL at the moment, but I think it makes sense to enable them for digest. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:7: #include <openssl/evp.h> Following style of _nss.cc, leave a space between system includes and Chrome headers. I can't seem to find a link to this in the style guide ATM. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:25: crypto::OpenSSLErrStackTracer(FROM_HERE); Two-space indentation throughout this function - here, and in following indentations. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:50: digCtx(EVP_MD_CTX_create()); Naming: Don't abbreviate and use underscores to separate words. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...
Hi Paul, Some initial style comments to get you moving and save you a round-trip. One other really nice thing about Chromium is that if you're developing on Mac or Linux, you can use Clang, and optionally use clang-format. You can find more about clang-format at http://clang.llvm.org/docs/ClangFormat.html . To use the latest version (eg: what Chromium developers at Google are using), you can use the clang update script in Chromium, at https://code.google.com/p/chromium/wiki/Clang "./tools/clang/scripts/update.sh --force-local-build --without-android" Then, follow the clang tools checkout instructions, http://clang.llvm.org/docs/ClangTools.html , to land it into src/third_party/llvm (/tools/clang/tools) And then re-run the script. You should have src/third_party/llvm-build/Release+Asserts/bin/clang-format after that. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:7: #include <openssl/evp.h> On 2013/09/18 21:36:21, Bryan Eyler wrote: > Following style of _nss.cc, leave a space between system includes and Chrome > headers. I can't seem to find a link to this in the style guide ATM. http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:27: const EVP_MD * digAlg; STYLE: "const EVP_MD *" -> "const EVP_MD* " (see http://www.chromium.org/developers/coding-style#Code_Formatting - "put * and & by the type rather than the variable name" STYLE: digest_algorithm or digest_alg ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... "eschew abbreviation") and http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... ("Variable names are all lowercase, with underscores between words") https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:57: if (!EVP_DigestUpdate(digCtx.get(), data, data_size)) STYLE NIT (eg: not critical, but perhaps more consistent) if (!EVP_DigestInit_ex(digest_context.get(), digest_algorithm, NULL) || !EVP_DigestUpdate(digest_context.get(), data, data_size) { return false; } Basically, condense conditionals. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:60: const size_t hashResultSize = EVP_MD_CTX_size(digCtx.get()); CORRECTNESS: EVP_MD_CTX_size returns an int, not a size_t, and MAY fail (return <= 0) STYLE: naming: hash_result_size / hash_result_length https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:64: unsigned char* const hashBuf = STYLE: hashBuf -> hash_buffer / result_buffer https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:67: unsigned int hashSize = 0; STYLE: hashSize -> hash_size https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:68: if (!(EVP_DigestFinal_ex(digCtx.get(), hashBuf, &hashSize))) STYLE: unnecessary added () STYLE NIT (eg: not critical, but perhaps more consistent) try: if (!EVP_DigestFinal_ex(digest_context.get(), hash_buffer, &hash_size)) || hash_size != hash_result_size) { return false; } return true; [I'm not sure the exact wrapping of the above, so adjust as appropriate] Handle more failures inline, as possible.
https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:7: #include <openssl/evp.h> On 2013/09/18 22:05:36, Ryan Sleevi wrote: > On 2013/09/18 21:36:21, Bryan Eyler wrote: > > Following style of _nss.cc, leave a space between system includes and Chrome > > headers. I can't seem to find a link to this in the style guide ATM. > > http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:25: crypto::OpenSSLErrStackTracer(FROM_HERE); On 2013/09/18 21:36:21, Bryan Eyler wrote: > Two-space indentation throughout this function - here, and in following > indentations. Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:27: const EVP_MD * digAlg; On 2013/09/18 22:05:36, Ryan Sleevi wrote: > STYLE: "const EVP_MD *" -> "const EVP_MD* " (see > http://www.chromium.org/developers/coding-style#Code_Formatting - "put * and & > by the type rather than the variable name" > > STYLE: digest_algorithm or digest_alg ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... > "eschew abbreviation") and > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... > ("Variable names are all lowercase, with underscores between words") Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:50: digCtx(EVP_MD_CTX_create()); On 2013/09/18 21:36:21, Bryan Eyler wrote: > Naming: Don't abbreviate and use underscores to separate words. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming... Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:57: if (!EVP_DigestUpdate(digCtx.get(), data, data_size)) On 2013/09/18 22:05:36, Ryan Sleevi wrote: > STYLE NIT (eg: not critical, but perhaps more consistent) > > if (!EVP_DigestInit_ex(digest_context.get(), digest_algorithm, NULL) || > !EVP_DigestUpdate(digest_context.get(), data, data_size) { > return false; > } > > Basically, condense conditionals. Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:60: const size_t hashResultSize = EVP_MD_CTX_size(digCtx.get()); On 2013/09/18 22:05:36, Ryan Sleevi wrote: > CORRECTNESS: EVP_MD_CTX_size returns an int, not a size_t, and MAY fail (return > <= 0) > STYLE: naming: hash_result_size / hash_result_length Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:64: unsigned char* const hashBuf = On 2013/09/18 22:05:36, Ryan Sleevi wrote: > STYLE: hashBuf -> hash_buffer / result_buffer Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:67: unsigned int hashSize = 0; On 2013/09/18 22:05:36, Ryan Sleevi wrote: > STYLE: hashSize -> hash_size Done. https://codereview.chromium.org/23621050/diff/6001/content/renderer/webcrypto... content/renderer/webcrypto_impl_openssl.cc:68: if (!(EVP_DigestFinal_ex(digCtx.get(), hashBuf, &hashSize))) On 2013/09/18 22:05:36, Ryan Sleevi wrote: > STYLE: unnecessary added () > STYLE NIT (eg: not critical, but perhaps more consistent) > > try: > if (!EVP_DigestFinal_ex(digest_context.get(), hash_buffer, > &hash_size)) || > hash_size != hash_result_size) { > return false; > } > > return true; > > [I'm not sure the exact wrapping of the above, so adjust as appropriate] > > Handle more failures inline, as possible. Done. Also, I realized I should release buffer (buffer->reset() ? ) if either of these failures occur, since we then don't trust what OpenSSL did to it, and it is directly accessible to the caller.
On 2013/09/18 21:36:21, Bryan Eyler wrote: > Also please enable unit tests for OpenSSL on this algorithm. The unit tests are > disabled for all OpenSSL at the moment, but I think it makes sense to enable > them for digest. I didn't upload my change content_test.gypi because I wondered if that change was making the trybots fail. I see they are red again. Can you please take a look?
On 2013/09/18 22:05:36, Ryan Sleevi wrote: > Hi Paul, > > Some initial style comments to get you moving and save you a round-trip. > > One other really nice thing about Chromium is that if you're developing on Mac > or Linux, you can use Clang, and optionally use clang-format. > > You can find more about clang-format at > http://clang.llvm.org/docs/ClangFormat.html . To use the latest version (eg: > what Chromium developers at Google are using), you can use the clang update > script in Chromium, at https://code.google.com/p/chromium/wiki/Clang > > "./tools/clang/scripts/update.sh --force-local-build --without-android" > > Then, follow the clang tools checkout instructions, > http://clang.llvm.org/docs/ClangTools.html , to land it into > src/third_party/llvm (/tools/clang/tools) > > And then re-run the script. > > You should have src/third_party/llvm-build/Release+Asserts/bin/clang-format > after that. Thanks for this info. Looks very cool, installing now. For the code I'm working on, what -style spec for clang-format should I use? Google, Chromium, or WebKit?
On 2013/09/18 23:24:07, padolph wrote: > On 2013/09/18 22:05:36, Ryan Sleevi wrote: > > Hi Paul, > > > > Some initial style comments to get you moving and save you a round-trip. > > > > One other really nice thing about Chromium is that if you're developing on Mac > > or Linux, you can use Clang, and optionally use clang-format. > > > > You can find more about clang-format at > > http://clang.llvm.org/docs/ClangFormat.html . To use the latest version (eg: > > what Chromium developers at Google are using), you can use the clang update > > script in Chromium, at https://code.google.com/p/chromium/wiki/Clang > > > > "./tools/clang/scripts/update.sh --force-local-build --without-android" > > > > Then, follow the clang tools checkout instructions, > > http://clang.llvm.org/docs/ClangTools.html , to land it into > > src/third_party/llvm (/tools/clang/tools) > > > > And then re-run the script. > > > > You should have src/third_party/llvm-build/Release+Asserts/bin/clang-format > > after that. > > Thanks for this info. Looks very cool, installing now. > For the code I'm working on, what -style spec for clang-format should I use? > Google, Chromium, or WebKit? Chromium.
https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gypi File content/content_tests.gypi (left): https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gyp... content/content_tests.gypi:784: 'renderer/webcrypto_impl_unittest.cc', Removal of this check is correct, but my guess is that the unit tests will fail if this is submitted at the moment (and probably results in try failures) because there are unit tests in webcrypto_impl_unittest.cc that test functionality beyond what you've implemented in this CL. Also add an #ifdef in webcrypto_impl_unittest.cc to prevent these tests from running. https://codereview.chromium.org/23621050/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_openssl.cc:49: if (!digest_context.get()) return false; For consistency (with EVP_Digest* ops and hash_expected_size check), I would put this on separate lines with braces. https://codereview.chromium.org/23621050/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_openssl.cc:66: unsigned int hash_size = 0; I believe Chromium style is to just use "unsigned" instead of "unsigned int".
https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gypi File content/content_tests.gypi (left): https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gyp... content/content_tests.gypi:784: 'renderer/webcrypto_impl_unittest.cc', On 2013/09/23 21:45:51, Bryan Eyler wrote: > Removal of this check is correct, but my guess is that the unit tests will fail > if this is submitted at the moment (and probably results in try failures) > because there are unit tests in webcrypto_impl_unittest.cc that test > functionality beyond what you've implemented in this CL. > > Also add an #ifdef in webcrypto_impl_unittest.cc to prevent these tests from > running. The workflow to do what you suggest is not clear to me. When I initially started this issue, there was only the single DigestSampleSets test. I think I would need to rebase this issue to tip-tree to get the latest changes to webcrypto_impl_unittest.cc and then apply the #ifdef. But I thought master rebases are discouraged in the Reitveld review workflow, because doing so will bring in a ton of changes. Should I go ahead an do a rebase patch to this issue? Or should I just bring on the tip-version of the single webcrypto_impl_unittest.cc file and work on that? https://codereview.chromium.org/23621050/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_openssl.cc:49: if (!digest_context.get()) return false; On 2013/09/23 21:45:51, Bryan Eyler wrote: > For consistency (with EVP_Digest* ops and hash_expected_size check), I would put > this on separate lines with braces. Done. https://codereview.chromium.org/23621050/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_openssl.cc:66: unsigned int hash_size = 0; On 2013/09/23 21:45:51, Bryan Eyler wrote: > I believe Chromium style is to just use "unsigned" instead of "unsigned int". Done.
On 2013/09/23 22:08:31, padolph wrote: > https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gypi > File content/content_tests.gypi (left): > > https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gyp... > content/content_tests.gypi:784: 'renderer/webcrypto_impl_unittest.cc', > On 2013/09/23 21:45:51, Bryan Eyler wrote: > > Removal of this check is correct, but my guess is that the unit tests will > fail > > if this is submitted at the moment (and probably results in try failures) > > because there are unit tests in webcrypto_impl_unittest.cc that test > > functionality beyond what you've implemented in this CL. > > > > Also add an #ifdef in webcrypto_impl_unittest.cc to prevent these tests from > > running. > > The workflow to do what you suggest is not clear to me. When I initially started > this issue, there was only the single DigestSampleSets test. I think I would > need to rebase this issue to tip-tree to get the latest changes to > webcrypto_impl_unittest.cc and then apply the #ifdef. But I thought master > rebases are discouraged in the Reitveld review workflow, because doing so will > bring in a ton of changes. Should I go ahead an do a rebase patch to this issue? > > Or should I just bring on the tip-version of the single > webcrypto_impl_unittest.cc file and work on that? I chose the latter, please see Path #7. Is this what you had in mind?
On 2013/09/23 23:17:24, padolph wrote: > On 2013/09/23 22:08:31, padolph wrote: > > https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gypi > > File content/content_tests.gypi (left): > > > > > https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gyp... > > content/content_tests.gypi:784: 'renderer/webcrypto_impl_unittest.cc', > > On 2013/09/23 21:45:51, Bryan Eyler wrote: > > > Removal of this check is correct, but my guess is that the unit tests will > > fail > > > if this is submitted at the moment (and probably results in try failures) > > > because there are unit tests in webcrypto_impl_unittest.cc that test > > > functionality beyond what you've implemented in this CL. > > > > > > Also add an #ifdef in webcrypto_impl_unittest.cc to prevent these tests from > > > running. > > > > The workflow to do what you suggest is not clear to me. When I initially > started > > this issue, there was only the single DigestSampleSets test. I think I would > > need to rebase this issue to tip-tree to get the latest changes to > > webcrypto_impl_unittest.cc and then apply the #ifdef. But I thought master > > rebases are discouraged in the Reitveld review workflow, because doing so will > > bring in a ton of changes. Should I go ahead an do a rebase patch to this > issue? > > > > Or should I just bring on the tip-version of the single > > webcrypto_impl_unittest.cc file and work on that? > > I chose the latter, please see Path #7. Is this what you had in mind? Master rebases are (generally) fine, and the preferred flow is: 1) Upload a CL 2) Get feedback 3) Make changes to feedback 4) Upload modified CL addressing feedback 5) Rebase 6) Upload modified CL containing *just* rebase That allows the reviewer to look at the inter-diff between the reviewed patchset and the changed patchset, seeing what was changed in response to feedback, but they can also look at the *latest* patchset and see the change in full (eg: what will be committed)
On 2013/09/23 23:21:52, Ryan Sleevi wrote: > On 2013/09/23 23:17:24, padolph wrote: > > On 2013/09/23 22:08:31, padolph wrote: > > > > https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gypi > > > File content/content_tests.gypi (left): > > > > > > > > > https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gyp... > > > content/content_tests.gypi:784: 'renderer/webcrypto_impl_unittest.cc', > > > On 2013/09/23 21:45:51, Bryan Eyler wrote: > > > > Removal of this check is correct, but my guess is that the unit tests will > > > fail > > > > if this is submitted at the moment (and probably results in try failures) > > > > because there are unit tests in webcrypto_impl_unittest.cc that test > > > > functionality beyond what you've implemented in this CL. > > > > > > > > Also add an #ifdef in webcrypto_impl_unittest.cc to prevent these tests > from > > > > running. > > > > > > The workflow to do what you suggest is not clear to me. When I initially > > started > > > this issue, there was only the single DigestSampleSets test. I think I would > > > need to rebase this issue to tip-tree to get the latest changes to > > > webcrypto_impl_unittest.cc and then apply the #ifdef. But I thought master > > > rebases are discouraged in the Reitveld review workflow, because doing so > will > > > bring in a ton of changes. Should I go ahead an do a rebase patch to this > > issue? > > > > > > Or should I just bring on the tip-version of the single > > > webcrypto_impl_unittest.cc file and work on that? > > > > I chose the latter, please see Path #7. Is this what you had in mind? > > Master rebases are (generally) fine, and the preferred flow is: > > 1) Upload a CL > 2) Get feedback > 3) Make changes to feedback > 4) Upload modified CL addressing feedback > 5) Rebase > 6) Upload modified CL containing *just* rebase > > That allows the reviewer to look at the inter-diff between the reviewed patchset > and the changed patchset, seeing what was changed in response to feedback, but > they can also look at the *latest* patchset and see the change in full (eg: what > will be committed) Done.
https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gypi File content/content_tests.gypi (left): https://codereview.chromium.org/23621050/diff/18001/content/content_tests.gyp... content/content_tests.gypi:784: 'renderer/webcrypto_impl_unittest.cc', On 2013/09/23 22:08:31, padolph wrote: > On 2013/09/23 21:45:51, Bryan Eyler wrote: > > Removal of this check is correct, but my guess is that the unit tests will > fail > > if this is submitted at the moment (and probably results in try failures) > > because there are unit tests in webcrypto_impl_unittest.cc that test > > functionality beyond what you've implemented in this CL. > > > > Also add an #ifdef in webcrypto_impl_unittest.cc to prevent these tests from > > running. > > The workflow to do what you suggest is not clear to me. When I initially started > this issue, there was only the single DigestSampleSets test. I think I would > need to rebase this issue to tip-tree to get the latest changes to > webcrypto_impl_unittest.cc and then apply the #ifdef. But I thought master > rebases are discouraged in the Reitveld review workflow, because doing so will > bring in a ton of changes. Should I go ahead an do a rebase patch to this issue? > > Or should I just bring on the tip-version of the single > webcrypto_impl_unittest.cc file and work on that? Done.
lgtm https://codereview.chromium.org/23621050/diff/166001/content/renderer/webcryp... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/166001/content/renderer/webcryp... content/renderer/webcrypto_impl_openssl.cc:59: if (hash_expected_size <= 0) { Would this be better as a DCHECK, since this shouldn't be <=0 under proper operating conditions?
https://codereview.chromium.org/23621050/diff/166001/content/renderer/webcryp... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23621050/diff/166001/content/renderer/webcryp... content/renderer/webcrypto_impl_openssl.cc:59: if (hash_expected_size <= 0) { On 2013/09/25 22:33:05, Bryan Eyler wrote: > Would this be better as a DCHECK, since this shouldn't be <=0 under proper > operating conditions? Probably, but I'm not sure we can define exact proper operating conditions at this stage, since digest_context contents has been changed by the previous two EVP_ calls. There might be some combination of EVP inputs that pass the first two calls but cause EVP_MD_CTX_size() to fail.
I sent this patch to the tryserver, the results should be visible above. It looks like the <vector> implementation for android does not support .data() so we will have to modify the tests a bit ../../content/renderer/webcrypto_impl_unittest.cc:61:5:error: 'class std::vector<unsigned char>' has no member named 'data'
(I will take care of the .data() problem)
@jochen: could you approve for content/content_tests.gypi ? (Paul is now in the signers list). Thanks
Paul: please rebase the changelist as these files have now moved. Thanks!
On 2013/09/27 08:29:00, eroman wrote: > Paul: please rebase the changelist as these files have now moved. Thanks! Done.
All my CL's are rebased now. On Fri, Sep 27, 2013 at 1:29 AM, <eroman@chromium.org> wrote: > Paul: please rebase the changelist as these files have now moved. Thanks! > > https://codereview.chromium.**org/23621050/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/09/28 01:26:58, padolph wrote: > All my CL's are rebased now. > > > On Fri, Sep 27, 2013 at 1:29 AM, <mailto:eroman@chromium.org> wrote: > > > Paul: please rebase the changelist as these files have now moved. Thanks! > > > > > https://codereview.chromium.**org/23621050/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hi Paul, The content_tests.gypi changes will need to rebased (probably best to wait until that CL lands?), since that's being committed as part of your other CL. The unit tests have also moved to webcrypto/, so your changes there will need to be rebased as well.
On 2013/09/30 23:52:00, Bryan Eyler wrote: > Hi Paul, > > The content_tests.gypi changes will need to rebased (probably best to wait until > that CL lands?), since that's being committed as part of your other CL. > > The unit tests have also moved to webcrypto/, so your changes there will need to > be rebased as well. Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/23621050/205001
Message was sent while issue was closed.
Change committed as 227175 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
