Bug 181522

Summary: [WebAuthN] Import a CBOR coder from Chromium
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, ews-watchlist, jiewen_tan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
bfulgham: review+, bfulgham: commit-queue-
Patch for landing none

Jiewen Tan
Reported 2018-01-10 22:31:28 PST
Import a CBOR coder from Chromium.
Attachments
Patch (132.00 KB, patch)
2018-01-10 23:10 PST, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (129.93 KB, patch)
2018-01-10 23:15 PST, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (126.04 KB, patch)
2018-01-11 17:43 PST, Jiewen Tan 		bfulgham: review+
bfulgham: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (126.15 KB, patch)
2018-01-11 19:34 PST, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jiewen Tan
Comment 1 2018-01-10 22:31:59 PST
<rdar://problem/36055729>
Jiewen Tan
Comment 2 2018-01-10 23:10:33 PST
Created attachment 331034 [details] Patch
Jiewen Tan
Comment 3 2018-01-10 23:15:15 PST
Created attachment 331035 [details] Patch
EWS Watchlist
Comment 4 2018-01-10 23:18:43 PST Comment hidden (obsolete)
youenn fablet
Comment 5 2018-01-11 02:22:21 PST
We usually import third party libraries in Source/ThirdParty. Would it be possible to do that and build a static library? I would also recommend to archive any change you are making as a separate file so that next time this library is imported, one can reapply the changes more easily.
Brent Fulgham
Comment 6 2018-01-11 08:43:42 PST
Comment on attachment 331035 [details] Patch I agree with Youenn about locating the sources in 'Source/ThirdParty/cbor'. I think you told me that you were able to build this as a stand-alone binary, so let's just make it a static library (like we do for libwebrtc). r- to make those changes.
Jiewen Tan
Comment 7 2018-01-11 17:43:54 PST
Created attachment 331146 [details] Patch
EWS Watchlist
Comment 8 2018-01-11 17:46:58 PST Comment hidden (obsolete)
Brent Fulgham
Comment 9 2018-01-11 19:17:28 PST
Last 500 characters of output: d long') [-Werror,-Wshorten-64-to-32] cborByteString.append(m_it, numBytes); ~~~~~~ ^~~~~~~~ 2 errors generated. ** BUILD FAILED **
Brent Fulgham
Comment 10 2018-01-11 19:22:28 PST
Comment on attachment 331146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331146&action=review This version looks good. Please fix the 32-bit issues before you land. > Source/WebCore/Modules/webauthn/cbor/CBORReader.cpp:35 > +#include <math.h> <cmath>?
Brent Fulgham
Comment 11 2018-01-11 19:23:40 PST
Note: wincairo fails due to this: c:\webkit-ews\webkit\source\webcore\modules\webauthn\cbor\CBORValue.h(44): error C2079: 'CBORValue' uses undefined class 'cbor::WEBCORE_TESTSUPPORT_EXPORT'
Brent Fulgham
Comment 12 2018-01-11 19:24:49 PST
Our own Windows build has th same error: c:\cygwin\home\buildbot\webkit\source\webcore\modules\webauthn\cbor\CBORValue.h(44): error C2079: 'CBORValue' uses undefined class 'cbor::WEBCORE_TESTSUPPORT_EXPORT' (compiling source file
Jiewen Tan
Comment 13 2018-01-11 19:32:10 PST
Comment on attachment 331146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331146&action=review Thanks Brent for r+ my patch. >> Source/WebCore/Modules/webauthn/cbor/CBORReader.cpp:35 >> +#include <math.h> > > <cmath>? Removed. Surprisingly, there is no dependences.
Jiewen Tan
Comment 14 2018-01-11 19:34:16 PST
Created attachment 331158 [details] Patch for landing
EWS Watchlist
Comment 15 2018-01-11 19:37:08 PST
Attachment 331158 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:60: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:68: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:76: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:95: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:104: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:118: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:127: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:293: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORValueTest.cpp:299: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:84: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:91: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:98: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:278: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:283: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:288: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:293: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:322: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:327: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:332: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:337: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WebCore/CBORReaderTest.cpp:693: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] Total errors found: 21 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jiewen Tan
Comment 16 2018-01-11 21:05:08 PST Comment hidden (obsolete)
Jiewen Tan
Comment 17 2018-01-11 21:05:16 PST
The mac EWS bots looks quite dead. rq+ regardless.
WebKit Commit Bot
Comment 18 2018-01-11 21:29:05 PST
Comment on attachment 331158 [details] Patch for landing Clearing flags on attachment: 331158 Committed r226862: <https://trac.webkit.org/changeset/226862>
Note You need to log in before you can comment on or make changes to this bug.