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

Description Alex Christensen 2018-08-01 21:43:56 PDT
Use WTF::Variant for WebPreferencesStore::Value
Comment 1 Alex Christensen 2018-08-01 21:55:47 PDT
Created attachment 346358 [details]
Patch
Comment 2 Alex Christensen 2018-08-02 11:06:28 PDT
Created attachment 346393 [details]
Patch
Comment 3 Alex Christensen 2018-08-02 11:09:59 PDT
Created attachment 346394 [details]
Patch
Comment 4 Alex Christensen 2018-08-02 11:11:57 PDT
Created attachment 346395 [details]
Patch
Comment 5 Sam Weinig 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.
Comment 6 Sam Weinig 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.
Comment 7 Alex Christensen 2018-08-02 13:25:21 PDT
I committed the WTF part in http://trac.webkit.org/r234514
Comment 8 Alex Christensen 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
Comment 9 Alex Christensen 2018-08-02 14:26:43 PDT
Created attachment 346418 [details]
Patch
Comment 10 Alex Christensen 2018-08-02 14:48:34 PDT
http://trac.webkit.org/r234516
Comment 11 Radar WebKit Bug Importer 2018-08-02 14:49:18 PDT
<rdar://problem/42874231>
Comment 12 Alex Christensen 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.
Comment 13 Darin Adler 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.