Bug 99627 – Move ResourceRequest construction out of SubresourceLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

CachedResource::load() fills out a bunch of http headers.  SubresourceLoader::create() adds a bunch more.  I don't think there's a good reason for these to be set in 2 different places.  Merging them also means one less copy.

By putting everything into a helper in CachedResource, it would also be cleaner to override what headers are set for different types of resources which will be useful for adding main resources to the cache.

The downside is that it complicates CachedRawResource::canReuse().  Because a bunch of additional headers are now set on CachedResource::m_resourceRequest, we will need to be a little more careful in terms of what headers force a load from network for CachedRawResources.

[Nate Chapin]

Created attachment 169234 [details]
patch

[Adam Barth]

Comment on attachment 169234 [details]
patch

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

> Source/WebCore/ChangeLog:10
> +        Note that this merge requires a bit more care in CachedRawReosurce::canReuse(),

typo: CachedRawReosurce

[Adam Barth]

Comment on attachment 169234 [details]
patch

This looks reasonable, but this code melts my brain.

[Adam Barth]

Comment on attachment 169234 [details]
patch

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

> Source/WebCore/loader/cache/CachedRawResource.cpp:132
> +static bool shouldIgnoreHeaderForCacheReuse(AtomicString headerName)

Where did this list of headers come from?

[Nate Chapin]

(In reply to comment #4)
> (From update of attachment 169234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169234&action=review
> 
> > Source/WebCore/loader/cache/CachedRawResource.cpp:132
> > +static bool shouldIgnoreHeaderForCacheReuse(AtomicString headerName)
> 
> Where did this list of headers come from?

It's the new headers that are set on CachedResource::m_resourceRequest, that are currently set directly or indirectly by SubsourceLoader::create() (the indirect ones are from FrameLoader::addExtraFieldsToRequest()).

[Adam Barth]

Comment on attachment 169234 [details]
patch

Ok.  The list of headers seems a bit hacky...

[Nate Chapin]

(In reply to comment #6)
> (From update of attachment 169234 [details])
> Ok.  The list of headers seems a bit hacky...

Yep, but I didn't have a better idea. :(

[Adam Barth]

Neither do I.

[Nate Chapin]

Created attachment 169271 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169271 [details]
Patch for landing

Clearing flags on attachment: 169271

Committed r131660: <http://trac.webkit.org/changeset/131660>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.