[webkit-reviews] review granted: [Bug 135548] QuickLook resources are cache-replaced with their original binary data causing ASSERT(m_data->size() == newBuffer->size()) in CachedResource.cpp : [Attachment 235942] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Pratik Solanki
<psolanki at apple.com>'s request for review:
Bug 135548: QuickLook resources are cache-replaced with their original binary
data causing ASSERT(m_data->size() == newBuffer->size()) in CachedResource.cpp
https://bugs.webkit.org/show_bug.cgi?id=135548

Attachment 235942: Patch
https://bugs.webkit.org/attachment.cgi?id=235942&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235942&action=review


r=me, but please removed the unused method disableEncodedDataReplacement()
unless there is a missing source file for this patch!

> Source/WebCore/loader/ResourceLoader.cpp:67
> +#if USE(QUICK_LOOK)
> +    , m_quickLookResource(false)
> +#endif

It would make the code a bit more readable if you removed this code from
USE(QUICK_LOOK) and only set m_quickLookResource to true in
ResourceLoader::didCreateQuickLookHandle().

> Source/WebCore/loader/ResourceLoader.h:227
> +#if USE(QUICK_LOOK)
> +public:
> +    bool isQuickLookResource() { return m_quickLookResource; }
> +private:
> +    bool m_quickLookResource;
> +#endif

Ditto.

> Source/WebCore/loader/cache/CachedRawResource.cpp:45
> +#if USE(QUICK_LOOK)
> +    , m_allowEncodedDataReplacement(true)
> +#endif

It would make the code a bit more readable if you removed this code from
USE(QUICK_LOOK) and only set m_allowEncodedDataReplacement to true in
CachedRawResource::finishLoading().

> Source/WebCore/loader/cache/CachedRawResource.h:51
> +#if USE(QUICK_LOOK)
> +    void disableEncodedDataReplacement() { m_allowEncodedDataReplacement =
false; }
> +#endif

This new method doesn't appear to be used anywhere?

> Source/WebCore/loader/cache/CachedRawResource.h:71
> +#if USE(QUICK_LOOK)
> +    virtual bool mayTryReplaceEncodedData() const override { return
m_allowEncodedDataReplacement; }
> +#else
>      virtual bool mayTryReplaceEncodedData() const override { return true; }
> +#endif

It would make the code a bit more readable if you removed this code from
USE(QUICK_LOOK) and only set m_allowEncodedDataReplacement to true in
CachedRawResource::finishLoading().

> Source/WebCore/loader/cache/CachedRawResource.h:86
> +#if USE(QUICK_LOOK)
> +    bool m_allowEncodedDataReplacement;
> +#endif

Ditto.