Bug 188250

Summary: Use WTF::Variant for WebPreferencesStore::Value
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, dbates, ews-watchlist, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Alex Christensen
Reported 2018-08-01 21:43:56 PDT
Use WTF::Variant for WebPreferencesStore::Value
Attachments
Patch (18.42 KB, patch)
2018-08-01 21:55 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (14.83 KB, patch)
2018-08-02 11:06 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (14.77 KB, patch)
2018-08-02 11:09 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (14.76 KB, patch)
2018-08-02 11:11 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Patch (16.16 KB, patch)
2018-08-02 14:26 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 2018-08-01 21:55:47 PDT
Created attachment 346358 [details] Patch
Alex Christensen
Comment 2 2018-08-02 11:06:28 PDT
Created attachment 346393 [details] Patch
Alex Christensen
Comment 3 2018-08-02 11:09:59 PDT
Created attachment 346394 [details] Patch
Alex Christensen
Comment 4 2018-08-02 11:11:57 PDT
Created attachment 346395 [details] Patch
Sam Weinig
Comment 5 2018-08-02 12:06:04 PDT
Comment on attachment 346395 [details] Patch Looks good. I think this would be a great opportunity to add tests to TestWebKitAPI for ArgumentCoder. As you will see, the name TestWebKitAPI is a bit misleading, as we have many tests for WTF and WebCore types that aren't actually exposed as API, and are really just good old unit tests.
Sam Weinig
Comment 6 2018-08-02 12:07:08 PDT
Comment on attachment 346395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346395&action=review > Source/WebKit/ChangeLog:8 > + It's basically a Variant now. This just uses templates. This is not a very clear commit message. Please actually state what you did. Also, per-function information would be appreciated.
Alex Christensen
Comment 7 2018-08-02 13:25:21 PDT
I committed the WTF part in http://trac.webkit.org/r234514
Alex Christensen
Comment 8 2018-08-02 14:26:27 PDT
(In reply to Sam Weinig from comment #5) > Comment on attachment 346395 [details] > Patch > > Looks good. I think this would be a great opportunity to add tests to > TestWebKitAPI for ArgumentCoder. As you will see, the name TestWebKitAPI is > a bit misleading, as we have many tests for WTF and WebCore types that > aren't actually exposed as API, and are really just good old unit tests. This requires significant refactoring because we have checks that HAVE and PLATFORM macros aren't exported in WebKit.framework's headers, and Encoder/Decoder and dependencies use those macros. I filed https://bugs.webkit.org/show_bug.cgi?id=188278
Alex Christensen
Comment 9 2018-08-02 14:26:43 PDT
Created attachment 346418 [details] Patch
Alex Christensen
Comment 10 2018-08-02 14:48:34 PDT
http://trac.webkit.org/r234516
Radar WebKit Bug Importer
Comment 11 2018-08-02 14:49:18 PDT
<rdar://problem/42874231>
Alex Christensen
Comment 12 2018-08-02 15:32:17 PDT
Comment on attachment 346418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346418&action=review > Source/WebKit/Platform/IPC/ArgumentCoders.h:564 > + ASSERT(static_cast<unsigned>(variant.index()) == variant.index()); I removed this assertion in http://trac.webkit.org/r234519 because it broke some builds.
Darin Adler
Comment 13 2018-08-03 08:09:09 PDT
Comment on attachment 346418 [details] Patch Not new to the patch: this code seems to misspell "overridden" repeatedly.
Note You need to log in before you can comment on or make changes to this bug.