Bug 144326 – ResourceLoadPriority should be enum class

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Antti Koivisto]

modernize

[Antti Koivisto]

Created attachment 251930 [details]
patch

[Antti Koivisto]

Created attachment 251939 [details]
patch

[Antti Koivisto]

Created attachment 251943 [details]
patch

[Darin Adler]

Comment on attachment 251943 [details]
patch

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

> Source/WebCore/loader/ResourceLoadScheduler.cpp:261
> +    copyValuesToVector(m_hosts, hostsToServe);
> +
> +    for (auto* host : hostsToServe) {

I really wish we had an accessor named copiedValues() that did this so we could write:

    for (auto* host : m_hosts.copiedValues())

> Source/WebCore/loader/ResourceLoadScheduler.cpp:275
> +        HostInformation::RequestQueue& requestsPending = host->requestsPending(priority);

auto&?

> Source/WebCore/loader/ResourceLoadScheduler.cpp:377
> -    for (int priority = ResourceLoadPriorityHighest; priority >= ResourceLoadPriorityLowest; --priority) {  
> -        RequestQueue::iterator end = m_requestsPending[priority].end();
> -        for (RequestQueue::iterator it = m_requestsPending[priority].begin(); it != end; ++it) {
> +    for (auto& requestQueue : m_requestsPending) {

This now goes from lowest to highest priority. But the old code went from highest to lowest. Is that an intentional change? Is it OK?

The fact that this goes in a particular priority order is now much less clear than in the old code. I think this needs a comment, and maybe even an assertion.

> Source/WebCore/loader/ResourceLoadScheduler.cpp:383
> +        for (auto it = requestQueue.begin(), end = requestQueue.end(); it != end; ++it) {
>              if (*it == resourceLoader) {
> -                m_requestsPending[priority].remove(it);
> +                requestQueue.remove(it);
>                  return;
>              }
>          }

It’s a little strange that we have Vector::find but we don’t have Deque::find, so we have to write out this loop. I suggest a helper function or adding something to Deque. Alternatively, I always wonder when I see a loop like this if maybe we are using the wrong data structure. Perhaps this should be a ListHashSet instead of a Deque?

> Source/WebCore/loader/ResourceLoadScheduler.h:109
> +        std::array<RequestQueue, static_cast<int>(ResourceLoadPriority::Highest) + 1> m_requestsPending;

Might be nice to have a constant for the number of resource load priorities, so the expression "Highest + 1" would be there instead of here.

> Source/WebCore/platform/network/ResourceLoadPriority.h:41
> +inline ResourceLoadPriority operator++(ResourceLoadPriority& priority)

The return value should be a reference too, since this is the prefix form.

> Source/WebCore/platform/network/ResourceLoadPriority.h:47
> +inline ResourceLoadPriority operator--(ResourceLoadPriority& priority)

The return value should be a reference too, since this is the prefix form.

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:154
>  

Extra blank line.

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.messages.in:31
> +        

Stray whitespace on this line that looks like a blank line. Git would tell you to remove it by painting it red.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:640
>  void InjectedBundle::dispatchPendingLoadRequests()
>  {
> -    resourceLoadScheduler()->servePendingRequests();
>  }

Why can’t we remove this entirely? If this is unused it seems we should be removing more. If it is used, then we shouldn’t be removing it.

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:249
> -void WebResourceLoadScheduler::servePendingRequests(ResourceLoadPriority minimumPriority)
> +void WebResourceLoadScheduler::servePendingRequests(ResourceLoadPriority)
>  {
> -    LOG(NetworkScheduling, "(WebProcess) WebResourceLoadScheduler::servePendingRequests");
> -    
> -    // The NetworkProcess scheduler is good at making sure loads are serviced until there are no more pending requests.
> -    // If this WebProcess isn't expecting requests to be served then we can ignore messaging the NetworkProcess right now.
> -    if (m_suspendPendingRequestsCount)
> -        return;
> -
> -    WebProcess::singleton().networkConnection()->connection()->send(Messages::NetworkConnectionToWebProcess::ServePendingRequests(minimumPriority), 0);
>  }

Same question. Can we remove this entirely?a

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.h:55
> -    virtual void servePendingRequests(WebCore::ResourceLoadPriority minimumPriority = WebCore::ResourceLoadPriorityVeryLow) override;
> +    virtual void servePendingRequests(WebCore::ResourceLoadPriority minimumPriority) override;

Same question. Can we remove this entirely?a

[Antti Koivisto]

> > Source/WebCore/loader/ResourceLoadScheduler.cpp:377
> > -    for (int priority = ResourceLoadPriorityHighest; priority >= ResourceLoadPriorityLowest; --priority) {  
> > -        RequestQueue::iterator end = m_requestsPending[priority].end();
> > -        for (RequestQueue::iterator it = m_requestsPending[priority].begin(); it != end; ++it) {
> > +    for (auto& requestQueue : m_requestsPending) {
> 
> This now goes from lowest to highest priority. But the old code went from
> highest to lowest. Is that an intentional change? Is it OK?
> 
> The fact that this goes in a particular priority order is now much less
> clear than in the old code. I think this needs a comment, and maybe even an
> assertion.

It is intentional change, the order doesn't matter (it is a search and there is maximum one hit). 

I don't know why the old code used reverse order.


> It’s a little strange that we have Vector::find but we don’t have
> Deque::find, so we have to write out this loop. I suggest a helper function
> or adding something to Deque. Alternatively, I always wonder when I see a
> loop like this if maybe we are using the wrong data structure. Perhaps this
> should be a ListHashSet instead of a Deque?

I think that is a decent argument for not having that helper on Deque (or Vector). This should probably be a ListHashSet.

> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:640
> >  void InjectedBundle::dispatchPendingLoadRequests()
> >  {
> > -    resourceLoadScheduler()->servePendingRequests();
> >  }
> 
> Why can’t we remove this entirely? If this is unused it seems we should be
> removing more. If it is used, then we shouldn’t be removing it.

This is part of the bundle API. Need to get rid of clients (if any before it can be removed).

> Same question. Can we remove this entirely?a

These override virtual functions with implementations. We want these functions to do nothing. 

We should really get rid of the whole thing where we have WebResourceLoadScheduler inheriting WebCore::WebResourceLoadScheduler.

[Antti Koivisto]

https://trac.webkit.org/r183563