Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1981)

Issue 24237013: Refactor webcrypto unittests. (Closed)

Created:
7 years, 3 months ago by eroman
Modified:
7 years, 2 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Visibility:
Public.

Description

Refactor webcrypto unittests. Extracts some helper functions which will be re-used for AES-CBC tests. Notably, importing of secret keys and comparison of arraybuffer against hex string. BUG=245025 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225365

Patch Set 1 #

Total comments: 3

Patch Set 2 : switch back to ARRAYSIZE_UNSAFE #

Total comments: 2

Patch Set 3 : Remove multiple inheritance and "using namespace WebKit" #

Total comments: 12

Patch Set 4 : address sleevi comment #

Total comments: 4

Patch Set 5 : Use forwarding #

Total comments: 1

Patch Set 6 : remove unneeded header #

Patch Set 7 : remove comment #

Patch Set 8 : moar ASSERT #

Patch Set 9 : fix forwarder #

Total comments: 1

Patch Set 10 : change to preincrement #

Patch Set 11 : Rebase onto master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+173 lines, -150 lines) Patch
M content/renderer/webcrypto_impl.h View 1 2 3 4 5 2 chunks +1 line, -3 lines 0 comments Download
M content/renderer/webcrypto_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +172 lines, -147 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
eroman
(Split this out from my upcoming AES-CBC changelist for simplicity) https://codereview.chromium.org/24237013/diff/1/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/1/content/renderer/webcrypto_impl_unittest.cc#newcode19 ...
7 years, 3 months ago (2013-09-21 22:23:44 UTC) #1
Bryan Eyler
lgtm https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto_impl_unittest.cc#newcode43 content/renderer/webcrypto_impl_unittest.cc:43: EXPECT_TRUE(handle.get()); Does the WebCryptoKey interface handle a null ...
7 years, 3 months ago (2013-09-23 20:04:50 UTC) #2
eroman
https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto_impl_unittest.cc#newcode43 content/renderer/webcrypto_impl_unittest.cc:43: EXPECT_TRUE(handle.get()); On 2013/09/23 20:04:51, Bryan Eyler wrote: > Does ...
7 years, 3 months ago (2013-09-23 20:11:22 UTC) #3
eroman
I have updated this changelist after conversation with rsleevi: * No longer using multiple inheritance ...
7 years, 3 months ago (2013-09-24 00:58:55 UTC) #4
Ryan Sleevi
https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypto_impl.h File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypto_impl.h#newcode44 content/renderer/webcrypto_impl.h:44: FRIEND_TEST_ALL_PREFIXES(WebCryptoImplTest, HMACSampleSets); nit: these two are no longer needed, ...
7 years, 3 months ago (2013-09-24 01:17:11 UTC) #5
eroman
https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypto_impl.h File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypto_impl.h#newcode44 content/renderer/webcrypto_impl.h:44: FRIEND_TEST_ALL_PREFIXES(WebCryptoImplTest, HMACSampleSets); On 2013/09/24 01:17:11, Ryan Sleevi wrote: > ...
7 years, 3 months ago (2013-09-24 01:26:41 UTC) #6
eroman
rsleevi: I added forwarding implementations in the test fixture per our discussion. Let me know ...
7 years, 3 months ago (2013-09-24 01:42:37 UTC) #7
Ryan Sleevi
https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_impl_unittest.cc#newcode32 content/renderer/webcrypto_impl_unittest.cc:32: array_buffer.data(), array_buffer.byteLength()).c_str()); if the operation fails, and |array_buffer| is ...
7 years, 3 months ago (2013-09-24 01:47:12 UTC) #8
eroman
https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_impl_unittest.cc#newcode70 content/renderer/webcrypto_impl_unittest.cc:70: EXPECT_TRUE(handle.get()); On 2013/09/24 01:47:13, Ryan Sleevi wrote: > This ...
7 years, 3 months ago (2013-09-24 02:13:15 UTC) #9
eroman
Anything else you want addressed? https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_impl_unittest.cc#newcode32 content/renderer/webcrypto_impl_unittest.cc:32: array_buffer.data(), array_buffer.byteLength()).c_str()); On 2013/09/24 ...
7 years, 3 months ago (2013-09-24 22:42:43 UTC) #10
Ryan Sleevi
lgtm https://codereview.chromium.org/24237013/diff/36001/content/renderer/webcrypto_impl_unittest.cc File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/36001/content/renderer/webcrypto_impl_unittest.cc#newcode282 content/renderer/webcrypto_impl_unittest.cc:282: test_index++) { nit: prefer pre-increment (more of a ...
7 years, 3 months ago (2013-09-24 23:11:17 UTC) #11
eroman
jamesr: Please review for content/renderer approval
7 years, 3 months ago (2013-09-24 23:29:30 UTC) #12
eroman
jamesr or jochen for content/renderer approval.
7 years, 2 months ago (2013-09-25 20:47:06 UTC) #13
jamesr
Stamp lgtm, although please fix "unnittests" -> "unittests" typo in description
7 years, 2 months ago (2013-09-25 21:30:19 UTC) #14
eroman
Thanks! Fixed the description.
7 years, 2 months ago (2013-09-25 21:53:50 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/24237013/44001
7 years, 2 months ago (2013-09-25 22:14:39 UTC) #16
commit-bot: I haz the power
7 years, 2 months ago (2013-09-26 06:47:16 UTC) #17
Message was sent while issue was closed.
Change committed as 225365

Powered by Google App Engine
This is Rietveld 408576698