Bug 107995

Summary: Add didChangePriority() to ResourceHandle
Product: WebKit Reporter: James Simonsen <simonjam>
Component: New BugsAssignee: James Simonsen <simonjam>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, dglazkov, fishd, jamesr, japhet, tkent+wkapi, webkit-ews, webkit.review.bot, willchan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description James Simonsen 2013-01-25 17:25:53 PST
Notify ResourceHandle when the requested resource is no longer only for preload
Comment 1 Alexey Proskuryakov 2013-01-25 19:34:24 PST
This is adding more layering violations to WebKit :(
Comment 2 James Simonsen 2013-01-28 11:41:27 PST
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?
Comment 3 Alexey Proskuryakov 2013-01-28 20:26:01 PST
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.
Comment 4 James Simonsen 2013-01-29 11:34:18 PST
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
Comment 5 James Simonsen 2013-02-01 16:39:35 PST
Created attachment 186184 [details]
Patch
Comment 6 WebKit Review Bot 2013-02-01 16:44:07 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 7 Early Warning System Bot 2013-02-01 16:52:59 PST
Comment on attachment 186184 [details]
Patch

Attachment 186184 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16342242
Comment 8 James Simonsen 2013-02-01 16:57:40 PST
Created attachment 186188 [details]
Patch
Comment 9 WebKit Review Bot 2013-02-01 23:41:42 PST
Comment on attachment 186188 [details]
Patch

Clearing flags on attachment: 186188

Committed r141684: <http://trac.webkit.org/changeset/141684>
Comment 10 WebKit Review Bot 2013-02-01 23:41:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Adam Barth 2013-02-02 00:27:03 PST
> 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.