Bug 107995 – Add didChangePriority() to ResourceHandle

• Component: New Bugs
• Status: RESOLVED FIXED

[James Simonsen]

Notify ResourceHandle when the requested resource is no longer only for preload

[Alexey Proskuryakov]

This is adding more layering violations to WebKit :(

[James Simonsen]

This was just my idea. I'm not stuck on it. What's the correct way to do it without violating layering?

Basically, we'd like Chromium's network stack to know:

1. When a resource is being speculatively loaded (from the preload scanner).

2. When a preload becomes non-speculative (the parser discovers it).

We need these to properly prioritize requests.

Another idea I had was to have WebCore dynamically change the resource's priority. Is that cleaner?

[Alexey Proskuryakov]

ResourceHandle is a cross-platform wrapper around platform networking library, which is not designed to have any idea about Web concepts. It is a huge layering violation that Chromium uses it as IPC injection point, and marshals more and more high level information through it.

I don't know what the good way forward is, given that this has been articulated years ago, and Chromium project leaders apparently decided to stay on the wrong course.

[James Simonsen]

That discussion apparently occurred way before I joined. I don't know anything about how that decision was made.

Regardless, any networking layer should care about this. It's important that the speculative preloads remain a lower priority than normal loads. Messing this up leads to a 5% drop in Speed Index [1] performance. No matter which network stack you're using, it needs to know the proper priority for preloads.

The priority is already included in the ResourceRequest. The only pieces missing are identifying preloads and updating their priority when a preload becomes a normal load. I don't care whether that signal uses the term "preload" or just dynamically changes the priority.

From my perspective, ResourceHandle seemed like the obvious place for that signal. I'd called it promotePreloadToNormalLoad(), but we could call it didChangeRequestPriority(newPriority) instead. WDYT?

[1] A measure of amount of pixels rendered during page load. https://sites.google.com/a/webpagetest.org/docs/using-webpagetest/metrics/speed-index

[James Simonsen]

Created attachment 186184 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 186184 [details]
Patch

Attachment 186184 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16342242

[James Simonsen]

Created attachment 186188 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186188 [details]
Patch

Clearing flags on attachment: 186188

Committed r141684: <http://trac.webkit.org/changeset/141684>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> I don't know what the good way forward is, given that this has been articulated years ago, and Chromium project leaders apparently decided to stay on the wrong course.

Instead, I would say that we disagree about which course is right.