Currently PLT ignores beacon and ping loads by checking the request priority by checking if CFURLRequestGetRequestPriority returns 0. This won't work anymore because I plan on giving both VeryLow (which is used by beacons and pings) and Low loads a CFURLRequestPriority of 0 in the fix for https://bugs.webkit.org/show_bug.cgi?id=203423.
To work around this, mark very-low pri loads using a key in the request dictionary. I think this is okay since ping and beacon requests are relatively rare, and we're already stuffing a bunch of other keys into the request dictionaries already.
Created attachment 382246 [details]
Comment on attachment 382246 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=382246&action=review
> + Mark very-low-pri beacon/ping loads using a request dictionary key. PLT currently ignores
Note that PLT has to somehow support old and new WebKit builds so whatever changes you make need to be compatible.
> +// Used by PLT to ignore very low pri beacon/ping loads via _CFURLRequestCopyProtocolPropertyForKey
Comments in WebKit need to end with a period. Also no abbreviations (like "pri").
> +#define kWebKitVeryLowLoadPriority CFSTR("WKVeryLowLoadPri")
We normally do not use prefixes (like k) for our constants in WebKit. We also avoid abbreviations. Could we use WKVeryLowLoadPriority instead of WKVeryLowLoadPri ?
> Note that PLT has to somehow support old and new WebKit builds so whatever changes you make need to be compatible.
What is the window for compatibility? Is being able to use PLT with a one week old build of WebKit good enough?
(In reply to Ben Nham from comment #3)
> > Note that PLT has to somehow support old and new WebKit builds so whatever changes you make need to be compatible.
> What is the window for compatibility? Is being able to use PLT with a one
> week old build of WebKit good enough?
Please discuss offline with PLT maintainers but my understanding is that we need to keep supporting very old builds of WebKit.
Created attachment 382261 [details]
I talked with some of the PLT maintainers and they actually said it'd be fine if I landed both the Safari and WebKit changes at the same time. It would break perf automation for at most one build since they build Safari and WebKit at the same time. Although you can manually ask for new Safari with old WebKit (or vice versa) via something like a bisection test that holds one constant while the other varies, in practice this doesn't actually work right now for various reasons.
I still feel better if we have no breakage at all, so I'd like to push out this WK change, let it sit for a couple of days, and then push out the Safari PLT change that depends on the new key that I've added.
I've also addressed style concerns in newest patch.
Comment on attachment 382261 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=382261&action=review
> +#define WKVeryLowLoadPriorityKey CFSTR("WKVeryLowLoadPriority")
This is not actually in CFNetwork. This should not be defined in CFNetworkSPI.h.
> +CFTypeRef _CFURLRequestCopyProtocolPropertyForKey(CFURLRequestRef, CFStringRef);
This is unused.
Created attachment 382505 [details]
Comment on attachment 382505 [details]
Clearing flags on attachment: 382505
Committed r251954: <https://trac.webkit.org/changeset/251954>
All reviewed patches have been landed. Closing bug.