WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
155008
Move CryptoDigest to WebCore/platform
https://bugs.webkit.org/show_bug.cgi?id=155008
Summary
Move CryptoDigest to WebCore/platform
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
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug