[Webkit-unassigned] [Bug 144326] ResourceLoadPriority should be enum class

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144326

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #251943|review?                     |review+
              Flags|                            |

--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 251943
  --> https://bugs.webkit.org/attachment.cgi?id=251943
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150429/0ebe01e6/attachment-0001.html>