Bug 207497

Created attachment 390285 [details]
patch

Could someone who works on linux get the linux build working with this change?

Created attachment 390312 [details]
sub-patch for WinCairo

Here's a diff for making WinCairo green.

One question that arose while doing this:
Is it okay that we don't write WTF::nullopt when we fail to decode?

What do you mean?  We usually try to use WTF::nullopt instead of { } because some people think { } is unclear.

(In reply to Alex Christensen from comment #4)
> What do you mean?  We usually try to use WTF::nullopt instead of { } because
> some people think { } is unclear.

Oh sorry, too terse. I meant "write (to the out param)". We can't (easily) recycle a variable when decoding repeatedly because the previous value will be preserved.

We are moving from this model:
1. Instantiating a default-constructed value
2. Decode members into the value
3. Return the value by reference

To this model:
1. Decode all members
2. Construct an object with all members

This avoids partially initialized objects and it allows non-default-constructible objects, so we can use UniqueRef and Ref in serialized types.  It's a slow move, but we'll get there any year now.

Created attachment 390353 [details]
Patch for soup

It builds with this patch, but it doesn't work.

(In reply to Carlos Garcia Campos from comment #7)
> Created attachment 390353 [details]
> Patch for soup
> 
> It builds with this patch, but it doesn't work.

Yeah, I should've mentioned the same for WinCairo. All relevant API tests fail, but this is probably not a platform-specific issue.

Comment on attachment 390285 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390285&action=review

> Source/WebCore/platform/network/ResourceRequestBase.h:363
>  }

How about idea using Optional only for non-default-constructable class?
How about idea adding error state to decoder as well as std::ios_base::iostate?
https://en.cppreference.com/w/cpp/io/ios_base/iostate

Then, ResourceRequestBase::decodeBase can be simplified like the following.

template<class Decoder>
ALWAYS_INLINE void ResourceRequestBase::decodeBase(Decoder& decoder)
{
    String firstPartyForCookies;

    decoder >> m_url;
    decoder >> m_timeoutInterval;
    decoder >> firstPartyForCookies;
    decoder >> m_httpMethod;
    decoder >> m_httpHeaderFields;
    decoder >> m_responseContentDispositionEncodingFallbackArray;
    decoder >> m_cachePolicy;
    decoder >> m_allowCookies;
    decoder >> m_sameSiteDisposition;
    decoder >> m_isTopSite;
    decoder >> m_priority;
    decoder >> m_requester;

    if (decoder.didFail)
        return;

    m_firstPartyForCookies = URL({ }, firstPartyForCookies);
}

Comment on attachment 390285 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390285&action=review

> Source/WTF/wtf/persistence/PersistentCoders.h:135
> +        tmp.reserveInitialCapacity(*size);

You added reserveInitialCapacity here.
It is dangerous to allocate memory without check bufferIsLargeEnoughToContain.
Use tryReserveCapacity and check the result.

> Source/WTF/wtf/persistence/PersistentCoders.h:201
> +        tempHashMap.reserveInitialCapacity(static_cast<unsigned>(*hashMapSize));

HashMap should have tryReserveCapacity method.

I removed my reserveInitialCapacity "optimization".
We could definitely add a flag to the decoder, but we would still want to decode to Optional<T> in case it fails.  The flag would prevent us from having to check every time we call operator>>.

Created attachment 396137 [details]
Patch

Created attachment 396139 [details]
Patch

Created attachment 396141 [details]
Patch

(In reply to Alex Christensen from comment #14)
> Created attachment 396141 [details]
> Patch

Sorry, this need to be rebased after r259917.

(In reply to David Kilzer (:ddkilzer) from comment #15)
> (In reply to Alex Christensen from comment #14)
> > Created attachment 396141 [details]
> > Patch
> 
> Sorry, this need to be rebased after r259917.

Actually...it might be okay.  I don't think this patch changes the same lines of code that r259917 did.

Comment on attachment 396141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396141&action=review

> Source/WebCore/platform/network/ResourceLoadPriority.h:69
> +template<> struct EnumTraits<WebCore::ResourceLoadPriority> {
> +    using values = EnumValues<
> +        WebCore::ResourceLoadPriority,
> +        WebCore::ResourceLoadPriority::VeryLow,
> +        WebCore::ResourceLoadPriority::Low,
> +        WebCore::ResourceLoadPriority::Medium,
> +        WebCore::ResourceLoadPriority::High,
> +        WebCore::ResourceLoadPriority::VeryHigh
> +    >;
> +};

I don't think MSVC++ likes this syntax:

ResourceRequestCFNet.cpp
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,29): error C2988: unrecognizable template declaration/definition (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,29): error C2143: syntax error: missing ';' before '<' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,12): error C2913: explicit specialization; 'WTF::EnumTraits' is not a specialization of a class template (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,29): error C2059: syntax error: '<' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,61): error C2143: syntax error: missing ';' before '{' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

<https://ews-build.webkit.org/#/builders/10/builds/14701>

Does MSVC++ not like the type specifier on the enum class?

enum class ResourceLoadPriority : uint8_t {
    [...]
};

Comment on attachment 396141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396141&action=review

>> Source/WebCore/platform/network/ResourceLoadPriority.h:69
>> +};
> 
> I don't think MSVC++ likes this syntax:
> 
> ResourceRequestCFNet.cpp
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,29): error C2988: unrecognizable template declaration/definition (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,29): error C2143: syntax error: missing ';' before '<' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,12): error C2913: explicit specialization; 'WTF::EnumTraits' is not a specialization of a class template (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,29): error C2059: syntax error: '<' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\ResourceLoadPriority.h(60,61): error C2143: syntax error: missing ';' before '{' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\network\cf\ResourceRequestCFNet.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> 
> <https://ews-build.webkit.org/#/builders/10/builds/14701>
> 
> Does MSVC++ not like the type specifier on the enum class?
> 
> enum class ResourceLoadPriority : uint8_t {
>     [...]
> };

Oh, maybe it needs this preceding it:

template<typename T> struct EnumTraits;

Comment on attachment 396141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396141&action=review

> Source/WebCore/platform/network/ResourceRequestBase.h:419
> +        WebCore::ResourceRequestBase::Requester::ImportScripts

Yes, I need to include EnumTraits.h to fix the Windows build.  I also need to include Ping and Beacon here to fix the tests.

Created attachment 396148 [details]
Patch

Created attachment 396152 [details]
Patch

Committed r259922: <https://trac.webkit.org/changeset/259922>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396152 [details].

<rdar://problem/61625504>

(In reply to Fujii Hironori from comment #9)
> How about idea using Optional only for non-default-constructable class?

I like this idea.