Bug 188250 - Use WTF::Variant for WebPreferencesStore::Value
Summary: Use WTF::Variant for WebPreferencesStore::Value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-01 21:43 PDT by Alex Christensen
Modified: 2018-08-03 08:09 PDT (History)
8 users (show)

See Also:

Attachments
Patch (18.42 KB, patch)
2018-08-01 21:55 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (14.83 KB, patch)
2018-08-02 11:06 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (14.77 KB, patch)
2018-08-02 11:09 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2018-08-02 11:11 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (16.16 KB, patch)
2018-08-02 14:26 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) 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 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.