Issue 1461703009: Switch //components from vector_as_array to vector::data.

Issue 1461703009: Switch //components from vector_as_array to vector::data. (Closed)

Created:
5 years, 1 month ago by davidben

Modified:
5 years, 1 month ago

Reviewers:
eroman, Cait (Slow)

CC:
chromium-reviews, Peter Beverloo, zea+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Switch //components from vector_as_array to vector::data. BUG=556678 Committed: https://crrev.com/aa62f3810eb5206d488e0dfdba3826cc0b3941dd Cr-Commit-Position: refs/heads/master@{#360926}

Patch Set 3 : eroman comment #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+70 lines, -117 lines)			Patch
M	components/gcm_driver/crypto/p256_key_util.cc	View		2 chunks	+3 lines, -5 lines	0 comments	Download
M	components/html_viewer/blink_platform_impl.cc	View		2 chunks	+1 line, -3 lines	0 comments	Download
M	components/ownership/owner_key_util.h	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	components/ownership/owner_key_util_impl.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	components/ownership/owner_key_util_impl_unittest.cc	View		2 chunks	+3 lines, -5 lines	0 comments	Download
M	components/webcrypto/algorithms/aes_cbc.cc	View		2 chunks	+3 lines, -6 lines	0 comments	Download
M	components/webcrypto/algorithms/aes_cbc_unittest.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	components/webcrypto/algorithms/aes_ctr.cc	View		5 chunks	+4 lines, -7 lines	0 comments	Download
M	components/webcrypto/algorithms/aes_ctr_unittest.cc	View		3 chunks	+3 lines, -4 lines	0 comments	Download
M	components/webcrypto/algorithms/aes_gcm_unittest.cc	View		2 chunks	+2 lines, -3 lines	0 comments	Download
M	components/webcrypto/algorithms/ec.cc	View		2 chunks	+1 line, -4 lines	0 comments	Download
M	components/webcrypto/algorithms/ecdh.cc	View		2 chunks	+2 lines, -4 lines	0 comments	Download
M	components/webcrypto/algorithms/ecdsa.cc	View	1 2	6 chunks	+5 lines, -11 lines	0 comments	Download
M	components/webcrypto/algorithms/hkdf.cc	View		2 chunks	+4 lines, -5 lines	0 comments	Download
M	components/webcrypto/algorithms/hmac.cc	View		3 chunks	+7 lines, -9 lines	0 comments	Download
M	components/webcrypto/algorithms/hmac_unittest.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	components/webcrypto/algorithms/pbkdf2.cc	View		1 chunk	+3 lines, -4 lines	0 comments	Download
M	components/webcrypto/algorithms/rsa.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	components/webcrypto/algorithms/rsa_oaep.cc	View		2 chunks	+2 lines, -3 lines	0 comments	Download
M	components/webcrypto/algorithms/rsa_oaep_unittest.cc	View		3 chunks	+2 lines, -3 lines	0 comments	Download
M	components/webcrypto/algorithms/rsa_sign.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	components/webcrypto/algorithms/rsa_ssa_unittest.cc	View	1	2 chunks	+1 line, -2 lines	0 comments	Download
M	components/webcrypto/algorithms/secret_key_util.cc	View		2 chunks	+2 lines, -3 lines	0 comments	Download
M	components/webcrypto/algorithms/sha.cc	View		2 chunks	+1 line, -3 lines	0 comments	Download
M	components/webcrypto/algorithms/sha_unittest.cc	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	components/webcrypto/algorithms/test_helpers.cc	View		3 chunks	+6 lines, -7 lines	0 comments	Download
M	components/webcrypto/algorithms/util.cc	View		4 chunks	+4 lines, -5 lines	0 comments	Download
M	components/webcrypto/crypto_data.cc	View		2 chunks	+2 lines, -4 lines	0 comments	Download
M	components/webcrypto/webcrypto_impl.cc	View		3 chunks	+2 lines, -3 lines	0 comments	Download

Messages

Total messages: 14 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

davidben

