[webkit-reviews] review granted: [Bug 175597] XMLHttpRequest should not sniff content encoding : [Attachment 325234] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 27 21:23:17 PDT 2017


JF Bastien <jfbastien at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 175597: XMLHttpRequest should not sniff content encoding
https://bugs.webkit.org/show_bug.cgi?id=175597

Attachment 325234: Patch

https://bugs.webkit.org/attachment.cgi?id=325234&action=review




--- Comment #16 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 325234
  --> https://bugs.webkit.org/attachment.cgi?id=325234
Patch

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

A small suggestion and bugs look sad:

/home/ews/ltilve-gtk-wk2-ews/WebKit/Source/WebKit/NetworkProcess/NetworkDataTas
k.cpp:59:206: error: no matching function for call to
'WebKit::NetworkDataTaskSoup::create(WebKit::NetworkSession&,
WebKit::NetworkDataTaskClient&, const WebCore::ResourceRequest&, const
WebCore::StoredCredentialsPolicy&, const
WebCore::ContentEncodingSniffingPolicy&, const bool&)'

Otherwise r=me

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:211
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101302

This doesn't also affect iOS?

> Source/WebCore/loader/ResourceLoader.cpp:219
> +    m_handle = ResourceHandle::create(frameLoader()->networkingContext(),
m_request, this, m_defersLoading, m_options.sniffContent == SniffContent,
m_options.sniffContentEncoding == ContentEncodingSniffingPolicy::Sniff);

I tend to like enum classes rather than booleans for this type of thing:
passing in multiple booleans is error prone and harder to read, whereas having
an enum class as parameter can't be messed up. So I'd just pass the
ContentEncodingSnifingPolicy strait into create(). Is that not practical?

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:480
> +    bool shouldContentEncodingSniff = true;

Even here, it's way more fool-proof to have an enum class.


More information about the webkit-reviews mailing list