Bug 155008

Summary: Move CryptoDigest to WebCore/platform
Product: WebKit Reporter: Daniel Bates <dbates>
Component: PlatformAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, bfulgham, cgarcia, commit-queue, mcatanzaro, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=155073
https://bugs.webkit.org/show_bug.cgi?id=162913
Bug Depends on:    
Bug Blocks: 155007    
Attachments:
Description Flags
Patch none

Daniel Bates
Reported 2016-03-03 20:53:12 PST
Towards making use of CryptoDigest in the implementation of Content Security Policy inline script and inline stylesheet hashes (bug #155007) we should move CryptoDigest from Source/WebCore/crypto to a place that conveys that it is not specific to the Web Crypto code. One idea is to move it to WebCore/platform/crypto.
Attachments
Patch (43.19 KB, patch)
2016-03-03 21:11 PST, Daniel Bates 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-03-03 20:53:33 PST
<rdar://problem/24969787>
Daniel Bates
Comment 2 2016-03-03 21:11:59 PST
Created attachment 272830 [details] Patch
WebKit Commit Bot
Comment 3 2016-03-03 21:14:15 PST
Attachment 272830 [details] did not pass style-queue: ERROR: Source/WebCore/platform/crypto/gnutls/CryptoDigestGnuTLS.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/crypto/CryptoDigest.h:40: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/crypto/CryptoDigest.h:41: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/crypto/CryptoDigest.h:42: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/crypto/CryptoDigest.h:43: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/crypto/CryptoDigest.h:44: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 6 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brent Fulgham
Comment 4 2016-03-03 21:18:58 PST
Comment on attachment 272830 [details] Patch r=me. Please confirm tests pass before landing.
Daniel Bates
Comment 5 2016-03-04 11:26:13 PST
Comment on attachment 272830 [details] Patch Clearing flags on attachment: 272830 Committed r197575: <http://trac.webkit.org/changeset/197575>
Daniel Bates
Comment 6 2016-03-04 11:26:17 PST
All reviewed patches have been landed. Closing bug.
Michael Catanzaro
Comment 7 2016-03-05 16:32:45 PST
It broke the GTK build unless you pass -DENABLE_SUBTLE_CRYPTO=ON to CMake to enable that experimental option, but you couldn't have noticed because this is what the bots do. Follow-up in bug #155073. At first I considered rolling this out, because it adds a "new" dependency on GnuTLS. (We normally make new dependencies optional by allowing users to disable the corresponding new feature.) But, looking over CryptoDigestGnuTLS.cpp, I believe all the GnuTLS functionality it requires is fairly old, so it's unlikely to cause problems for our distributors in practice beyond one failed build to discover the new dependency. So I think a required dependency on GnuTLS is fine, we just need to remember to mention this in our NEWS. I'm testing a build fix now.
Michael Catanzaro
Comment 8 2016-03-05 17:20:55 PST
Committed r197623: <http://trac.webkit.org/changeset/197623>
Michael Catanzaro
Comment 9 2016-10-04 11:06:35 PDT
(In reply to comment #7) > At first I considered rolling this out, because it adds a "new" dependency > on GnuTLS. (We normally make new dependencies optional by allowing users to > disable the corresponding new feature.) But, looking over > CryptoDigestGnuTLS.cpp, I believe all the GnuTLS functionality it requires > is fairly old, so it's unlikely to cause problems for our distributors in > practice beyond one failed build to discover the new dependency. So I think > a required dependency on GnuTLS is fine, we just need to remember to mention > this in our NEWS. Turns out this introduced a legal problem, see bug #162913.
Daniel Bates
Comment 10 2016-10-05 20:33:14 PDT
(In reply to comment #9) > (In reply to comment #7) > > At first I considered rolling this out, because it adds a "new" dependency > > on GnuTLS. (We normally make new dependencies optional by allowing users to > > disable the corresponding new feature.) But, looking over > > CryptoDigestGnuTLS.cpp, I believe all the GnuTLS functionality it requires > > is fairly old, so it's unlikely to cause problems for our distributors in > > practice beyond one failed build to discover the new dependency. So I think > > a required dependency on GnuTLS is fine, we just need to remember to mention > > this in our NEWS. > > Turns out this introduced a legal problem, see bug #162913. :(
Michael Catanzaro
Comment 11 2016-10-06 06:25:26 PDT
(In reply to comment #10) > :( Turns out I misunderstood the licenses. It's OK. We'll still try to remove the dependency.
Note You need to log in before you can comment on or make changes to this bug.