[refactor] Move webcrypto test data to separate files.

[refactor] Move webcrypto test data to separate files.

BUG=245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248637

[eroman, 2014-01-25 06:51:36]

I will move over the other tests as followup changes if this direction is agreed
upon.

[Ryan Sleevi, 2014-01-28 20:37:20]

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:29: #include
"third_party/re2/re2/re2.h"
I don't think this sort ordering is correct

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:99:
::testing::AssertionResult ReadJsonTestFile(
Document

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:113: << file_path.value();
nit: align

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:119: LOG(ERROR) <<
file_contents;
Debug spam?

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:131:
::testing::AssertionResult ReadJsonTestFileToList(
Document

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:2158:
ASSERT_TRUE(ReadJsonTestFileToList("pkcs1v15sign.json", &tests));
nit: Add some form of word separator

pkcs1v15_sign.json ?

[eroman, 2014-01-29 03:26:53]

Converted the remainder of the sample vectors (all the remains now is some key
bytes to move out)

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:29: #include
"third_party/re2/re2/re2.h"
On 2014/01/28 20:37:21, Ryan Sleevi wrote:
> I don't think this sort ordering is correct

I believe this is correct -- it is in lexicographic order 'r' > 'W'

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:99:
::testing::AssertionResult ReadJsonTestFile(
On 2014/01/28 20:37:21, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:113: << file_path.value();
On 2014/01/28 20:37:21, Ryan Sleevi wrote:
> nit: align

Done.

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:119: LOG(ERROR) <<
file_contents;
On 2014/01/28 20:37:21, Ryan Sleevi wrote:
> Debug spam?

Oops! Removed.

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:131:
::testing::AssertionResult ReadJsonTestFileToList(
On 2014/01/28 20:37:21, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/147613002/diff/1/content/renderer/webcrypto/w...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:2158:
ASSERT_TRUE(ReadJsonTestFileToList("pkcs1v15sign.json", &tests));
On 2014/01/28 20:37:21, Ryan Sleevi wrote:
> nit: Add some form of word separator
> 
> pkcs1v15_sign.json ?

The name was inspired by the ftp:// URL (but interestingly I changed it from
even that...)

Done.

[Ryan Sleevi, 2014-01-31 20:53:35]

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:191: } kDigestNameToId[] =
{
make this struct const/static too?

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:638: for (size_t
test_index = 0; test_index < tests->GetSize(); ++test_index) {
So, this is valid code, so consider this a question about "performance" /
"style"

you alternatively could write this as:

size_t i = 0;
for (base::ListValue::const_iterator it = tests->begin();
     it != tests->end(); ++it) {
  SCOPED_TRACE(++i);
  base::DictionaryValue* test;
  ASSERT_TRUE(it->GetAsDictionary(&test));
}

The benefit here is you avoid some 'extra overhead' of Get/GetAsDictionary. Up
to you though.

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:811:
ASSERT_TRUE(ReadJsonTestFileToList("aescbc.json", &tests));
aes_cbc?

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:813: for (size_t index =
0; index < tests->GetSize(); index++) {
++index

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:1625:
ASSERT_TRUE(ReadJsonTestFile("rsaes.json", &json));
rsa_es

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:1915: for (size_t idx = 0;
idx < tests->GetSize(); ++idx) {
naming: idx -> index? matches consistency with the other tests

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:2041:
ASSERT_TRUE(ReadJsonTestFileToList("aesgcm.json", &tests));
aes_gcm

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:2044: for (size_t index =
0; index < tests->GetSize(); index++) {
++index

https://codereview.chromium.org/147613002/diff/60001/content/test/data/webcry...
File content/test/data/webcrypto/digest.json (right):

https://codereview.chromium.org/147613002/diff/60001/content/test/data/webcry...
content/test/data/webcrypto/digest.json:12: { "algorithm": "sha-224", "input":
"",
line break before input

[eroman, 2014-01-31 22:15:32]

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:638: for (size_t
test_index = 0; test_index < tests->GetSize(); ++test_index) {
On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> So, this is valid code, so consider this a question about "performance" /
> "style"
> 
> you alternatively could write this as:
> 
> size_t i = 0;
> for (base::ListValue::const_iterator it = tests->begin();
>      it != tests->end(); ++it) {
>   SCOPED_TRACE(++i);
>   base::DictionaryValue* test;
>   ASSERT_TRUE(it->GetAsDictionary(&test));
> }
> 
> The benefit here is you avoid some 'extra overhead' of Get/GetAsDictionary. Up
> to you though.

Not sure I understand this argument from a "performance" standpoint. 

The amount of work is for all intents and purposes identical. The only
difference is the Get() approach using an index does a bounds check on the
index, whereas iterators do not.
Given that I am reading in a JSON file from disk, doing a regexp substitution
over it, and parsing it, I think this considerations is insignificant.

From a style perspective, I comes down to whether one prefers incrementing an
integer index, or incrementing an iterator class. I don't really care either way
here, and would be happy to entertain this as a style follow-up if you have a
strong preference.

[eroman, 2014-01-31 22:55:11]

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:813: for (size_t index =
0; index < tests->GetSize(); index++) {
On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> ++index

Done.

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:1915: for (size_t idx = 0;
idx < tests->GetSize(); ++idx) {
On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> naming: idx -> index? matches consistency with the other tests

Done (test_index throughout)

https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:2044: for (size_t index =
0; index < tests->GetSize(); index++) {
On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> ++index

Done.

https://codereview.chromium.org/147613002/diff/60001/content/test/data/webcry...
File content/test/data/webcrypto/digest.json (right):

https://codereview.chromium.org/147613002/diff/60001/content/test/data/webcry...
content/test/data/webcrypto/digest.json:12: { "algorithm": "sha-224", "input":
"",
On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> line break before input

Done.

[Ryan Sleevi, 2014-01-31 23:18:49]

On 2014/01/31 22:55:11, eroman wrote:
>
https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
> File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
> content/renderer/webcrypto/webcrypto_impl_unittest.cc:813: for (size_t index =
> 0; index < tests->GetSize(); index++) {
> On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> > ++index
> 
> Done.
> 
>
https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
> content/renderer/webcrypto/webcrypto_impl_unittest.cc:1915: for (size_t idx =
0;
> idx < tests->GetSize(); ++idx) {
> On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> > naming: idx -> index? matches consistency with the other tests
> 
> Done (test_index throughout)
> 
>
https://codereview.chromium.org/147613002/diff/60001/content/renderer/webcryp...
> content/renderer/webcrypto/webcrypto_impl_unittest.cc:2044: for (size_t index
=
> 0; index < tests->GetSize(); index++) {
> On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> > ++index
> 
> Done.
> 
>
https://codereview.chromium.org/147613002/diff/60001/content/test/data/webcry...
> File content/test/data/webcrypto/digest.json (right):
> 
>
https://codereview.chromium.org/147613002/diff/60001/content/test/data/webcry...
> content/test/data/webcrypto/digest.json:12: { "algorithm": "sha-224", "input":
> "",
> On 2014/01/31 20:53:36, Ryan Sleevi wrote:
> > line break before input
> 
> Done.

In the future, please don't rebase and address comments in the same patchset. It
makes viewing inter-diffs... "difficult"

[Ryan Sleevi, 2014-01-31 23:19:26]

LGTM, assuming owners are happy with the RE2 dep.

[eroman, 2014-01-31 23:35:01]

+darin for content/DEPS approval:

I have added a dependency on re2 from content_unittests. Unfortunately in the
DEPS file I turn it on for all of content, is there a way to better scope that?

The dependency on re2 is for stripping some lines from test data. I can also
code that part up manually and remove the dependency if it is a concern.

Thanks!

[darin (slow to review), 2014-02-03 15:44:04]

OK, LGTM for content/DEPS

[eroman, 2014-02-03 22:24:42]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-02-03 22:29:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/147613002/280001

[commit-bot: I haz the power, 2014-02-04 00:48:49]

Change committed as 248637

[commit-bot: I haz the power, 2014-02-04 00:48:52]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:48:53]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:48:53]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:48:56]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:48:58]

CQ bit was unchecked on CL. Ignoring.