eroman: components/webcrypto caitkp: components/ except for webcrypto. https://codereview.chromium.org/1461703009/diff/20001/components/webcrypto/algorithms/ecdsa.cc File components/webcrypto/algorithms/ecdsa.cc (right): https://codereview.chromium.org/1461703009/diff/20001/components/webcrypto/algorithms/ecdsa.cc#newcode69 components/webcrypto/algorithms/ecdsa.cc:69: ECDSA_SIG_from_bytes(signature->data(), signature->size())); ...

5 years, 1 month ago (2015-11-19 23:57:21 UTC) #2

eroman

lgtm https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/crypto/p256_key_util.cc File components/gcm_driver/crypto/p256_key_util.cc (right): https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/crypto/p256_key_util.cc#newcode74 components/gcm_driver/crypto/p256_key_util.cc:74: out_private_key->assign(reinterpret_cast<const char*>(private_key.data()), Any reason not to use: out_private_key->assign(private_key.begin(), ...

5 years, 1 month ago (2015-11-20 00:35:14 UTC) #3

davidben

https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/crypto/p256_key_util.cc File components/gcm_driver/crypto/p256_key_util.cc (right): https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/crypto/p256_key_util.cc#newcode74 components/gcm_driver/crypto/p256_key_util.cc:74: out_private_key->assign(reinterpret_cast<const char*>(private_key.data()), On 2015/11/20 00:35:14, eroman wrote: > Any ...

5 years, 1 month ago (2015-11-20 19:12:50 UTC) #5

https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/c...
File components/gcm_driver/crypto/p256_key_util.cc (right):

https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/p256_key_util.cc:74:
out_private_key->assign(reinterpret_cast<const char*>(private_key.data()),
On 2015/11/20 00:35:14, eroman wrote:
> Any reason not to use:
>   out_private_key->assign(private_key.begin(), private_key.end());
> 
> I tried locally and it didn't complain about the signed/unsigned conversion,
but
> might be MSVC does, dunno.

That works? Hrm. We do this (ugly) reinterpret_cast thing everywhere, so let's
experiment with that separately. I could imagine dumb things happen like, I
dunno, the iterator version of assign being written to not assume you can
compare the iterators for size.

https://codereview.chromium.org/1461703009/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/p256_key_util.cc:77: reinterpret_cast<const
char*>(public_key_x509.data()),
On 2015/11/20 00:35:14, eroman wrote:
> same question throughout.

Acknowledged.

https://codereview.chromium.org/1461703009/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/ecdsa.cc (right):

https://codereview.chromium.org/1461703009/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/ecdsa.cc:246: *signature_match =
!!EVP_DigestVerifyFinal(ctx.get(), der_signature.data(),
On 2015/11/20 00:35:14, eroman wrote:
> tbh i don't find !! more readable than 1==

Switched back.

https://codereview.chromium.org/1461703009/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/rsa_oaep_unittest.cc (right):

https://codereview.chromium.org/1461703009/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/rsa_oaep_unittest.cc:35:
base::StringPiece(reinterpret_cast<const char*>(input.data()),
On 2015/11/20 00:35:14, eroman wrote:
> side note: wonder if we should have an automatic thing for building
StringPiece
> given std::vector of bytes. 
> Or maybe more generally any std container whose valuetype is 1 byte large.

Yeah, we certainly need something. Doing these reinterpret_casts is awful. A
uint8_t StringPiece is an option.

I'll never forgive C for making char signed[*]. That was a horrible idea.

[*] It's not even signed. It's implementation-specific whether it's signed or
unsigned!

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461703009/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461703009/40001

5 years, 1 month ago (2015-11-20 19:15:33 UTC) #8

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/144064)

5 years, 1 month ago (2015-11-20 20:39:10 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461703009/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461703009/40001

5 years, 1 month ago (2015-11-20 21:18:36 UTC) #12

commit-bot: I haz the power

5 years, 1 month ago (2015-11-20 23:03:28 UTC) #14

Message was sent while issue was closed.

Patchset 3 (id:??) landed as
https://crrev.com/aa62f3810eb5206d488e0dfdba3826cc0b3941dd
Cr-Commit-Position: refs/heads/master@{#360926}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages