Bug 141186 – Drop ResourceLoadPriorityUnresolved resource load priority and use Optional<> instead

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Make ResourceLoadPriorityUnresolved a CachedResourceRequest-specific priority as it is not a valid value for ResourceRequest. This makes it clearer ResourceRequest::priority() cannot be ResourceLoadPriorityUnresolved and thus cannot be negative.

A new CachedResourceLoadPriority enum is introduced for CachedResourceRequest's priority. ResourceLoadPriorityUnresolved is also renamed to CachedResourceLoadPriorityTypeDefault as it makes its meaning clearer. If CachedResourceLoadPriorityTypeDefault priority is set on the CachedResourceRequest, it is later going to be converted to a ResourceLoadPriority by selecting the default priority for the CachedResource's type.

[Chris Dumez]

Created attachment 245923 [details]
Patch

[Antti Koivisto]

Comment on attachment 245923 [details]
Patch

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

> Source/WebCore/platform/network/ResourceLoadPriority.h:44
> +enum CachedResourceLoadPriority {
> +    CachedResourceLoadPriorityTypeDefault = -1,
> +    CachedResourceLoadPriorityVeryLow = 0,
> +    CachedResourceLoadPriorityLow,
> +    CachedResourceLoadPriorityMedium,
> +    CachedResourceLoadPriorityHigh,
> +    CachedResourceLoadPriorityVeryHigh,
> +};
> +
>  enum ResourceLoadPriority {
> -    // The unresolved priority is here for the convenience of the clients. It should not be passed to the ResourceLoadScheduler.
> -    ResourceLoadPriorityUnresolved = -1,
> -    ResourceLoadPriorityVeryLow = 0,
> +    ResourceLoadPriorityVeryLow,
>      ResourceLoadPriorityLow,
>      ResourceLoadPriorityMedium,
>      ResourceLoadPriorityHigh,

It seems pretty unfortunate to have two of these. Could we find other solution, for example by replacing uses of CachedResourceLoadPriority with Optional<ResourceLoadPriority>?

Defining CachedResourceLoadPriority here is a (minor) platform violation. Nothing in platform/ shouldn't know about CachedResources. The definition should probably be in CachedResources.h or thereabouts.

[Chris Dumez]

Created attachment 245964 [details]
Patch

[Antti Koivisto]

Comment on attachment 245964 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.h:54
> +    void setPriority(Optional<ResourceLoadPriority> priority) { m_priority = WTF::move(priority); }

I can't find any clients for this. Maybe it is not used and could be removed?

> Source/WebCore/platform/network/ResourceRequestBase.h:226
> +        unsigned m_allowCookies : 1;
> +        mutable unsigned m_resourceRequestUpdated : 1;
> +        mutable unsigned m_platformRequestUpdated : 1;
> +        mutable unsigned m_resourceRequestBodyUpdated : 1;
> +        mutable unsigned m_platformRequestBodyUpdated : 1;
> +        unsigned m_reportUploadProgress : 1;
> +        unsigned m_reportLoadTiming : 1;
> +        unsigned m_reportRawHeaders : 1;
> +        unsigned m_hiddenFromInspector : 1;
> +        unsigned m_priority : 4;

These could probably just be regular fields instead of bitfields. I don't think there is meaningful memory savings here.

[Chris Dumez]

Created attachment 245978 [details]
Patch

[Chris Dumez]

Created attachment 245979 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 245979 [details]
Patch

Clearing flags on attachment: 245979

Committed r179584: <http://trac.webkit.org/changeset/179584>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.