Bug 188250 – Use WTF::Variant for WebPreferencesStore::Value

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use WTF::Variant for WebPreferencesStore::Value

[Alex Christensen]

Created attachment 346358 [details]
Patch

[Alex Christensen]

Created attachment 346393 [details]
Patch

[Alex Christensen]

Created attachment 346394 [details]
Patch

[Alex Christensen]

Created attachment 346395 [details]
Patch

[Sam Weinig]

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 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]

I committed the WTF part in http://trac.webkit.org/r234514

[Alex Christensen]

(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]

Created attachment 346418 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r234516

[Radar WebKit Bug Importer]

<rdar://problem/42874231>

[Alex Christensen]

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 on attachment 346418 [details]
Patch

Not new to the patch: this code seems to misspell "overridden" repeatedly.