<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - ResourceLoadPriority should be enum class"
href="https://bugs.webkit.org/show_bug.cgi?id=144326">bug 144326</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #251943 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - ResourceLoadPriority should be enum class"
href="https://bugs.webkit.org/show_bug.cgi?id=144326#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - ResourceLoadPriority should be enum class"
href="https://bugs.webkit.org/show_bug.cgi?id=144326">bug 144326</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=251943&action=diff" name="attach_251943" title="patch">attachment 251943</a> <a href="attachment.cgi?id=251943&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251943&action=review">https://bugs.webkit.org/attachment.cgi?id=251943&action=review</a>
<span class="quote">> Source/WebCore/loader/ResourceLoadScheduler.cpp:261
> + copyValuesToVector(m_hosts, hostsToServe);
> +
> + for (auto* host : hostsToServe) {</span >
I really wish we had an accessor named copiedValues() that did this so we could write:
for (auto* host : m_hosts.copiedValues())
<span class="quote">> Source/WebCore/loader/ResourceLoadScheduler.cpp:275
> + HostInformation::RequestQueue& requestsPending = host->requestsPending(priority);</span >
auto&?
<span class="quote">> 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) {</span >
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.
<span class="quote">> 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;
> }
> }</span >
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?
<span class="quote">> Source/WebCore/loader/ResourceLoadScheduler.h:109
> + std::array<RequestQueue, static_cast<int>(ResourceLoadPriority::Highest) + 1> m_requestsPending;</span >
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.
<span class="quote">> Source/WebCore/platform/network/ResourceLoadPriority.h:41
> +inline ResourceLoadPriority operator++(ResourceLoadPriority& priority)</span >
The return value should be a reference too, since this is the prefix form.
<span class="quote">> Source/WebCore/platform/network/ResourceLoadPriority.h:47
> +inline ResourceLoadPriority operator--(ResourceLoadPriority& priority)</span >
The return value should be a reference too, since this is the prefix form.
<span class="quote">> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:154
> </span >
Extra blank line.
<span class="quote">> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.messages.in:31
> + </span >
Stray whitespace on this line that looks like a blank line. Git would tell you to remove it by painting it red.
<span class="quote">> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:640
> void InjectedBundle::dispatchPendingLoadRequests()
> {
> - resourceLoadScheduler()->servePendingRequests();
> }</span >
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.
<span class="quote">> 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);
> }</span >
Same question. Can we remove this entirely?a
<span class="quote">> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.h:55
> - virtual void servePendingRequests(WebCore::ResourceLoadPriority minimumPriority = WebCore::ResourceLoadPriorityVeryLow) override;
> + virtual void servePendingRequests(WebCore::ResourceLoadPriority minimumPriority) override;</span >
Same question. Can we remove this entirely?a</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>