Bug 181522 - [WebAuthN] Import a CBOR coder from Chromium
Summary: [WebAuthN] Import a CBOR coder from Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency tree / graph
 
Reported: 2018-01-10 22:31 PST by Jiewen Tan
Modified: 2018-01-22 13:26 PST (History)
6 users (show)

See Also:

Attachments
Patch (132.00 KB, patch)
2018-01-10 23:10 PST, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (129.93 KB, patch)
2018-01-10 23:15 PST, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (126.04 KB, patch)
2018-01-11 17:43 PST, Jiewen Tan 		bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (126.15 KB, patch)
2018-01-11 19:34 PST, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-01-10 22:31:28 PST 
Import a CBOR coder from Chromium.
Comment 1 Jiewen Tan 2018-01-10 22:31:59 PST 
<rdar://problem/36055729>
Comment 2 Jiewen Tan 2018-01-10 23:10:33 PST 
Created attachment 331034 [details]
Patch
Comment 3 Jiewen Tan 2018-01-10 23:15:15 PST 
Created attachment 331035 [details]
Patch
Comment 4 EWS Watchlist 2018-01-10 23:18:43 PST Comment hidden (obsolete)
Comment 5 youenn fablet 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.
Comment 6 Brent Fulgham 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.
Comment 7 Jiewen Tan 2018-01-11 17:43:54 PST 
Created attachment 331146 [details]
Patch
Comment 8 EWS Watchlist 2018-01-11 17:46:58 PST Comment hidden (obsolete)
Comment 9 Brent Fulgham 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 **
Comment 10 Brent Fulgham 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>?
Comment 11 Brent Fulgham 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'
Comment 12 Brent Fulgham 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
Comment 13 Jiewen Tan 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.
Comment 14 Jiewen Tan 2018-01-11 19:34:16 PST 
Created attachment 331158 [details]
Patch for landing
Comment 15 EWS Watchlist 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.
Comment 16 Jiewen Tan 2018-01-11 21:05:08 PST Comment hidden (obsolete)
Comment 17 Jiewen Tan 2018-01-11 21:05:16 PST 
The mac EWS bots looks quite dead. rq+ regardless.
Comment 18 WebKit Commit Bot 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>