[webkit-reviews] review granted: [Bug 48684] Should make use of purge priorities for different resource types : [Attachment 72581] Patch.v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Pratik Solanki <psolanki at apple.com>'s
request for review:
Bug 48684: Should make use of purge priorities for different resource types
https://bugs.webkit.org/show_bug.cgi?id=48684

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72581&action=review

Looks fine to me, but I’m not setting commit-queue+ at this time because you
may want to make some of the changes I suggest here.

> WebCore/loader/CachedCSSStyleSheet.h:63
>      protected:
> +	   virtual PurgePriority resourcePurgePriority() const { return
PurgeLast; }
> +
>	   RefPtr<TextResourceDecoder> m_decoder;
>	   String m_decodedSheetText;

I think these members should be private, not protected, unless there is a
derived class that needs to access these.

> WebCore/loader/CachedImage.h:91
> +protected:
> +    virtual PurgePriority resourcePurgePriority() const { return PurgeFirst;
}

This should be private, not protected, unless derived classes have to call it
for some reason.

> WebCore/loader/CachedResource.h:212
> +    virtual PurgePriority resourcePurgePriority() const { return
PurgeDefault; }

Given that this class is already named CachedResource, it seems this message
should be called purgePriority, not resourcePurgePriority. The prefix doesn’t
add anything.

This function should be private, not protected. There is no reason for a
derived class to call it.

> WebCore/loader/CachedScript.h:56
> +    protected:
> +	   virtual PurgePriority resourcePurgePriority() const { return
PurgeLast; }

This should be private, not protected.