Bug 203594 – Mark VeryLow priority requests using a request dictionary key

• Component: Page Loading
• Status: RESOLVED FIXED

[Ben Nham]

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.

[Ben Nham]

Created attachment 382246 [details]
Patch

[Chris Dumez]

Comment on attachment 382246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382246&action=review

> Source/WebCore/PAL/ChangeLog:8
> +        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.

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:252
> +// 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").

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:253
> +#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 ?

[Ben Nham]

> 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?

[Chris Dumez]

(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.

[Ben Nham]

Created attachment 382261 [details]
Patch

[Ben Nham]

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.

[Alex Christensen]

Comment on attachment 382261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382261&action=review

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:253
> +#define WKVeryLowLoadPriorityKey CFSTR("WKVeryLowLoadPriority")

This is not actually in CFNetwork.  This should not be defined in CFNetworkSPI.h.

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:308
> +CFTypeRef _CFURLRequestCopyProtocolPropertyForKey(CFURLRequestRef, CFStringRef);

This is unused.

[Ben Nham]

Created attachment 382505 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382505 [details]
Patch

Clearing flags on attachment: 382505

Committed r251954: <https://trac.webkit.org/changeset/251954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56832923>